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

Clarify intentions around method redefinitions #29233

Merged
merged 2 commits into from Sep 1, 2017

Conversation

@matthewd
Copy link
Member

@matthewd matthewd commented May 26, 2017

Don't use remove_method or remove_possible_method just before a new definition: at best the purpose is unclear, and at worst it creates a race condition.

Instead, prefer redefine_method when practical, and silence_redefinition_of_method otherwise.

By eliminating the remove+define pattern, this fixes #26758, fixes #14021.

@@ -1,3 +1,5 @@
require "active_support/core_ext/module/redefine_method"
Copy link
Member Author

@matthewd matthewd May 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compatibility, because that file now contains redefine_method, which was previously in here.

Loading

if method_defined?(method) || private_method_defined?(method)
# This suppresses the "method redefined" warning; the self-alias
# looks odd, but means we don't need to generate a unique name
alias_method method, method
Copy link
Member Author

@matthewd matthewd May 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The validity of this self-alias pattern is potentially an open question: https://bugs.ruby-lang.org/issues/13574

But if we have to switch to a generated-name alternative, that's fine too.

Loading

kaspth
kaspth approved these changes May 27, 2017
Copy link
Member

@kaspth kaspth left a comment

Dig the clarification 👍

Loading

@matthewd matthewd force-pushed the redefine-method branch from 17757c9 to 546b0e0 Sep 1, 2017
matthewd added 2 commits Sep 1, 2017
Don't use remove_method or remove_possible_method just before a new
definition: at best the purpose is unclear, and at worst it creates a
race condition.

Instead, prefer redefine_method when practical, and
silence_redefinition_of_method otherwise.
@matthewd matthewd force-pushed the redefine-method branch from 546b0e0 to 50dbf81 Sep 1, 2017
@matthewd matthewd merged commit d0d7ce0 into rails:master Sep 1, 2017
2 checks passed
Loading
@matthewd matthewd deleted the redefine-method branch Sep 6, 2017
meetme2meat added a commit to meetme2meat/rails that referenced this issue Apr 17, 2018
meetme2meat added a commit to meetme2meat/rails that referenced this issue Apr 17, 2018
meetme2meat added a commit to meetme2meat/rails that referenced this issue Apr 25, 2018
Backport rails#29233 + Silence the warning `method redefined; discarding o…
meetme2meat added a commit to meetme2meat/rails that referenced this issue May 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants