[mypyc] Fix order of dispatch type checking in singledispatch functions #10844
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
When creating a dispatch function for a singledispatch function, avoid checking any classes before we've checked all of their subclasses. That makes sure we use the implementation with the dispatch type that appears first in the actual argument's type's MRO, which is what the standard library implementation of singledispatch does (it goes up the argument's MRO, looking for classes that have registered implementations associated with them).
|
The CI failures looks a bit weird, so I restarted the builds just in case. |
ilevkivskyi
reviewed
Jul 22, 2021
mypyc/irbuild/function.py
Outdated
| # gets checked first | ||
| if any(is_subclass(typ, prev_type) for prev_type in dispatch_types): | ||
| dispatch_types.insert(0, typ) | ||
| funcs.insert(0, impl) |
There was a problem hiding this comment.
I think this logic is flawed. Imagine a situation like this:
class A: ...
class B(A): ...
class C(B): ...Then if these classes appear in impls in the order A, C, B, then after this "sorting", they will be in the order B, C, A, which is wrong.
Also when fixing this I think it could make sense to make it stable (i.e. order of unrelated items should be preserved).
There was a problem hiding this comment.
I think this should be fixed now. Could you take a look and let me know what you think?
Add a test to reproduce the bug in the class sorting implementation mentioned in python#10844 (comment).
Improve the sorting of classes to: - always put subclasses before any of their superclasses, even when there are multiple related classes (see testMultipleRelatedClassesBeingRegistered for an example of this) - preserve the order of implementations with unrelated dispatch types
The one use of Iterable was removed in the previous commit, so this is unnecessary.
msullivan
reviewed
Jul 22, 2021
| @@ -857,3 +858,44 @@ def gen_dispatch_func_ir( | |||
| func_decl = FuncDecl(dispatch_name, None, builder.module_name, sig) | |||
| dispatch_func_ir = FuncIR(func_decl, args, blocks) | |||
| return dispatch_func_ir | |||
|
|
|||
|
|
|||
| def sort_with_subclasses_first( | |||
There was a problem hiding this comment.
This seems plausible, but I think that it might be more straightforward to implement this as a topological sort, instead. I'll follow up via PM.
Change the sorting of classes that we use when generating dispatch functions to use topological sort on a graph made up of the classes with edges pointing from subclasses to the classes on their MRO. This also modifies the signature of topsort to take any type for the vertices of the graph.
msullivan
approved these changes
Jul 23, 2021
Use a dictionary comprehension to make the graph creation nicer.
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.
This PR changes the order that the dispatch function checks dispatch types of registered implementations to always check any of the class's subclasses (if any have registered implementations) before checking the class itself.
That matches the semantics of the standard library implementation of singledispatch more closely, which goes up the MRO of the argument's type and uses the registered implementation that corresponds with the first type that it finds that is also a dispatch type.
Test Plan
I added a test to run-singledispatch.test that reproduces this bug.