Conversation
Gankra
commented
Dec 17, 2025
Comment on lines
+45
to
+51
| // When we visit the root it won't `enter_node` so do that manually. | ||
| // Can this check fail? I don't know. It's fine. | ||
| let ancestors = if root.range().contains_range(range) { | ||
| vec![root] | ||
| } else { | ||
| Vec::new() | ||
| }; |
Contributor
Author
There was a problem hiding this comment.
This change was required to make ModModule be included, or else we couldn't find the sibling Statement in a global attribute of a module. This had a drive-by side-effect on selection_range that I'm not sure if it's correct/problematic.
Member
There was a problem hiding this comment.
How's this different from the check on line 59? Can we remove the check on line 59?
I think a better solution is to add a walk_node method to the source_order module like this:
pub fn walk_node<'a, V>(visitor: &mut V, node: AnyNodeRef<'a>)
where
V: SourceOrderVisitor<'a> + ?Sized,
{
if visitor.enter_node(node).is_traverse() {
node.visit_source_order(visitor);
}
visitor.leave_node(node);
}and call that over node.visit_source_order and remove the check on line 59
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
Gankra
commented
Dec 17, 2025
Comment on lines
68
to
+76
| info[selection-range]: Selection Range 0 | ||
| --> main.py:1:1 | ||
| | | ||
| 1 | / | ||
| 2 | | x = 1 + 2 | ||
| | |__________^ | ||
| | | ||
|
|
||
| info[selection-range]: Selection Range 1 |
Contributor
Author
There was a problem hiding this comment.
drive-by side-effect: the ModModule is always a valid Even Bigger selection range. I don't use this API so I have no sense of if this is good or bad.
|
|
MichaReiser
approved these changes
Dec 18, 2025
Member
MichaReiser
left a comment
There was a problem hiding this comment.
Looks good, but I suggest that you use the existing suite helper to also cover if, try, ...
Comment on lines
+45
to
+51
| // When we visit the root it won't `enter_node` so do that manually. | ||
| // Can this check fail? I don't know. It's fine. | ||
| let ancestors = if root.range().contains_range(range) { | ||
| vec![root] | ||
| } else { | ||
| Vec::new() | ||
| }; |
Member
There was a problem hiding this comment.
How's this different from the check on line 59? Can we remove the check on line 59?
I think a better solution is to add a walk_node method to the source_order module like this:
pub fn walk_node<'a, V>(visitor: &mut V, node: AnyNodeRef<'a>)
where
V: SourceOrderVisitor<'a> + ?Sized,
{
if visitor.enter_node(node).is_traverse() {
node.visit_source_order(visitor);
}
visitor.leave_node(node);
}and call that over node.visit_source_order and remove the check on line 59
Comment on lines
68
to
+76
| info[selection-range]: Selection Range 0 | ||
| --> main.py:1:1 | ||
| | | ||
| 1 | / | ||
| 2 | | x = 1 + 2 | ||
| | |__________^ | ||
| | | ||
|
|
||
| info[selection-range]: Selection Range 1 |
Comment on lines
+182
to
+191
| let parent_body = if let Some(module) = parent.as_mod_module() { | ||
| &module.body | ||
| } else if let Some(function) = parent.as_stmt_function_def() { | ||
| // This is important for `self.a = ...` in e.g. an initializer | ||
| &function.body | ||
| } else if let Some(class) = parent.as_stmt_class_def() { | ||
| &class.body | ||
| } else { | ||
| return None; | ||
| }; |
Member
There was a problem hiding this comment.
Another case where it's real pain that our AST doesn't have a Suite node.
Especially, because it's non-trivial to find the suite to which a node belongs, e.g. in the case of a try statement that has multiple suites.
But, look what I've found:
ruff/crates/ruff_python_ast/src/traversal.rs
Lines 5 to 46 in e402e27
You might want to change it so that it also supports ModModule.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
I should have factored this better but this includes a drive-by move of find_node to ruff_python_ast so ty_python_semantic can use it too.
Test Plan
Snapshots galore