Skip to content

Comments

[red-knot] Fix equivalence of differently ordered unions that contain Callable types#17145

Merged
AlexWaygood merged 2 commits intomainfrom
alex/callable-equivalence-2
Apr 2, 2025
Merged

[red-knot] Fix equivalence of differently ordered unions that contain Callable types#17145
AlexWaygood merged 2 commits intomainfrom
alex/callable-equivalence-2

Conversation

@AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Apr 2, 2025

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 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:

    (, ,) -> 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 CallableTypes, 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 CallableTypes 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

@AlexWaygood AlexWaygood added bug Something isn't working ty Multi-file analysis & type inference labels Apr 2, 2025
@AlexWaygood AlexWaygood requested a review from dhruvmanila April 2, 2025 10:39
@github-actions
Copy link
Contributor

github-actions bot commented Apr 2, 2025

mypy_primer results

No ecosystem changes detected ✅

@dhruvmanila
Copy link
Member

Should these normalizations be done eagerly as part of a CallableType constructor?

There's some context on this in astral-sh/ty#165 and the linked comment in that issue description.

Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Apr 2, 2025

The tests don't really seem to cover any of the parameter normalization behaviors?

In is_equivalent_to.md, f3 and f4 have equivalent Callable supertypes even though their parameters are differently named. The differently named parameters caused the added test to fail previously: the Callable supertypes would compare equivalent if you compared them directly, but would not compare equivalent when nested inside unions and intersections. So the tests cover one part of the parameter normalization behaviour; the tests fail without those specific changes.

I should also add a test where there a parameter in a function X has a different default value type to a parameter in a function Y, though; that's a good point.

Copy link
Member

@dhruvmanila dhruvmanila left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good apart from one minor comment, thanks!

@AlexWaygood AlexWaygood enabled auto-merge (squash) April 2, 2025 17:41
@AlexWaygood AlexWaygood merged commit c2bb5d5 into main Apr 2, 2025
21 checks passed
@AlexWaygood AlexWaygood deleted the alex/callable-equivalence-2 branch April 2, 2025 17:43
dcreager added a commit that referenced this pull request Apr 3, 2025
* 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
  ...
dcreager added a commit that referenced this pull request Apr 3, 2025
* 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)
  ...
maxmynter pushed a commit to maxmynter/ruff that referenced this pull request Apr 3, 2025
… `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`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[red-knot] Differently ordered unions that include Callable types are not always considered equivalent

3 participants