Add CallNode field to NodeContext

[andyw8]

When we listen for node events, there are some situations where we are interested in not just the node, but the surrounding context. For cases such as a DSL call has_many :users, we make a definition request for the :users symbol, but we also want to know the surrounding method (has_many in this case). This will allow features such as Shopify/ruby-lsp-rails#397

The implementation is fairly simple: While iterating over the queue, if the parent is a CallNode, then we keep track of that. So at the end of the iteration, we have the innermost CallNode, which we can then pass to NodeContext.new.

[andyw8]

I feel there may be situations that aren't properly handled with this approach, but haven't found a concrete one yet. 🤔

[vinistock]

What if the target itself is a call node? Does it find the right surrounding one?

For example:

call1(call2)
# ^ this should be the surrounding
#      ^ this should be the target

Let's add a test for this case.

[andyw8]

Added a test and fixed the behaviour.

[vinistock]

The implementation looks good to me. Let's just test that possible corner case

[vinistock]

What if the target itself is a call node? Does it find the right surrounding one?

For example:

call1(call2)
# ^ this should be the surrounding
#      ^ this should be the target

Let's add a test for this case.

[st0012]

@andyw8 could you add some description in the PR(s)? Neither PR describes what feature/problem we're trying to solve and it's not easy to review with this little information 🙂

[andyw8]

I've expanded the PR description.

[andyw8]

(looking into the two failures, will set to draft)

[st0012]

What do we expect in these cases:

foo do
  123 # this is the target
end

foo(
  if bar
    123 # this is the target
  end
end

Are we expecting foo being the call node in both cases?

[andyw8]

@st0012 good question. This doesn't handle integers, but if we imagine those are symbols or string instead, then I'd say NodeContext#call_node should be nil for each, i.e. we only care about the call_node if it's the immediate parent. I've pushed an update to handle that case.

[andyw8]

(the first case could be debatable... foo isn't the immediate parent, but for some situations it might still be useful to make it available as the call_node. But I think we should wait until there are use cases rather than trying to predict what might be needed)

[andyw8]

@st0012 after pairing with @vinistock, we've changed the approach so that we do match non-immediate parents. It doesn't add much too much complexity, and allows us to have more flexibilities for future use cases.

[vinistock]

Nice

[vinistock]

Let's add a comment so that we remember why this adjustment is necessary.

[andyw8]

(merged on the command line since the labels check seemed to be stuck)

[st0012]

This doesn't handle integers, but if we imagine those are symbols or string instead

Not a blocker, but what are the criteria for the types of node to have this field?

so that we do match non-immediate parents.

In that case, I think it should be named closest_call_node instead?

[andyw8]

Not a blocker, but what are the criteria for the types of node to have this field?

It's hard to say since we can't anticipate all the possible things an addon author might want to build. Currently I just have the Rails DSLs in mind. We can consider others case-by-case.

In that case, I think it should be named closest_call_node instead?

I'm open to changing it to that. @vinistock ?