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

Use char_is_identifier_start() to check for valid method name and fix serialization #2264

Merged
merged 4 commits into from
Jan 24, 2024

Conversation

maxprokopiev
Copy link
Contributor

I've noticed an interesting case where snippets like foo.🌊 = 1 are not accepted and failing with:

[#<Prism::ParseError @message="unexpected write target" @location=#<Prism::Location @start_offset=10 @length=1 start_line=1>>]

The reason for that is because the current check is only for _ and any alphanumeric char, so it makes sense to change that to use char_is_identifier_start() which will check for any valid identifier (while also making the condition cleaner ✨).

After fixing that I've noticed that test cases started to fail with:

...
prism/test/prism/test_helper.rb:60:in `assert_equal_nodes'
prism/test/prism/parse_test.rb:230:in `block (3 levels) in <class:ParseTest>'
prism/test/prism/parse_test.rb:225:in `each'
prism/test/prism/parse_test.rb:225:in `block (2 levels) in <class:ParseTest>'
<:🌊=>(UTF-8) expected but was
<:"\xF0\x9F\x8C\x8A=">(ASCII-8BIT)

diff:
? :🌊                =
?  "\xF0\x9F\x8C\x8A "
?  ? +

? Encoding: UTF  -8
?           ASCII  BIT
?           ???  +++

That seems to be an issue with not forcing encoding during the deserialization, which is also fixed in 9323243

alnum_char() only checks for alphanumeric characters while
ignoring other valid cases (like emoji codepoints for example)
otherwise we get failing tests if we have non-ascii
characters in fixtures/**/*.txt
Copy link
Collaborator

@kddnewton kddnewton left a comment

Choose a reason for hiding this comment

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

Yeah this looks good, thanks!

@kddnewton
Copy link
Collaborator

It looks like there's a failure on truffleruby because of Symbol#inspect, where on TruffleRuby they're making it :"🌊" whereas on CRuby it's :🌊.

To fix this, can you move this fixture into its own file and then skip it on TruffleRuby in parse_test.rb? We have something like this already there because of this exact problem (which I forgot about...).

@kddnewton
Copy link
Collaborator

And then we just need to regenerate the snapshots by rerunning the tests

@maxprokopiev
Copy link
Contributor Author

was running the tests with truffleruby locally πŸ˜… pushed the snapshots in 459a9f5

@kddnewton kddnewton merged commit a8c05e8 into ruby:main Jan 24, 2024
47 checks passed
@eregon
Copy link
Member

eregon commented Jan 24, 2024

I filed oracle/truffleruby#3407 to fix this in TruffleRuby

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants