[ty] Discard the types of parameter defaults during cycle normalization#22845
Closed
AlexWaygood wants to merge 1 commit intomainfrom
Closed
[ty] Discard the types of parameter defaults during cycle normalization#22845AlexWaygood wants to merge 1 commit intomainfrom
AlexWaygood wants to merge 1 commit intomainfrom
Conversation
Typing conformance resultsNo changes detected ✅ |
ede691c to
72b678e
Compare
|
Contributor
|
With #23014 we no longer enter into spurious cycles when defining a decorated function with parameters with default values, which fixes the motivating case here -- the results on that snippet are now the same, with or without the patch to I'd suggest rebasing the next PR in this stack on top of #23014 and see if any behavior changes are still observed. If not, I'd suggest we don't make this change for now. |
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.
Summary
This PR tweaks ty's cycle recovery normalization for
Callabletypes (and other types that have load-bearingSignatures) so that the exact type of a parameter default is discarded: aCallablewith type(x: bool = True) -> intbecomes aCallablewith type(x: bool = ...) -> int.Background
The motivation for this PR stems from the observation that applying this diff to the
mainbranch:leads to a behaviour change on this snippet:
On
main, we emit 4 diagnostics on that snippet; but with the above patch applied, we emit 0. That's odd, because the patch should be a no-op in terms of behaviour!Adding some debug prints appeared to indicate that we appear to enter a cycle when inferring a type for the
aloadfunction, and different iterations of the cycle yield different types: in one iteration we infer this type:and in another iteration we infer this type:
now: these are equivalent types! Normally, therefore, our
UnionBuilderwould never allow them to coexist in aUnionTypetogether. Unfortunately, however, the union simplification we are able to do in cycle recovery is much more limited than the union simplification we are able to do in other scenarios: we cannot do anyis_redundant_withcalls during cycle recovery, because that could lead to further nested cycles! As such, we are unable to detect the mutual subtype relationship between the two types (because they are not identical types), and so the solved type ofaloadafter cycle recovery is the union of the two:On
main, it appears that aUnionType::filter()call somewhere (I'm not sure exactly where) is then simplifying that union back into aCallabletype, but with theUnionType::filter()patch above applied, this no longer happens, so it remains aUnionType. The result of that is that somewhere down the line, we appear to have a branch of code that treats a singleCallabledifferently to a union ofCallables (again, I haven't been able to track down precisely where this is, unfortunately), so that with the patch applied we infer the type ofloadas being(...) -> Unknownwhereas onmainwe infer the type ofloadas being[T](x: T, name: str, validate: bool = ...) -> T. The inference onmainseems clearly better to me!It's desirable to be able to apply something similar to the patch above, because it leads to a significant speedup, especially on the incremental benchmark. Meanwhile, it's desirable to avoid the behaviour change, because the above snippet is a minimized repro of code in the prefect ecosystem project (specifically,
src/integrations/prefect-azure/prefect_azure/experimental/bundles/execute.py). These weren't the flakes that we usually see onprefect: the reduction in diagnostics on this file persisted across several mypy_primer runs and could also be consistently reproduced locally.More debugging details
The reason for the behaviour change appears to be that
loadis inferred as having type(...) -> Unknownwith the above patch applied, whereas onmainit's inferred as having type[T](x: T, name: str, validate: bool = ...) -> T.To investigate further, I added some debug prints to the
mainbranch:And then ran the
mainbranch on the above repro, with these results:So we can see that on
main, the type ofaloadis inferred to be a union with redundant elements in it (presumably due to cycle recovery somewhere), and then theUnionType::filter()call simplifies that union to a non-union type. Whereas on this branch, theUnionType::filter()call sees that all elements in the union satisfy thefilterpredicate, assumes that the union will already have been adequately normalized, and returns the union as-is.The next question to investigate is: why are we treating a union of two identical callables differently to a single callable? They should be treated equivalently by ty...
Adding these debug prints indicates that after inferring the type of
aloadas (in different cycle iterations)([T](x: T, name: str, validate: bool = ...) -> T)and([T](x: T, name: str, validate: bool = True) -> T), at some point we fallback to(...) -> Unknown:Unanswered questions
There's still a lot I don't fully understand about what's going on here. I've tried to figure out some of them, but without much success:
aload?aloadon different iterations of the cycle?UnionType::filter()call that simplifies the union back to a singleCallabletype onmainand "saves" us?loadifaloadis inferred as having a union type at some point?Proposed change in this PR
Despite the unanswered questions above, I'm proposing a change to our cycle normalization for
Parameters. I don't think the questions above need to be answered for this PR to be a demonstrably worthwhile change.On this PR branch, we discard the exact type of a parameter default (replacing it with
Never, simply because it's an extremely simple type) when cycle-normalizingParameters. We now retain only the knowledge that the parameter has a default value -- which is all that is relevant for type-checking. The only situation where we ever use the type of the default value is when we're displaying a signature to the user in a diagnostic or on-hover. This is exactly the same as what we already do for "regular" (non-cycle) normalization:ruff/crates/ty_python_semantic/src/types/signatures.rs
Lines 2376 to 2403 in e8e8235
The change made here means that we are able to identify even inside cycle recovery that
[T](x: T, name: str, validate: bool = ...) -> Tis equivalent to[T](x: T, name: str, validate: bool = True) -> T-- because they both cycle-normalize to the same type:[T](x: T, name: str, validate: bool = ...) -> T. As such, applying the above patch to this PR branch does not cause ty to exhibit the same behaviour change as applying the same patch tomaindoes.This PR therefore unblocks the performance optimisation proposed in #22352. I also think it makes sense on its own merits:
Parameters, which should theoretically speed us up when analyzing projects with many nested cycles.Callabletypes converge successfully.As well as the above motivations, I'm making this a standalone change to demonstrate that it has no impact on the ecosystem, our test suite, or our benchmarks. (Codspeed reports speedups of 1-2% on some benchmarks, but that's small enough that it could well just be noise.)
Test Plan