Fix resolveNodeHandle to check not only child nodes but ancestor nodes#4033
Fix resolveNodeHandle to check not only child nodes but ancestor nodes#4033jet2jet wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR improves Session.resolveNodeHandle by adding an ancestor-walk fallback to locate the exact AST node when the initially resolved node’s kind/end don’t match the expected values.
Changes:
- Added an ancestor traversal to attempt an exact node match before falling back to child traversal.
| if node.Kind != kind || node.End() != end { | ||
| // Try to find the exact node by walking ancestors | ||
| for parent := node.Parent; parent != nil && parent.Kind != ast.KindSourceFile; parent = parent.Parent { | ||
| if parent.Pos() == pos && parent.End() == end && parent.Kind == kind { | ||
| return parent, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
kind == ast.KindSourceFile is already checked in the previous line (1830). Therefore it is not necessary to check for parent with parent.kind == ast.KindSourceFile here.
| @@ -1839,6 +1839,12 @@ func (s *Session) resolveNodeHandle(program *compiler.Program, handle Handle[ast | |||
|
|
|||
| // Verify the kind and end match | |||
| if node.Kind != kind || node.End() != end { | |||
There was a problem hiding this comment.
Isn't the real issue here (at least part of the issue?) just that the original Pos() is not checked either?
There was a problem hiding this comment.
I think it is rare that pos is different, but for safety, I added the check with 1d8cc89 .
...But I'm thinking that using own implementation for node traversal may be better than using ast.GetNodeAtPosition due to ast.GetNodeAtPosition's implementation (ast.GetNodeAtPosition should not be changed because it came from Strada).
Moreover, having something like mapping between Node and NodeHandle may be better for performance and reliability (although it may cost the memory and have potentially memory leak).
Fixes #3938
Add ancestor traversals in
resolveNodeHandleto detect the actual Node.Since
ast.GetNodeAtPositiononly usespos(notposandend), some nodes may be hidden by their children (with samepos), such asPropertyAccessExpression(itself andexpressionnode may be the samepos, and ifexpressionis such asExpressionnode,ast.GetNodeAtPositionreturnsexpression, not the parentPropertyAccessExpression).This PR fixes those patterns.