Skip to content
  • Kevin Newton's avatar
    b673b5b4
    [ruby/prism] Split up CallNode in target position · b673b5b4
    Kevin Newton authored
    In this commit we're splitting up the call nodes that were in target
    positions (that is, for loop indices, rescue error captures, and
    multi assign targets).
    
    Previously, we would simply leave the call nodes in place. This had
    the benefit of keeping the AST relatively simple, but had the
    downside of not being very explicit. If a static analysis tool wanted
    to only look at call nodes, it could easily be confused because the
    method would have 1 fewer argument than it would actually be called
    with.
    
    This also brings some consistency to the AST. All of the nodes in
    a target position are now *TargetNode nodes. These should all be
    treated the same, and the call nodes can now be treated the same.
    
    Finally, there is benefit to memory. Because being in a target
    position ensures we don't have some fields, we can strip down the
    number of fields on these nodes.
    
    So this commit introduces two new nodes: CallTargetNode and
    IndexTargetNode. For CallTargetNode we get to drop the opening_loc,
    closing_loc, arguments, and block. Those can never be present. We
    also get to mark their fields as non-null, so they will always be
    seen as present.
    
    The IndexTargetNode keeps around most of its fields but gets to
    drop both the name (because it will always be []=) and the
    message_loc (which was always super confusing because it included
    the arguments by virtue of being inside the []).
    
    Overall, this adds complexity to the AST at the expense of memory
    savings and explicitness. I believe this tradeoff is worth it in
    this case, especially because these are very much not common nodes
    in the first place.
    
    https://github.com/ruby/prism/commit/3ef71cdb45
    b673b5b4
    [ruby/prism] Split up CallNode in target position
    Kevin Newton authored
    In this commit we're splitting up the call nodes that were in target
    positions (that is, for loop indices, rescue error captures, and
    multi assign targets).
    
    Previously, we would simply leave the call nodes in place. This had
    the benefit of keeping the AST relatively simple, but had the
    downside of not being very explicit. If a static analysis tool wanted
    to only look at call nodes, it could easily be confused because the
    method would have 1 fewer argument than it would actually be called
    with.
    
    This also brings some consistency to the AST. All of the nodes in
    a target position are now *TargetNode nodes. These should all be
    treated the same, and the call nodes can now be treated the same.
    
    Finally, there is benefit to memory. Because being in a target
    position ensures we don't have some fields, we can strip down the
    number of fields on these nodes.
    
    So this commit introduces two new nodes: CallTargetNode and
    IndexTargetNode. For CallTargetNode we get to drop the opening_loc,
    closing_loc, arguments, and block. Those can never be present. We
    also get to mark their fields as non-null, so they will always be
    seen as present.
    
    The IndexTargetNode keeps around most of its fields but gets to
    drop both the name (because it will always be []=) and the
    message_loc (which was always super confusing because it included
    the arguments by virtue of being inside the []).
    
    Overall, this adds complexity to the AST at the expense of memory
    savings and explicitness. I believe this tradeoff is worth it in
    this case, especially because these are very much not common nodes
    in the first place.
    
    https://github.com/ruby/prism/commit/3ef71cdb45
Loading