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

May break on Ruby versions below 3.2.0 due to non-evaluatable Symbol#inspect #359

Closed
prikha opened this issue Jan 18, 2024 · 14 comments · Fixed by #361
Closed

May break on Ruby versions below 3.2.0 due to non-evaluatable Symbol#inspect #359

prikha opened this issue Jan 18, 2024 · 14 comments · Fixed by #361

Comments

@prikha
Copy link
Contributor

prikha commented Jan 18, 2024

For String and Symbol Unparser uses #inspect for serialization back to Ruby-code. Which makes sense. But apparently #inspect hasn't always worked the same way for Symbols across Ruby versions:

ruby-2.0.0-p0       :"8 >="
...
ruby-2.2.0-preview1 :"8 >="
ruby-2.2.0-preview2 :8 >=
...
ruby-3.2.0-preview1 :8 >=
ruby-3.2.0-preview2 :"8 >="
...
ruby-3.3.0          :"8 >="

This bug has been reported in Ruby itself and fixed in >3.2.0
https://bugs.ruby-lang.org/issues/18905

There is nothing to do about it, but might be of interest for those still running older Rubies.

@prikha prikha changed the title Version prior 0.6.12 have no Ruby version requirement and may struggle from non-evaluatable Symbol#inspect Versions prior 0.6.12 have no Ruby version requirement and may hit a non-evaluatable Symbol#inspect Jan 18, 2024
@mbj
Copy link
Owner

mbj commented Jan 18, 2024

@prikha mhh, are you sure this affects ruby 2.7 also, which is the min ruby version requirement for 0.6.12? I'm compiling 2.7 right now (may not compile as its quite EOL and thus may not work against my linux openssl anymore). Even if its broken on 2.7, I'd just accept that given 2.7 is alreday EOL.

Conceptually I've got not that much intention to keep my OSS working for EOL rubies, if someone needs to have my OSS to work on an EOL ruby I'm happy to do so, for compensation.

@mbj
Copy link
Owner

mbj commented Jan 18, 2024

Context at the time I had 2.7 still in the text matrix, I was round tripping rubyspec (on CI) that includes this kinds of symbols.

@mbj
Copy link
Owner

mbj commented Jan 18, 2024

So 2.7 does not compile on a bison symbol error, and I do not actually want to invest time into an EOL ruby. If someone can proof me 2.7 is indeed broken for the 0.6.12 releases, I'm happy to yank these gems and or put a warning into this repos readme.

@mbj
Copy link
Owner

mbj commented Jan 18, 2024

@prikha or are you saying its borken on 3.x? Can you give me something that produces an issue on bundle exec bin/unparser -e '$broken' ?

@prikha
Copy link
Contributor Author

prikha commented Jan 18, 2024

Sorry Markus, it may sounded a bit misleading. There is no action point, I just wanted to pinpoint that older versions of unparser are affected by the bug mentioned.

####ruby 3.2.0:

./bin/unparser -e ':"8 <="'
Success: (string)

ruby 2.7.8:

./bin/unparser -e ':"8 <="'
(string)
Original-Source:
:"8 <="
Generated-Source:
:8 <=
Original-Node:
(sym :8 <=)
Generated-Node:
#<Parser::SyntaxError: unexpected token tCOLON>
...
Error: (string)

With Ruby '>ruby-3.2.0-preview2` it's solved, and gemspec specifies dependency well.
If anyone would face an issue, they'll probably come by this ticket. Feel free to close it.

@prikha
Copy link
Contributor Author

prikha commented Jan 18, 2024

Again I didn't mean that you have to do any changes or spend some extra time, you're doing great running this project already, cheers! 🍺

@prikha
Copy link
Contributor Author

prikha commented Jan 18, 2024

The only thing I would to is to update the required ruby version to >3.2 in the gemspec, so that the fix is present in the Ruby itself.

@mbj
Copy link
Owner

mbj commented Jan 18, 2024

Nah, I need >= 3.0 if its borken on any ruby >=3.0 I'll work around it for these versions.

@prikha prikha changed the title Versions prior 0.6.12 have no Ruby version requirement and may hit a non-evaluatable Symbol#inspect May break on Ruby versions below 3.2.0 due to non-evaluatable Symbol#inspect Jan 18, 2024
@prikha
Copy link
Contributor Author

prikha commented Jan 18, 2024

@mbj may I ask for a tip - there is a fix for 3.0, 3.1 ☝🏻 but Mutant is catching me by the hand with ruby version check and I'm a bit puzzled how should we overcome that. E.g. if/else branches are equivalent on 3.3, 3.2 so mutating if condition won't fail any test in CI as it's the sole purpose of the fix to produce same results on different versions.

@mbj
Copy link
Owner

mbj commented Jan 18, 2024

@prikha Yeah so mutant needs ruby version based disable. What I'd do right now is to:

  • mutation cover it on 3.0, and 3.1
  • once done: add a # mutant:disable to the method, with a comment about its version specific.

Ideally I get mutant to support version specific disables in the future.

@mbj
Copy link
Owner

mbj commented Jan 18, 2024

@prikha your PR drops support for non EOL rubies, not acceptable. This needs a ruby version specific branch to add the missing quotes, if any.

@prikha
Copy link
Contributor Author

prikha commented Jan 18, 2024

@mbj ok closed the first one, and updated the second one. Mind taking a look 🙏🏻

@mbj
Copy link
Owner

mbj commented Jan 22, 2024

@prikha Sorry travelling right now. I cannot offer my time deterministically ATM.

@mbj mbj closed this as completed in #361 Feb 1, 2024
@mbj
Copy link
Owner

mbj commented Feb 1, 2024

@prikha Released as 0.6.13, ty!

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