-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty] Distinguish "unconstrained" from "constrained to any type" #21539
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
Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
carljm
left a comment
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.
Awesome!
|
|
||
| # fmt: off | ||
|
|
||
| def constrained_by_gradual[T: (Base, Any)](): |
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.
This is a weird case, and the more I think about it, the weirder it gets.
I think the principles of constrained type var should probably be:
- We should always solve to one of the constrained types (or to
UnknownorAny). This is the core invariant of constrained type variables. - If the constraints don't allow us to narrow down to a single constrained type, but could allow multiple, we should prefer the subtype, if one constrained type is a subtype of the other. For example, for
def f[T: (Base, Sub)](x: T)we should prefer solvingf(typed_as sub)toSubrather thanBase, even though both solutions would be possible. - If multiple constrained types are possible, and neither is a subtype of the other, we should... do what? Fall back to the typevar default?
But if we agree with these principles, then we can never solve this constrained type var to Base, because any set of constraints that would allow us to solve to Base would also allow solving to Any, therefore we can't narrow it down to one of the choices.
mypy seems to just always solve such a typevar to Any. (Mypy doesn't support typevar defaults, nor distinguish Any vs Unknown, so I can't really experiment with how it handles fallback-to-default.)
pyright also always solves to Any, unless the typevar is totally unconstrained, in which case it falls back to the typevar default (which I've explicitly specified to be Base in this example, but could also just implicitly be Unknown.) I'm not sure how pyright decides that f(base) or f(sub) should be Any rather than "the typevar default", given that those calls are compatible with either constrained type.
pyrefly does the same thing you originally did here (always solve to a fully static type), which I'm pretty sure is wrong. It fails both the promises of a constrained type var and of the gradual guarantee, and I suspect will cause false positive errors in practice (the fact that pyright and mypy both prefer solving to Any strengthens this intuition.) If the Any constrained type means "there is really some static type here, but we don't know what it is" (imagine its an Unknown from a failed import of a type), then it's wrong to solve to some arbitrary fully static type, because we don't know that's what the constrained type was "supposed" to be - and it could cause a lot of cascading errors.
(I think it's fine to punt all of this, but I will suggest some different TODOs below based on this analysis.)
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.
But if we agree with these principles, then we can never solve this constrained type var to
Base, because any set of constraints that would allow us to solve toBasewould also allow solving toAny, therefore we can't narrow it down to one of the choices.
Is it worth trying to consider which materializations of Any are valid specializations? If Base is a valid specialization but no subtypes are (if that were to violate some other requirement of the specialization), then the Any could not materialize to anything that would take precedence according to your rule (2).
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.
Yes, that makes sense! So if we have a lower bound of Base (e.g. because we got a call that provided an instance of Base as argument for this typevar), then we can solve to Base because it must be the most precise option.
| # revealed: ty_extensions.Specialization[T@constrained_by_gradual = Base] | ||
| reveal_type(generic_context(constrained_by_gradual).specialize_constrained(ConstraintSet.always())) |
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.
I would expect this to solve to the typevar default, in this case Unknown. What information allows us to pick Base over Any here? (If we are picking between Base and Any, I think Any is probably the better choice in practice, and is what mypy/pyright choose, but I'm not sure what heuristic they are using.)
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.
What information allows us to pick
BaseoverAnyhere?
This is a side effect of the change I added from this discussion #21414 (comment). If there are multiple solutions, my original plan was to produce the union of them. But I don't think that's correct, so instead we either:
- find a single specialization that satisfies all of the solutions, or
- return
Noneto indicate an ambiguous result.
Here, the constraint of Any can materialize to any type T = *, and still satisfy ConstraintSet.always(). The constraint of Base only satisfies the constraint set when T = Base. The intersection of those two solutions is T = Base, and so that's what we return.
| # TODO: revealed: ty_extensions.Specialization[T@constrained_by_gradual = Any] | ||
| # revealed: ty_extensions.Specialization[T@constrained_by_gradual = object] | ||
| reveal_type(generic_context(constrained_by_gradual).specialize_constrained(ConstraintSet.always())) | ||
| reveal_type(generic_context(constrained_by_gradual).specialize_constrained(ConstraintSet.range(Never, T, object))) |
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.
Here I think either Any or Unknown (typevar default) would be reasonable choices?
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.
Yep, this one already has Any as the TODO
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.
(This one I think the right TODO is Any not Unknown because here we are now explicitly saying that T can be any type, as opposed to not having said anything about what T can be. The latter clearly should fall back on the default. The former I don't think should.)
| # revealed: ty_extensions.Specialization[T@constrained_by_gradual = Base] | ||
| reveal_type(generic_context(constrained_by_gradual).specialize_constrained(ConstraintSet.range(Never, T, Base))) |
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.
Pyright and mypy would also solve this to Any. Similar to above, I'm not sure why we'd pick Base over Any here as a solution.
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.
Added TODO
| # revealed: ty_extensions.Specialization[T@constrained_by_gradual = Base] | ||
| reveal_type(generic_context(constrained_by_gradual).specialize_constrained(ConstraintSet.range(Never, T, Super))) |
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.
Same as above cases, I think this should also solve to Any or Unknown
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.
Added TODO
| # revealed: ty_extensions.Specialization[T@constrained_by_gradual = Base] | ||
| reveal_type(generic_context(constrained_by_gradual).specialize_constrained(ConstraintSet.range(Sub, T, object))) |
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.
And again here.
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.
Added TODO
| /// | ||
| /// We support several kinds of sequent: | ||
| /// | ||
| /// - `¬C₁ → false`: This indicates that `C₁` is always true. Any path that assumes it is false is |
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.
Mostly just curiosity: is there an important reason why we frame this as ¬C₁ → false rather than C₁ → true? Are these different in some way? The prose here suggests not.
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.
"C₁ is always true" is true → C₁, not C₁ → true. But given that change, you're right that true → C₁ and ¬C₁ → false are the same thing. One focuses on what is always true, and the other focuses on what's impossible. The focus of this data structure is to track situations that are impossible, so I thought it would be best to frame it with false on the rhs.
Though I guess that means I could go further and rewrite some the examples below — for instance C₁ ∧ C₂ → D and C₁ ∧ C₂ ∧ ¬D → false are equivalent in exactly the same way!
…d-typevar * origin/main: (24 commits) [ty] Remove brittle constraint set reveal tests (#21568) [`ruff`] Catch more dummy variable uses (`RUF052`) (#19799) [ty] Use the same snapshot handling as other tests (#21564) [ty] suppress autocomplete suggestions during variable binding (#21549) Set severity for non-rule diagnostics (#21559) [ty] Add `with_type` convenience to display code (#21563) [ty] Implement docstring rendering to markdown (#21550) [ty] Reduce indentation of `TypeInferenceBuilder::infer_attribute_load` (#21560) Bump 0.14.6 (#21558) [ty] Improve debug messages when imports fail (#21555) [ty] Add support for relative import completions [ty] Refactor detection of import statements for completions [ty] Use dedicated collector for completions [ty] Attach subdiagnostics to `unresolved-import` errors for relative imports as well as absolute imports (#21554) [ty] support PEP 613 type aliases (#21394) [ty] More low-hanging fruit for inlay hint goto-definition (#21548) [ty] implement `TypedDict` structural assignment (#21467) [ty] Add more random TypeDetails and tests (#21546) [ty] Add goto for `Unknown` when it appears in an inlay hint (#21545) [ty] Add type definitions for `Type::SpecialForm`s (#21544) ...
…d-typevar * origin/main: (30 commits) [ty] Double click to insert inlay hint (#21600) [ty] Switch the error code from `unresolved-attribute` to `possibly-missing-attribute` for submodules that may not be available (#21618) [ty] Substitute for `typing.Self` when checking protocol members (#21569) [ty] Don't suggest things that aren't subclasses of `BaseException` after `raise` [ty] Add hint about resolved Python version when a user attempts to import a member added on a newer version (#21615) Use release commit for actions/checkout (#21610) [ty] Add failing mdtest for known `Protocol` panic (#21594) [`parser`] Fix panic when parsing IPython escape command expressions (#21480) Fix cargo shear in CI (#21609) Update actions/checkout digest to c2d88d3 (#21601) Update dependency ruff to v0.14.6 (#21603) Update astral-sh/setup-uv action to v7.1.4 (#21602) Update Rust crate clap to v4.5.53 (#21604) Update taiki-e/install-action action to v2.62.56 (#21608) Update Rust crate hashbrown to v0.16.1 (#21605) Update Rust crate indexmap to v2.12.1 (#21606) Update Rust crate syn to v2.0.111 (#21607) [ty] Check method definitions on subclasses for Liskov violations (#21436) [ty] Fix panic for unclosed string literal in type annotation position (#21592) [ty] Fix rendering of unused suppression diagnostic (#21580) ...
* main: [ty] Extend Liskov checks to also cover classmethods and staticmethods (astral-sh#21598) Dogfood ty on the `scripts` directory (astral-sh#21617) [ty] support generic aliases in `type[...]`, like `type[C[int]]` (astral-sh#21552) [ty] Retain the function-like-ness of `Callable` types when binding `self` (astral-sh#21614) [ty] Distinguish "unconstrained" from "constrained to any type" (astral-sh#21539) Disable ty workspace diagnostics for VSCode users (astral-sh#21620) [ty] Double click to insert inlay hint (astral-sh#21600) [ty] Switch the error code from `unresolved-attribute` to `possibly-missing-attribute` for submodules that may not be available (astral-sh#21618) [ty] Substitute for `typing.Self` when checking protocol members (astral-sh#21569) [ty] Don't suggest things that aren't subclasses of `BaseException` after `raise` [ty] Add hint about resolved Python version when a user attempts to import a member added on a newer version (astral-sh#21615)
Before, we would collapse any constraint of the form
Never ≤ T ≤ objectdown to the "always true" constraint set. This is correct in terms of BDD semantics, but loses information, since "not constraining a typevar at all" is different than "constraining a typevar to take on any type". Once we get to specialization inference, we should fall back on the typevar's default for the former, but not for the latter.This is much easier to support now that we have a sequent map, since we need to treat
¬(Never ≤ T ≤ object)as being impossible, and prune it when we walk through BDD paths, just like we do for other impossible combinations.