Skip to content

Conversation

@eregon
Copy link
Member

@eregon eregon commented Dec 14, 2025

@tompng
Copy link
Member

tompng commented Dec 14, 2025

Looks like Proc generated by for in syntax is missing

@eregon eregon force-pushed the node_for branch 2 times, most recently from dd02f75 to 5d510f5 Compare December 14, 2025 17:53
@eregon
Copy link
Member Author

eregon commented Dec 14, 2025

The 2.7 failure is quite annoying: https://github.com/ruby/prism/actions/runs/20211767571/job/58018650394?pr=3808
It can't parse def inline_method = 42. We can't wrap that in eval otherwise Prism.node_for won't work on it.
I suppose we could use a separate file, but it feels messy.
I tried to repro locally but the main Gemfile doesn't even work in 2.7, so it seems difficult to repro & test locally.
I wish we could just drop 2.7, it has this partial kwargs separation which makes it pretty messy in general.

@Earlopain
Copy link
Collaborator

I wish we could just drop 2.7, it has this partial kwargs separation which makes it pretty messy in general.

This is something we should probably talk about sometime in the future. It was added for the convenience of rubocop as far as I'm aware and I appreciate that this happened. But I do not want to keep these long EOL versions supported forever.

For now I would just put this in a separate file like you said. You shoudl be able to use the version-specific gemfile with BUNDLE_GEMFILE=gemfiles/2.7/Gemfile, that works for me at least.

@eregon eregon force-pushed the node_for branch 2 times, most recently from 24bdf87 to 3c0b91c Compare December 14, 2025 19:45
@eregon
Copy link
Member Author

eregon commented Dec 14, 2025

Looks like Proc generated by for in syntax is missing

@tompng Do you mean:

$ bin/parse -e 'for e in foo; body; end'
@ ProgramNode (location: (1,0)-(1,23))
├── flags: ∅
├── locals: [:e]
└── statements:
    @ StatementsNode (location: (1,0)-(1,23))
    ├── flags: ∅
    └── body: (length: 1)
        └── @ ForNode (location: (1,0)-(1,23))
            ├── flags: newline
            ├── index:
            │   @ LocalVariableTargetNode (location: (1,4)-(1,5))
            │   ├── flags: ∅
            │   ├── name: :e
            │   └── depth: 0
            ├── collection:
            │   @ CallNode (location: (1,9)-(1,12))
            │   ├── flags: variable_call, ignore_visibility
            │   ├── receiver: ∅
            │   ├── call_operator_loc: ∅
            │   ├── name: :foo
            │   ├── message_loc: (1,9)-(1,12) = "foo"
            │   ├── opening_loc: ∅
            │   ├── arguments: ∅
            │   ├── closing_loc: ∅
            │   ├── equal_loc: ∅
            │   └── block: ∅
            ├── statements:
            │   @ StatementsNode (location: (1,14)-(1,18))
            │   ├── flags: ∅
            │   └── body: (length: 1)
            │       └── @ CallNode (location: (1,14)-(1,18))
            │           ├── flags: newline, variable_call, ignore_visibility
            │           ├── receiver: ∅
            │           ├── call_operator_loc: ∅
            │           ├── name: :body
            │           ├── message_loc: (1,14)-(1,18) = "body"
            │           ├── opening_loc: ∅
            │           ├── arguments: ∅
            │           ├── closing_loc: ∅
            │           ├── equal_loc: ∅
            │           └── block: ∅
            ├── for_keyword_loc: (1,0)-(1,3) = "for"
            ├── in_keyword_loc: (1,6)-(1,8) = "in"
            ├── do_keyword_loc: ∅
            └── end_keyword_loc: (1,20)-(1,23) = "end"

does not have a BlockNode?
I suppose we could return the ForNode#statements or probably better the ForNode itself.

@eregon
Copy link
Member Author

eregon commented Dec 14, 2025

or probably better the ForNode itself.

Implemented.
@tompng Thank you for the reminder, I didn't think of the block given to for.

@eregon
Copy link
Member Author

eregon commented Dec 14, 2025

Looking at things like node.compact_child_nodes.each maybe we should have node.each_child_node? It would avoid the Array allocation and since there is no point to yield nil there is no need to name it node.each_compact_child_node.
Definitely something for another PR, but @kddnewton WDYT?

@eregon eregon force-pushed the node_for branch 5 times, most recently from 966dda3 to eafdef7 Compare December 15, 2025 10:33
@eregon
Copy link
Member Author

eregon commented Dec 15, 2025

I found that {Method,UnboundMethod,Proc}#source_location returns columns in bytes and not in characters.
I think that's a bug and filed https://bugs.ruby-lang.org/issues/21783 but nevertheless it's not a problem for this PR.

EDIT: It also means line_and_character_column_to_byte_offset isn't used in this PR anymore but I think it's a good addition anyway, and it will become used again when/if that CRuby bug is fixed.

end_offset = parse_result.source.line_to_byte_offset(end_line) + end_column
start_byte_column = start_offset - parse_result.source.line_start(start_offset)

found = root.tunnel(start_line, start_byte_column).reverse.find do |node|
Copy link
Member Author

Choose a reason for hiding this comment

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

rfind would be nice here but of course it's only RUby 4+.
I guess we could do a polyfill but for this case I doubt it matters, most of the time is not spent here but in creating the Ruby objects for the AST, there are typically only a few nodes returned by tunnel.

@eregon
Copy link
Member Author

eregon commented Dec 15, 2025

Re the test-all failure, I'll fix it, makes it clear test-unit assert_raise is quite problematic: test-unit/test-unit#347

@Earlopain
Copy link
Collaborator

For reference, this won't be available (at least in Ruby 4.0) unfortunately https://bugs.ruby-lang.org/issues/21783#note-8

@kddnewton
Copy link
Collaborator

@eregon we've marked this as blocked until and unless the source location thing gets resolved. Another option is requesting an official node_id API. Is that something you would consider implementing in TruffleRuby?

@Earlopain Earlopain mentioned this pull request Dec 23, 2025
@eregon
Copy link
Member Author

eregon commented Dec 30, 2025

ruby/ruby#15580 is merged so this is unblocked now :)
Though there is no ruby-dev-builder build with it yet, but that should be done soon.

@eregon eregon requested a review from kddnewton December 30, 2025 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants