Test multiple prism versions in CI + Fix Prism head #243
Conversation
It used to not emit this token type, but now it does. So when a newer version of prism is present, we can fall back to the same code that ripper uses. Ref: * ruby/ruby#15914 * ruby/prism#3859
schneems
changed the title
The reason this logic for different methods branches in the class instead of internally was to be eagerly aggressive about runtime performance. This code is currently only used once for the document where it's invoked ~N times (where N is number of lines):
```ruby
module SyntaxSuggest
class CleanDocument
# ...
def join_trailing_slash!
trailing_groups = @document.select(&:trailing_slash?).map do |code_line|
take_while_including(code_line.index..) { |x| x.trailing_slash? }
end
join_groups(trailing_groups)
self
end
```
Since this is not currently a hot-spot I think merging the branches and using a case statement is a reasonable tradeoff and avoids the need to do specific version testing.
An alternative idea was presented in #241 of behavior-based testing for branch logic (which I would prefer), however, calling the code triggered requiring a `DelegateClass` when the `syntax_suggest/api` is being required.
There was a problem hiding this comment.
Testing against multiple prism versions is a good idea 👍. Your change looks fine to me.
Would it be bad if users use newer prism but don't yet have this fix? There are test failures but I can't really tell what would happen in practice.
I'm thinking if we first need to distribute syntax_suggest to users via a new ruby version. Users don't usually have syntax_suggest in their gemfile but for prism this is more common so many will get only the prism update.
It probably only impacts code that uses line continuations in some way. Do you think that would be fine?
|
Regarding how this will affect things. The problem this code is used for is this: it "code can be split" \
"across multiple lines" doIf you don't logically "join" those two lines, then removing one or the other could either accidentally "fix" a syntax error (incorrectly) or induce a new syntax error that didn't exist before. So the output might point to these lines, even if the REAL syntax error is unrelated. Then the output could point to the wrong location and hide the real issue. Which, is...not great. Here's a copy of the failing tests preserved for future posterity https://gist.github.com/schneems/834c75345b2085bda54dab69d913bcc0. And the trailing lines don't need to be in the code you're writing for the problem to occur, just exist in the file that you're editing. Anyone getting a newer prism and an older syntax_suggest would hit that problem. It's not idiomatic to have trailing lines split like that, but it does happen. I'll merge this and cut a release of After this issue, if you've got a spare moment, I would appreciate your thoughts and feedback on #244. |
|
This is out now with 2.0.3: |
|
Thank you. I created https://bugs.ruby-lang.org/issues/21847 to backport this |
on_spwhen using prism #241 is adding branching logic for different prism versions based on changes to HEAD. I want to guard against regressions. This change adds the ability to test against multiple versions, including HEAD (which is currently failing on main, but passing on their branch).on_spinsyntax_suggestruby#15914on_spwhen using prism #241 to get tests to pass on head.