[Issue-1317][Mock solution] Enable validation between methods defined via inheritance and mixins #8766
Conversation
1134c93 to
a2f8d56
Compare
NOTE: This allows checkSubtype() to be called from the perspective of a receiver rather than the perspective of the owner of a submethod (the owner of a submethod and receiver may be different in the case of abstract methods).
1eff8d5 to
8ce1495
Compare
…() and validateOverridingForMethodInClass() NOTE: The purpose of validateOverridingForMethodInClass() is to allow for validation of overriding of a method from the view of the certain class
e5dfe09 to
d23a55f
Compare
NOTE: - This allows the error reports in validateCompatibleOverride() to point to some location other then the method which is passed in - This commit updates the error location to be the relevant class in the case that class does not own the method causing the failure
…om requirement to use 'override' tag NOTE: this is the current behavior, but also it seems like there would be no way for a programmer to respect this requirement in the case an abstract method is implemented by a mixin. Perhaps they could re-define the method, call super and update the sig to have an 'override', but that seems cumbersome.
d23a55f to
838263e
Compare
…e the case where the initial super class method found has the overload name NOTE: Implemented as a result of failures see in rewriter/struct_initialize test (and others).
… was not considered when evaluating type compatibility of 'attached_class' NOTE: Implemented as a result of failures seein in initialize_class_bound test (on ChildGoodThing class complaining about T.attached_class). Likely due to abstract T.attached_class mixed with inheritance.
…method implementation which conflicts with types on initial definition
838263e to
20e6fe2
Compare
| // If no direct match, try to find through inheritance chain | ||
| for (auto &el : params) { | ||
| // Check if el's owner derives from this->definition's owner or vice versa | ||
| if (el.data(gs)->owner.asClassOrModuleRef().data(gs)->derivesFrom(gs, this->definition.data(gs)->owner.asClassOrModuleRef()) || | ||
| this->definition.data(gs)->owner.asClassOrModuleRef().data(gs)->derivesFrom(gs, el.data(gs)->owner.asClassOrModuleRef())) { | ||
| // If they're related, check if they represent the same type parameter | ||
| if (el.data(gs)->name == this->definition.data(gs)->name) { | ||
| return targs[&el - ¶ms.front()]; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
NOTE: Error log which prompted this change. test.log
| } | ||
|
|
||
| ENFORCE(superMethod.exists()); |
There was a problem hiding this comment.
NOTE: Error log which prompted this change. test.log
|
Hey thanks for taking an interest in Sorbet! Note that #1317 is likely the sort of task that will not be able to be completed without substantial involvement from the Sorbet team to fix the issues that arise on Stripe's codebase. You're welcome to tinker with this for your own curiosity, but it's unlikely that this change will land until possibly many months or years in the future when someone from our team can prioritize giving it the attention it will need. |
|
@jez Thanks for chiming in and for sharing the reality that this will be difficult to land in practice. If you have time (and if it matches how you'd usually engage with these things), would you be able to provide any feedback on the approach taken here? My changes seem to pass tests locally and seem to provide correct-looking error outputs for the examples shared in #1317. Of course I'd love to get this in (I have a use case here at Gusto that I would like to have Sorbet report on), but I understand that actually getting this landed would take investment beyond just a review. 🙏 |
In the above code snippet I would like the
Childclass to report that the implementation offooit has received fromMdoes not satisfy the abstract method fromParent(I would also like an error reported ifMwere the abstract class).Changes
validateCompatibleOverride()to accept a "receiverKlass" and "loc_for_error", so it can report on errors that may not just be relevant to the methods owner.validateOverridinginvalidateAbstractMotivation
This is a response to the issue raised here. I would like to follow a Ruby pattern similar to the one outlined in the issue and would like to have sorbet raise relevant errors.
Test plan
The plan is to add new tests! See included automated tests.