Fix method name validation edge-case #1045
Merged
Conversation
This file contains 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
Morriar
approved these changes
Jul 7, 2022
rafaelfranca
approved these changes
Jul 7, 2022
It turns out that `:4_to_5=` is not a valid symbol but `"4_to_5=".to_sym` does not quote it and represents it as `:4_to_5=`, which throws off our whole method name validation logic. The fix is to check validity of the name after stripping trailing `=` as well. That leaves `==` as an edge-case, that we handle separately.
paracycle
force-pushed
the
uk-fix-method-name-validation
branch
from
af49ba7 to
75f6857
Compare
paracycle
added a commit
that referenced
this pull request
Jul 18, 2022
Our current way of doing method name and parameter name checks are a bit hacky and [that's been showing through](#1045). A better way to do this is to use the compiler to do the name checks, similar to [how Ruby does it in its codebase](https://github.com/ruby/ruby/blob/241dced625f9ba8a4071954579778a0940e75179/ext/rubyvm/lib/forwardable/impl.rb#L3-L9). This method relies on compiling a method definition with the given name and then checking if the compiled code actually defines a method with that name.
paracycle
added a commit
that referenced
this pull request
Jul 18, 2022
Our current way of doing method name and parameter name checks are a bit hacky and [that's been showing through](#1045). A better way to do this is to use the compiler to do the name checks, similar to [how Ruby does it in its codebase](https://github.com/ruby/ruby/blob/241dced625f9ba8a4071954579778a0940e75179/ext/rubyvm/lib/forwardable/impl.rb#L3-L9). This method relies on compiling a method definition with the given name and then checking if the compiled code actually defines a method with that name.
paracycle
added a commit
that referenced
this pull request
Jul 18, 2022
Our current way of doing method name and parameter name checks are a bit hacky and [that's been showing through](#1045). A better way to do this is to use the compiler to do the name checks, similar to [how Ruby does it in its codebase](https://github.com/ruby/ruby/blob/241dced625f9ba8a4071954579778a0940e75179/ext/rubyvm/lib/forwardable/impl.rb#L3-L9). This method relies on compiling a method definition with the given name and then checking if the compiled code actually defines a method with that name.
egiurleo
pushed a commit
that referenced
this pull request
Aug 19, 2022
Fix method name validation edge-case
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation
Fixes #1042
It turns out that
:4_to_5=is not a valid symbol but"4_to_5=".to_symdoes not quote it and represents it as:4_to_5=instead of:"4_to_5", which throws off our whole method name validation logic.The behaviour of Ruby was correct on this symbol until
ruby-2.2.0-preview2, after which it was broken:I think it is related to this change: ruby/ruby@986a893
Implementation
The fix is to check validity of the name after stripping trailing
=as well. That leaves==as an edge-case, that we handle separately.Tests
Improved existing tests and added explicit AR column tests for invalid method named column names