-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[mypyc] Improve support for compiling singledispatch #10795
[mypyc] Improve support for compiling singledispatch #10795
Conversation
Going through the registered implementations in the opposite order that they were defined means that implementations that are defined later in a file are used instead of the ones earlier in the file when they overlap.
Registered implementations are always decorated because they always have a register decorator, and we can't make native calls to decorated functions because otherwise we wouldn't be able to use the function emitted from the decorator (which would be a Python callable object). Previously, we removed the register decorator on those implementations and treated them as non-decorated functions, but that strategy stops working as soon as you add another decorator, so this just treats those implementations as regular decorated functions and uses a non-native call for those functions.
Instead of adding the logic to dispatch to the correct function to the main singledispatch function itself, generate 2 separate functions: one that compiles the logic of the undecorated function normally, and a separate dispatch function that checks the first argument's type and dispatches either to one of the registered implementations or the fallback. That makes it easier to generate code when the main singledispatch function is a generator or some other kind of special function, as the code generation can generate the main singledispatch function the same way it would otherwise. Because we're no longer changing how that main singledispatch function is generated based on whether it's a singledispatch function, we don't need to store whether the current function is a singledispatch function in FuncInfo.
This makes several changes to the singledispatch run tests: - in testRegisterDoesntChangeFunction, pass the type as an argument to register to avoid a mypy error - remove a test where we pass in an invalid type to a singledispatch function, because mypy correctly treats that as an error - in testKeywordArguments, change the keyword argument type and return value to make it easier to tell what's failing - add 2 new tests related to registering functions while applying other decorators to the registered functions - change several previously xfailed tests to non-xfailed tests because they are now able to pass due to previous changes (like generating a separate dispatch function and using non-native calls)
When trying to use the Mapping ABC as the dispatch type for a registered implementation, we previously tried to load the type for the isinstance check as `typing.Mapping` even if it was imported from `collections.abc`, causing a compilation error due to the fact that we hadn't defined CPyModule_typing. To fix that, this loads the type from the globals dict instead for most types, and using the types in builtin_names for builtin types, which won't be present in the globals dict.
Correctly supporting decorators being applied to functions after they've been registered complicates the implementation because we wouldn't be able to generate a call to the function that's been registered when dispatching. Instead, we would have to: - run all the decorators below the register decorator (we already do this) - register the function at runtime by storing what the function looks like after those decorators have been run - run the rest of the decorators Doing that makes the implementation more complicated and having decorators run after registering probably isn't common anyways, so we just show an error.
Iterating over the decorators in reverse causes the numbers returned by enumerate to be incorrect indices, which causes us to remove the wrong decorators when using the indices in removed to delete elements from the decorators_to_store list.
Move the functions that had decorators after registers to commandline.test so that we can test that compilation errors are correctly emitted. Change the remaining decorator-related tests in run-singledispatch.test to make sure any decorators are applied to functions before the functions are registered.
Correct several bugs in tests that allows them to start passing. Also changes several xfail tests to regular tests because they are now able to pass.
Add a newline to the end of commandline.test, and remove some trailing whitespace in run-singledispatch.test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Look into my one question and we can merge
Move the creation of the FuncIR corresponding to the dispatch function to a separate function. This makes gen_func_item cleaner because more of the logic is in a separate function.
[mypyc] Improve support for compiling singledispatch (python#10795)
It looks like this may have regressed performance a bit: https://github.com/mypyc/mypyc-benchmark-results/blob/master/reports/benchmarks/sum_tree_singledispatch.md The trend of performance is a bit worrying -- from 1.14x to 1.00x. It would be good to investigate what is going on. |
This makes several improvements to the support for compiling singledispatch that was introduced in #10753 by:
Test Plan
I extended the
testErrorOutput
test in commandline.test, added a few tests to run-singledispatch.test, and changed some of the existing tests in run-singledispatch.test from xfails to regular tests.