Prevent overwriting Hash#except method present in Ruby 3+
#557
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
Merged
Ruby 3+ introduced `Hash#except` method natively [1], so we can skip I18n's custom implementation here if the method is already defined. This follows the same Rails convention [2]. Note: This seems to be causing a `stack level too deep (SystemStackError)` with a specific set of Gemfiles under Ruby 3, reported to `SimpleForm` originally [3]. I haven't been able to fully narrow it down after some initial investigation, nor reproduce it without the given set of gems in the Gemfile reported, but it seems there's some interactions between those gems, and combined with the fact that Ancestry loads I18n early on to setup the load path [4], is possibly causing calls to `Hash#except` to hang or raise with the `stack level too deep` error, and after skipping the method re-declaration here it's not reproducible anymore. [1] https://bugs.ruby-lang.org/issues/15822 [2] https://github.com/rails/rails/blob/5f3ff60084ab5d5921ca3499814e4697f8350ee7/activesupport/lib/active_support/core_ext/hash/except.rb#L12-L14 [3] heartcombo/simple_form#1724 steps to reproduce issue: heartcombo/simple_form#1724 (comment) [4] https://github.com/stefankroes/ancestry/blob/71fe7042791f5943b5370240e4c6068dce73233d/lib/ancestry.rb#L9-L10
carlosantoniodasilva
force-pushed
the
ca-except-ruby3
branch
from
a80b157
to
cead3e5
Compare
|
@carlosantoniodasilva To be clear: this PR should fix those stack level too deep errors, right? |
|
@radar yes, it should, hopefully. I tried a few things to repro on I18n alone but couldn't find a way. In the test app based one the Gemfile above this fix worked to prevent the issue, and the person there who helped me repro commented back that their app worked against this branch on Ruby 3. |
|
OK, let's see how this one goes. Thank you @carlosantoniodasilva! |
|
🤞🙌 thank you! |
|
This has now been released in 1.8.9. |
This was referenced Mar 11, 2021
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.
Note: this was updated to be based on #558 which fixes the build for the Rails main matrix.
Ruby 3+ introduced
Hash#exceptmethod natively [1], so we can skipI18n's custom implementation here if the method is already defined.
This follows the same Rails convention [2].
Note: This seems to be causing a
stack level too deep (SystemStackError)with a specific set of Gemfiles under Ruby 3, reported to
SimpleFormoriginally [3]. I haven't been able to fully narrow it down after some
initial investigation, nor reproduce it without the given set of gems in
the Gemfile reported, but it seems there's some interactions between
those gems, and combined with the fact that Ancestry loads I18n early on
to setup the load path [4], is possibly causing calls to
Hash#exceptto hang or raise with the
stack level too deeperror, and afterskipping the method re-declaration here it's not reproducible anymore.
[1] https://bugs.ruby-lang.org/issues/15822
[2] https://github.com/rails/rails/blob/5f3ff60084ab5d5921ca3499814e4697f8350ee7/activesupport/lib/active_support/core_ext/hash/except.rb#L12-L14
[3] heartcombo/simple_form#1724
steps to reproduce issue: heartcombo/simple_form#1724 (comment)
[4] https://github.com/stefankroes/ancestry/blob/71fe7042791f5943b5370240e4c6068dce73233d/lib/ancestry.rb#L9-L10
Reproducing
This is from the original SimpleForm issue.
Minimal Gemfile used to reproduce:
Demo app: https://github.com/khustochka/sf_ancestry_test (I haven't tested this one directly myself, I had an app set up with that Gemfile above)
Simply going to
rails consoleand typingHash.new.method(:except)would hang, and runningrails serverwould raise the stack error, without the change here. More info about my testing.