Skip to content

Conversation

@AlexWaygood
Copy link
Member

Summary

Currently our Type::Callable wraps a four-variant CallableType enum. But as time has gone on, I think we've found that the four variants in CallableType are really more different to each other than they are similar to each other:

  • GeneralCallableType is a structural type describing all callable types with a certain signature, but the other three types are "literal types", more similar to the FunctionLiteral variant
  • GeneralCallableType is not a singleton or a single-valued type, but the other three are all single-valued types (WrapperDescriptorDunderGet is even a singleton type)
  • GeneralCallableType has (or should have) ambiguous truthiness, but all possible inhabitants of the other three types are always truthy.
  • As a structural type, GeneralCallableType can contain inner unions and intersections that must be sorted in some contexts in our internal model, but this is not true for the other three variants.

This PR flattens Type::Callable into four distinct Type:: variants. In the process, it fixes a number of latent bugs that were concealed by the current architecture but are laid bare by the refactor. Unit tests for these bugs are included in the PR.

@AlexWaygood AlexWaygood added the ty Multi-file analysis & type inference label Apr 1, 2025
@AlexWaygood AlexWaygood requested a review from dhruvmanila April 1, 2025 17:08
@github-actions
Copy link
Contributor

github-actions bot commented Apr 1, 2025

mypy_primer results

No ecosystem changes detected ✅

@AlexWaygood AlexWaygood force-pushed the alex/flatten-callable-types branch 2 times, most recently from 4cfc2e3 to ae40234 Compare April 1, 2025 17:16
@AlexWaygood
Copy link
Member Author

Some other notes:

  • This structure more accurately reflects the fact that lots of the Type variants have possible inhabitants that would also inhabit Callable (instances can be callable, class objects are nearly always callable, same for type[] types) -- the distinction implied by the current structure that there are "4 callable types" and all other variants do therefore not represent "callable types" is incorrect
  • This doesn't fix [red-knot] Differently ordered unions that include Callable types are not always considered equivalent  #17058, but it should make it easier to fix that issue (at least how I'm currently thinking of fixing it!)

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.

Nice, yeah I think this structure is less error-prone: avoids grouping very different types together in a way that makes it too easy to wrongly handle them all identically.

@AlexWaygood AlexWaygood force-pushed the alex/flatten-callable-types branch from db872b1 to ff29d69 Compare April 1, 2025 18:24
@AlexWaygood AlexWaygood force-pushed the alex/flatten-callable-types branch from ff29d69 to 3853d0f Compare April 1, 2025 18:26
@AlexWaygood AlexWaygood merged commit d6dcc37 into main Apr 1, 2025
22 checks passed
@AlexWaygood AlexWaygood deleted the alex/flatten-callable-types branch April 1, 2025 18:30
@dhruvmanila
Copy link
Member

dhruvmanila commented Apr 1, 2025

Thanks Alex for doing this! I still want to take a stab at merging the types to reduce the number of variants but that's something for later.

dcreager added a commit that referenced this pull request Apr 1, 2025
* main:
  [red-knot] Add property tests for callable types (#17006)
  [red-knot] Disjointness for callable types (#17094)
  [red-knot] Flatten `Type::Callable` into four `Type` variants (#17126)
  mdtest.py: do a full mdtest run immediately when the script is executed (#17128)
  [red-knot] Fix callable subtyping for standard parameters (#17125)
  [red-knot] Fix more `redundant-cast` false positives (#17119)
  Sync vendored typeshed stubs (#17106)
  [red-knot] support Any as a class in typeshed (#17107)
  Visit `Identifier` node as part of the `SourceOrderVisitor` (#17110)
  [red-knot] Don't infer Todo for quite so many tuple type expressions (#17116)
  CI: Run pre-commit on depot machine (#17120)
  Error instead of `panic!` when running Ruff from a deleted directory (#16903) (#17054)
  Control flow graph: setup (#17064)
  [red-knot] Playground improvements (#17109)
  [red-knot] IDE crate (#17045)
  Update dependency vite to v6.2.4 (#17104)
  [red-knot] Add redundant-cast error (#17100)
  [red-knot] Narrowing on `in tuple[...]` and `in str` (#17059)
@sharkdp
Copy link
Contributor

sharkdp commented Apr 1, 2025

I still want to take a stab at merging the types to reduce the number of variants but that's something for later.

It would be great if we could postpone this until after #17017 has been merged; that PR makes major changes to two of these callable types, and rebasing on top of this branch here was a significant amount of work already. But in general, that sounds like a good idea 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants