Restore the original order of const_added and inherited callbacks #13085
+26
−98
Conversation
This file contains hidden or 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
byroot
reviewed
Apr 9, 2025
| [:const_added, :Subclass, :location], | ||
| ] | ||
| end | ||
| end |
There was a problem hiding this comment.
I think we should keep these specs, just update them to match the old behavior.
There was a problem hiding this comment.
I have that one in my original/followup patch.
There was a problem hiding this comment.
Let me add that my test verifies exactly callback execution order, which is what the test is about. I'll add @eregon suggestion too.
Then, I could add tests that say: In both callbacks, inheritance has happened, and constant assignment has happened (there is a location, constants has the constant, the class has a permanent name, etc.).
Originally, if a class was defined with the class keyword, the cref had a
const_added callback, and the superclass an inherited callback, const_added was
called first, and inherited second.
This was discussed in
https://bugs.ruby-lang.org/issues/21143
and an attempt at changing this order was made.
While both constant assignment and inheritance have happened before these
callbacks are invoked, it was deemed nice to have the same order as in
C = Class.new
This was mostly for alignment: In that last use case things happen at different
times and therefore the order of execution is kind of obvious, whereas when the
class keyword is involved, the order is opaque to the user and it is up to the
interpreter.
However, soon in
https://bugs.ruby-lang.org/issues/21193
Matz decided to play safe and keep the existing order.
This reverts commits:
de097fb
de48e47
05a9535 to
63f90f4
Compare
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.
Originally, if a class was defined with the
classkeyword, the cref had aconst_addedcallback, and the superclass aninheritedcallback,const_addedwas called first, and inherited second.This was discussed in #21143 and an attempt at changing this order was made.
While both constant assignment and inheritance have happened before these callbacks are invoked, it was deemed nice to have the same order as in
This was mostly for alignment: In that last use case things happen at different times and therefore the order of execution is kind of obvious, whereas when the
classkeyword is involved, the order is opaque to the user and it is up to the interpreter.However, soon in #21193, Matz decided to play safe and keep the existing order.
This reverts commits de097fb and de48e47.