Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CallNode field to NodeContext #2115

Merged
merged 1 commit into from
Jun 5, 2024
Merged

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented May 31, 2024

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 andyw8 force-pushed the andyw8/add-callnode-to-node-context branch 2 times, most recently from 68a19f7 to 464a482 Compare May 31, 2024 17:50
@andyw8 andyw8 added the chore Chore task label May 31, 2024
@andyw8 andyw8 force-pushed the andyw8/add-callnode-to-node-context branch 4 times, most recently from 60f5ed8 to b3ad3c3 Compare June 3, 2024 19:27
@andyw8 andyw8 changed the title WIP: Add CallNode field to NodeContext Add CallNode field to NodeContext Jun 3, 2024
@andyw8 andyw8 marked this pull request as ready for review June 3, 2024 19:35
@andyw8 andyw8 requested a review from a team as a code owner June 3, 2024 19:35
@andyw8 andyw8 requested review from st0012 and vinistock June 3, 2024 19:35
@@ -159,6 +160,10 @@ def locate(node, char_position, node_types: [])
nesting << candidate
end

if candidate.is_a?(Prism::CallNode)
call_node = candidate
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test and fixed the behaviour.

Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -159,6 +160,10 @@ def locate(node, char_position, node_types: [])
nesting << candidate
end

if candidate.is_a?(Prism::CallNode)
call_node = candidate
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

st0012 commented Jun 3, 2024

@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
Copy link
Contributor Author

andyw8 commented Jun 4, 2024

I've expanded the PR description.

@andyw8
Copy link
Contributor Author

andyw8 commented Jun 4, 2024

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

@andyw8 andyw8 marked this pull request as draft June 4, 2024 16:36
Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

andyw8 commented Jun 4, 2024

@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
Copy link
Contributor Author

andyw8 commented Jun 4, 2024

(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 andyw8 force-pushed the andyw8/add-callnode-to-node-context branch 3 times, most recently from 2fd6c34 to ed93c79 Compare June 5, 2024 16:25
@andyw8 andyw8 marked this pull request as ready for review June 5, 2024 16:26
@andyw8
Copy link
Contributor Author

andyw8 commented Jun 5, 2024

@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.

Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

Comment on lines +84 to +86
elsif target.is_a?(Prism::SymbolNode) && parent.is_a?(Prism::BlockArgumentNode)
target = parent
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@andyw8 andyw8 force-pushed the andyw8/add-callnode-to-node-context branch from ed93c79 to 840277f Compare June 5, 2024 16:55
@andyw8 andyw8 enabled auto-merge (squash) June 5, 2024 16:55
@andyw8 andyw8 added chore Chore task and removed chore Chore task labels Jun 5, 2024
@andyw8 andyw8 merged commit 2d0a508 into main Jun 5, 2024
32 checks passed
@andyw8 andyw8 deleted the andyw8/add-callnode-to-node-context branch June 5, 2024 17:28
@andyw8
Copy link
Contributor Author

andyw8 commented Jun 5, 2024

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

@st0012
Copy link
Member

st0012 commented Jun 5, 2024

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
Copy link
Contributor Author

andyw8 commented Jun 5, 2024

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 ?

@andyw8 andyw8 added the enhancement New feature or request label Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Chore task enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants