[red-knot] Fix equivalence of differently ordered unions that contain Callable types#17145
[red-knot] Fix equivalence of differently ordered unions that contain Callable types#17145AlexWaygood merged 2 commits intomainfrom
Callable types#17145Conversation
… `Callable` types
|
There's some context on this in astral-sh/ty#165 and the linked comment in that issue description. |
carljm
left a comment
There was a problem hiding this comment.
This looks good to me. Might be nice for @dhruvmanila to take a look as well?
The tests don't really seem to cover any of the parameter normalization behaviors?
In I should also add a test where there a parameter in a function |
dhruvmanila
left a comment
There was a problem hiding this comment.
This looks good apart from one minor comment, thanks!
* origin/main: (35 commits) [red-knot] Callable types are disjoint from literals (#17160) [red-knot] Fix inference for `pow` between two literal integers (#17161) [red-knot] Add GitHub PR annotations when mdtests fail in CI (#17150) [red-knot] Fix equivalence of differently ordered unions that contain `Callable` types (#17145) [red-knot] Add initial set of tests for unreachable code (#17159) [`airflow`] Move `AIR302` to `AIR301` and `AIR303` to `AIR302` (#17151) ruff_db: simplify lifetimes on `DiagnosticDisplay` [red-knot] Detect division-by-zero in unions and intersections (#17157) [`airflow`] Add autofix infrastructure to `AIR302` name checks (#16965) [`flake8-bandit`] Mark `str` and `list[str]` literals as trusted input (`S603`) (#17136) [`airflow`] Add autofix for `AIR302` attribute checks (#16977) [`airflow`] Extend `AIR302` with additional symbols (#17085) [`airflow`] Move `AIR301` to `AIR002` (#16978) [`airflow`] Add autofix for `AIR302` method checks (#16976) ruff_db: switch diagnostic rendering over to `std::fmt::Display` [red-knot] Add 'Goto type definition' to the playground (#17055) red_knot_ide: update snapshots red_knot_python_semantic: remove comment about `TypeCheckDiagnostic` ruff_db: delete most of the old diagnostic code red_knot: use `Diagnostic` inside of red knot ...
* origin/main: (82 commits) [red-knot] Fix more [redundant-cast] false positives (#17170) [red-knot] Three-argument type-calls take 'str' as the first argument (#17168) Control flow: `return` and `raise` (#17121) Bump 0.11.3 (#17173) [red-knot] Improve `Debug` implementation for `semantic_index::SymbolTable` (#17172) [red-knot] Fix `str(…)` calls (#17163) [red-knot] visibility_constraint analysis for match cases (#17077) [red-knot] Fix playground crashes when diagnostics are stale (#17165) [red-knot] Callable types are disjoint from literals (#17160) [red-knot] Fix inference for `pow` between two literal integers (#17161) [red-knot] Add GitHub PR annotations when mdtests fail in CI (#17150) [red-knot] Fix equivalence of differently ordered unions that contain `Callable` types (#17145) [red-knot] Add initial set of tests for unreachable code (#17159) [`airflow`] Move `AIR302` to `AIR301` and `AIR303` to `AIR302` (#17151) ruff_db: simplify lifetimes on `DiagnosticDisplay` [red-knot] Detect division-by-zero in unions and intersections (#17157) [`airflow`] Add autofix infrastructure to `AIR302` name checks (#16965) [`flake8-bandit`] Mark `str` and `list[str]` literals as trusted input (`S603`) (#17136) [`airflow`] Add autofix for `AIR302` attribute checks (#16977) [`airflow`] Extend `AIR302` with additional symbols (#17085) ...
… `Callable` types (astral-sh#17145) ## Summary Fixes astral-sh#17058. Equivalent callable types were not understood as equivalent when they appeared nested inside unions and intersections. This PR fixes that by ensuring that `Callable` elements nested inside unions, intersections and tuples have their representations normalized before one union type is compared with another for equivalence, or before one intersection type is compared with another for equivalence. The normalizations applied to a `Callable` type are: - the type of the default value is stripped from all parameters (only whether the parameter _has_ a default value is relevant to whether one `Callable` type is equivalent to another) - The names of the parameters are stripped from positional-only parameters, variadic parameters and keyword-variadic parameters - Unions and intersections that are present (top-level or nested) inside parameter annotations or return annotations are normalized. Adding a `CallableType::normalized()` method also allows us to simplify the implementation of `CallableType::is_equivalent_to()`. ### Should these normalizations be done eagerly as part of a `CallableType` constructor? I considered this. It's something that we could still consider doing in the future; this PR doesn't rule it out as a possibility. However, I didn't pursue it for now, for several reasons: 1. Our current `Display` implementation doesn't handle well the possibility that a parameter might not have a name or an annotated type. Callable types with parameters like this would be displayed as follows: ```py (, ,) -> None: ... ``` That's fixable! It could easily become something like `(Unknown, Unknown) -> None: ...`. But it also illustrates that we probably want to retain the parameter names when displaying the signature of a `lambda` function if you're hovering over a reference to the lambda in an IDE. Currently we don't have a `LambdaType` struct for representing `lambda` functions; if we wanted to eagerly normalize signatures when creating `CallableType`s, we'd probably have to add a `LambdaType` struct so that we would retain the full signature of a `lambda` function, rather than representing it as an eagerly simplified `CallableType`. 2. In order to ensure that it's impossible to create `CallableType`s without the parameters being normalized, I'd either have to create an alternative `SimplifiedSignature` struct (which would duplicate a lot of code), or move `CallableType` to a new module so that the only way of constructing a `CallableType` instance would be via a constructor method that performs the normalizations eagerly on the callable's signature. Again, this isn't a dealbreaker, and I think it's still an option, but it would be a lot of churn, and it didn't seem necessary for now. Doing it this way, at least to start with, felt like it would create a diff that's easier to review and felt like it would create fewer merge conflicts for others. ## Test Plan - Added a regression mdtest for astral-sh#17058 - Ran `QUICKCHECK_TESTS=1000000 cargo test --release -p red_knot_python_semantic -- --ignored types::property_tests::stable`
Summary
Fixes #17058.
Equivalent callable types were not understood as equivalent when they appeared nested inside unions and intersections. This PR fixes that by ensuring that
Callableelements nested inside unions, intersections and tuples have their representations normalized before one union type is compared with another for equivalence, or before one intersection type is compared with another for equivalence.The normalizations applied to a
Callabletype are:Callabletype is equivalent to another)Adding a
CallableType::normalized()method also allows us to simplify the implementation ofCallableType::is_equivalent_to().Should these normalizations be done eagerly as part of a
CallableTypeconstructor?I considered this. It's something that we could still consider doing in the future; this PR doesn't rule it out as a possibility. However, I didn't pursue it for now, for several reasons:
Our current
Displayimplementation doesn't handle well the possibility that a parameter might not have a name or an annotated type. Callable types with parameters like this would be displayed as follows:That's fixable! It could easily become something like
(Unknown, Unknown) -> None: .... But it also illustrates that we probably want to retain the parameter names when displaying the signature of alambdafunction if you're hovering over a reference to the lambda in an IDE. Currently we don't have aLambdaTypestruct for representinglambdafunctions; if we wanted to eagerly normalize signatures when creatingCallableTypes, we'd probably have to add aLambdaTypestruct so that we would retain the full signature of alambdafunction, rather than representing it as an eagerly simplifiedCallableType.In order to ensure that it's impossible to create
CallableTypes without the parameters being normalized, I'd either have to create an alternativeSimplifiedSignaturestruct (which would duplicate a lot of code), or moveCallableTypeto a new module so that the only way of constructing aCallableTypeinstance would be via a constructor method that performs the normalizations eagerly on the callable's signature. Again, this isn't a dealbreaker, and I think it's still an option, but it would be a lot of churn, and it didn't seem necessary for now. Doing it this way, at least to start with, felt like it would create a diff that's easier to review and felt like it would create fewer merge conflicts for others.Test Plan
Callabletypes are not always considered equivalent #17058QUICKCHECK_TESTS=1000000 cargo test --release -p red_knot_python_semantic -- --ignored types::property_tests::stable