Be more defensive in the parser translator lexer by Earlopain · Pull Request #3551 · ruby/prism

Earlopain · 2025-04-17T08:59:33Z

Generally I have been good about safely accessing the tokens but failed to properly guard against no tokens in places
where it could theoretically happen through invalid syntax.

I added a test case for one occurance, other changes are theoretical only.

Also, TIL that this works:

a = [[1,2], [3,4]]
i = 0

while (foo, _ = a[i])
  puts foo
  i += 1
end
# => 1
# => 3

I make use of that construct to safely access the data without much changes. Makes some code easier actually.

Earlopain · 2025-06-30T12:07:14Z

Hi @kddnewton, sorry for the bother but can you check this out?

kddnewton · 2025-06-30T12:30:24Z


              if percent_array?(quote_stack.pop)
-                prev_token = lexed[index - 2][0] if index - 2 >= 0
+                prev_token, _ = lexed[index - 2]


This is going to wrap around now, but I don't think it should

Oops, indeed. I should not have removed that

kddnewton · 2025-06-30T12:30:48Z


-                prev_token = lexed[index - 2][0] if index - 2 >= 0
-                next_token = lexed[index][0]
+                prev_token, _ = lexed[index - 2]


Same here, we don't want it to wrap

kddnewton · 2025-07-08T14:10:25Z

                  current_length += token.value.bytesize
                  # Heredoc interpolation can have multiple STRING_CONTENT nodes on the same line.
-                  is_first_token_on_line = lexed[index - 1] && token.location.start_line != lexed[index - 2][0].location&.start_line
+                  is_first_token_on_line = lexed[index - 1] && token.location.start_line != lexed[index - 2]&.first.location&.start_line


Is it possible for lexed[index - 2] to be nil here? If it is, then we need &.location as well. If it's not, then we shouldn't use &.first

I meant this to be "safe" safe navigation. I don't believe this matters for valid syntax, but with invalid one I can't say so wanted to be on the safe side.

Earlopain · 2025-08-11T08:20:40Z

Any chance to get this merged? This is also a problem for rubocop extensions that try to extract ruby code from templating like erb or slim (r7kamura/rubocop-slim#31)

Morriar · 2025-08-11T18:18:13Z


-                while !((next_token = lexed[index][0]) && next_token.type == :EMBDOC_END) && (index < length - 1)


I think you can also do the same line 284?

Yeah, that works (if you meant what I changed that is)

Morriar · 2025-08-11T18:21:17Z

                  current_length += token.value.bytesize
                  # Heredoc interpolation can have multiple STRING_CONTENT nodes on the same line.
-                  is_first_token_on_line = lexed[index - 1] && token.location.start_line != lexed[index - 2][0].location&.start_line
+                  is_first_token_on_line = lexed[index - 1] && token.location.start_line != lexed[index - 2]&.first&.location&.start_line


Can we avoid the mix of index accesses and first with something like this?

Suggested change

is_first_token_on_line = lexed[index - 1] && token.location.start_line != lexed[index - 2]&.first&.location&.start_line

prev_prev_token, _ = lexed[index - 2] if index - 2 >= 0

is_first_token_on_line = lexed[index - 1] && token.location.start_line != prev_prev_token&.location&.start_line

Yeah, sure. It's a bit better. It's actually just prev_token, even if it is[index -2]. Right at the top of the loop index already gets incremented so you need to unintuitively look back a bit further.

I should move most of the state from the huge to_a into the class itself to make writing helper methods more convenient sometime.

paracycle · 2025-08-14T14:34:43Z

          result = Float::MAX

-          while (lexed[next_token_index] && next_token = lexed[next_token_index][0])
+          while (next_token, _ = lexed[next_token_index])


I don't think the behaviour of these two constructs (old and new) will be the same in every instance. For example:

irb> foo = [nil, 1] #=> [nil, 1] irb> while (i, _ = foo); puts "42"; break; end 42 #=> nil irb> while (i = foo&.first); puts "42"; break; end #=> nil

The same would be true for any assignments in if conditions as well.

I would very much like to see &.first instead of destructuring used to safely get these values, since its behaviour would be the same as [0] (but safe against nil values) and I also find that it is more readable.

Good catch, I was expecting the destructure to be falsy

The token being nil would be a bug in prism I believe (and probably blow up in a different place later). I can see now how this isn't quite the same but practially the difference should be zero.

Anyways, I changed it.

Thanks for changing it, but I don't get why you think it would be a bug. The loop is explicitly conditional on the next_token being truthy, and if it isn't it should break.

I was just saying that this difference shouldn't matter for the data that prism provides. It's always [token, some_internal_state], if there's no token (nil, then that should not have been part of lexed in the first place.

Generally I have been good about safely accessing the tokens but failed to properly guard against no tokens in places where it could theoretically happen through invalid syntax. I added a test case for one occurance, other changes are theoretical only.

Earlopain · 2025-08-14T16:06:21Z

Thanks everyone

kddnewton · 2025-08-14T17:05:40Z

Thanks for your patience @Earlopain — I'll be much faster when I'm back from paternity leave.

Earlopain · 2025-08-14T18:24:37Z

@kddnewton don't worry about it at all. Your labour on the previous PRs more than makes up for it

This was referenced Apr 17, 2025

undefined method '[]' for nil with (incomplete) case statement rubocop/rubocop#14100

Closed

Parser fails to tokenize ERB with two subsequent returning expressions #3550

Closed

kddnewton requested changes Jun 30, 2025

View reviewed changes

Earlopain force-pushed the parser-translator-lexer-invalid-syntax branch from 329bd6e to b1a9b32 Compare June 30, 2025 12:46

kddnewton requested changes Jul 8, 2025

View reviewed changes

Earlopain force-pushed the parser-translator-lexer-invalid-syntax branch from b1a9b32 to 63e30a1 Compare July 8, 2025 18:15

rafaelfranca approved these changes Jul 16, 2025

View reviewed changes

rafaelfranca added the Need another review Need an extra review label Jul 16, 2025

koic mentioned this pull request Aug 11, 2025

Prism::Translation::Parser::Lexer#to_a -- undefined method '[]' for nil rubocop/rubocop#14434

Closed

Morriar reviewed Aug 11, 2025

View reviewed changes

Earlopain force-pushed the parser-translator-lexer-invalid-syntax branch 2 times, most recently from 247e841 to a932a4f Compare August 11, 2025 19:25

Morriar approved these changes Aug 14, 2025

View reviewed changes

paracycle reviewed Aug 14, 2025

View reviewed changes

Earlopain force-pushed the parser-translator-lexer-invalid-syntax branch from a932a4f to 4a3866a Compare August 14, 2025 15:06

kddnewton approved these changes Aug 14, 2025

View reviewed changes

kddnewton merged commit 0984117 into ruby:main Aug 14, 2025
60 checks passed

Earlopain deleted the parser-translator-lexer-invalid-syntax branch October 3, 2025 10:37


		while !((next_token = lexed[index][0]) && next_token.type == :EMBDOC_END) && (index < length - 1)

	is_first_token_on_line = lexed[index - 1] && token.location.start_line != lexed[index - 2]&.first&.location&.start_line
	prev_prev_token, _ = lexed[index - 2] if index - 2 >= 0
	is_first_token_on_line = lexed[index - 1] && token.location.start_line != prev_prev_token&.location&.start_line

Uh oh!

Conversation

Earlopain commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Earlopain commented Jun 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Earlopain commented Aug 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Earlopain Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Earlopain commented Aug 14, 2025

Uh oh!

kddnewton commented Aug 14, 2025

Uh oh!

Earlopain commented Aug 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Earlopain commented Apr 17, 2025 •

edited

Loading

Earlopain Aug 11, 2025 •

edited

Loading