Rework method/parameter name validation to use the compiler #1066
Conversation
paracycle
force-pushed
the
uk-rework-method-name-validation
branch
2 times, most recently
from
40fd719 to
653e838
Compare
paracycle
changed the title
| end | ||
|
|
||
| sig { params(name: String).returns(T::Boolean) } | ||
| def valid_parameter_name?(name) | ||
| /^([[:lower:]]|_|[^[[:ascii:]]])([[:alnum:]]|_|[^[[:ascii:]]])*$/.match?(name) | ||
| sentinel_method_name = :sentinel_method_name |
There was a problem hiding this comment.
How cleaner and faster is this compared to a regular expression?
There was a problem hiding this comment.
I thought the compiler version might be drastically slower, so I ran some large gem RBI generation (rubocop) on the codebase, before and after this change and got super similar results:
$ hyperfine --prepare "git reset --hard 822611bf" "bundle exec tapioca gem rubocop" --prepare "git reset --hard 1984b06b" "bundle exec tapioca gem rubocop # symbol hack"
Benchmark 1: bundle exec tapioca gem rubocop
Time (mean ± σ): 26.091 s ± 0.474 s [User: 23.142 s, System: 2.413 s]
Range (min … max): 25.420 s … 27.032 s 10 runs
Benchmark 2: bundle exec tapioca gem rubocop # symbol hack
Time (mean ± σ): 26.132 s ± 0.684 s [User: 23.127 s, System: 2.402 s]
Range (min … max): 25.498 s … 27.923 s 10 runs
Summary
'bundle exec tapioca gem rubocop' ran
1.00 ± 0.03 times faster than 'bundle exec tapioca gem rubocop # symbol hack'
(the one with the comment # symbol hack being the version before these changes)
In general, I think this is "cleaner" since we rely on the Ruby to tell us what is considered a valid method name or parameter name, instead of relying on regular expressions that may or may not cover edge cases.
There was a problem hiding this comment.
Interesting!
It's definitely better than the symbol hack and, as you said, at least we don't have to bother with it anymore since now it's handled by the Ruby VM.
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
force-pushed
the
uk-rework-method-name-validation
branch
from
653e838 to
28de350
Compare
Motivation
Our current way of doing method name and parameter name checks are a bit hacky and that's been showing through.
Implementation
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. (Thanks to @tenderlove for showing me this method name validation check)
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.
Tests
Only updated the error message from a test to make it explicit which case is failing.