Allow abstract methods to be implemented via method_missing - #36
Open
kshtzkr wants to merge 1 commit into
Open
Conversation
Previously, AbstractInstanceMethodReceiver#method_missing raised AbstractMethodNotImplementedError before calling super, so a method_missing defined further up the ancestor chain never got the chance to provide a dynamic implementation of an abstract method. Now super is called first, and the resulting NoMethodError is only translated into AbstractMethodNotImplementedError when it is about the same method that was called. NoMethodErrors raised for other methods (e.g. from inside a broken dynamic implementation) propagate unchanged, and the translated error hides its NoMethodError cause, which is unactionable noise. Closes Shopify#11
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
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.
Closes #11
Problem
AbstractInstanceMethodReceiver#method_missingraisedAbstractMethodNotImplementedErrorbefore callingsuper, so amethod_missingdefined further up the ancestor chain (e.g. on a superclass) never got the chance to provide a dynamic implementation of an abstract method:Approach
As suggested in #11 (comment): call
superfirst, and only translate the resultingNoMethodErrorinto anAbstractMethodNotImplementedErrorif nothing handled the call.Two refinements over the sketch in the issue:
NoMethodErroris only translated whene.name == method_name. ANoMethodErrorfor a different method — e.g. raised from inside a broken dynamic implementation of the abstract method — is a real error and propagates unchanged, instead of being misreported as "abstract method never implemented".cause: nil(as in the issue's sketch), since the underlying "undefined method" error is unactionable noise beneath the more precise error.I went with rescuing
NoMethodErrorrather than checkingrespond_to_missing?first, per the reasoning in the issue: lots of code overridesmethod_missingwithout overridingrespond_to_missing?.Tests
Three new specs in
interface_spec.rb:method_missingcan be calledAbstractMethodNotImplementedError, with noNoMethodErrorcause attachedNoMethodErrorraised inside a dynamic implementation propagates as-isAll local checks pass:
rake typecheck(incl. Prism),rake test(92 runs, 0 failures),rake rubocop,bin/tapioca gem --verify,bin/tapioca check-shims.Note: #11 had an assignment request from April that was never assigned and has no linked PR — happy to step aside if that work is still in flight.