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

Use __send__ instead of send #140

Merged
merged 1 commit into from Oct 29, 2020
Merged

Use __send__ instead of send #140

merged 1 commit into from Oct 29, 2020

Conversation

mfilej
Copy link
Contributor

@mfilej mfilej commented Oct 29, 2020

We had an issue in our codebase where we would see a mysterious ArgumentError - wrong number of arguments (given 2, expected 3) that was being traced to this line:

(gem) zeitwerk-2.3.0/lib/zeitwerk/loader.rb:796:in `unload_autoload'

We finally realized that this is down to a class in our codebase that defines its own self.send method (that does something unrelated, hence the mismatch in arity).

Now I'm completely aware that this is bad practice and we're going to fix the offending class, but it's also true that the resulting error message is very confusing and it's not immediately clear how to fix it.

If zeitwerk was to use __send__ instead of send it would avoid this issue. I'm not sure if there are any downsides to this, or if this should be merged at all -- consider this pull request an invitation for a discussion.

This should avoid issues where a class defines its own #send method.
@casperisfine
Copy link
Contributor

Another possibility would be the same trick we use for Module#name: https://github.com/fxn/zeitwerk/blob/60021d90af973d98ee19e9634a552b25c39320cc/lib/zeitwerk/real_mod_name.rb

But yeah __send__ is very unlikely to be redefined.

@fxn
Copy link
Owner

fxn commented Oct 29, 2020

❤️

@fxn fxn merged commit 26f232e into fxn:master Oct 29, 2020
@fxn
Copy link
Owner

fxn commented Oct 29, 2020

Version 2.4.1 is out with this fix, thanks!

@mfilej mfilej deleted the send branch October 29, 2020 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants