[ty] do not union Unknown into unannotated container types#23718
[ty] do not union Unknown into unannotated container types#23718
Conversation
Typing conformance results improved 🎉The percentage of diagnostics emitted that were expected errors increased from 86.95% to 87.07%. The percentage of expected errors that received a diagnostic increased from 77.43% to 77.62%. SummaryHow are test cases classified?Each test case represents one expected error annotation or a group of annotations sharing a tag. Counts are per test case, not per diagnostic — multiple diagnostics on the same line count as one. Required annotations (
Test file breakdown1 file altered
True positives added (2)2 diagnostics
False positives removed (1)1 diagnostic
True positives changed (8)8 diagnostics
|
Memory usage reportSummary
Significant changesClick to expand detailed breakdownflake8
trio
sphinx
prefect
|
|
03b7cec to
35e5f43
Compare
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-argument-type |
996 | 247 | 1,728 |
invalid-assignment |
688 | 9 | 185 |
unsupported-operator |
38 | 16 | 325 |
unresolved-attribute |
25 | 8 | 304 |
type-assertion-failure |
30 | 101 | 60 |
no-matching-overload |
170 | 0 | 0 |
invalid-return-type |
48 | 0 | 66 |
unused-type-ignore-comment |
2 | 58 | 0 |
invalid-parameter-default |
0 | 0 | 34 |
not-iterable |
1 | 0 | 32 |
redundant-cast |
10 | 0 | 0 |
call-non-callable |
1 | 3 | 4 |
not-subscriptable |
0 | 0 | 7 |
unsupported-base |
4 | 0 | 0 |
division-by-zero |
0 | 2 | 0 |
invalid-declaration |
1 | 0 | 1 |
assert-type-unspellable-subtype |
1 | 0 | 0 |
call-top-callable |
1 | 0 | 0 |
invalid-exception-caught |
0 | 0 | 1 |
possibly-unresolved-reference |
0 | 1 | 0 |
| Total | 2,016 | 445 | 2,747 |
|
09fcc93 to
24e967a
Compare
charliermarsh
left a comment
There was a problem hiding this comment.
This makes sense to me (though seems unlikely that I should be the sole approver here).
dcreager
left a comment
There was a problem hiding this comment.
We've discussed this extensively as a team, so 👍 to change. And thank you for the thorough ecosystem analysis analysis! I agree with all of your suggestions for punting things to follow-on work.
| // If a valid type annotation was not provided, avoid restricting the type of the | ||
| // collection by unioning the inferred type with `Unknown`. | ||
| let elt_tcx = elt_tcx.unwrap_or(Type::unknown()); | ||
|
|
||
| builder | ||
| .infer(&constraints, Type::TypeVar(elt_ty), elt_tcx) | ||
| .ok()?; | ||
| // If there is no applicable context for this element type variable, we infer from the | ||
| // literal elements directly. This violates the gradual guarantee (we don't know that | ||
| // our inference is compatible with subsequent additions to the collection), but it | ||
| // matches the behavior of other type checkers and is usually the desired behavior. | ||
| if let Some(elt_tcx) = elt_tcx { | ||
| builder | ||
| .infer(&constraints, Type::TypeVar(elt_ty), elt_tcx) | ||
| .ok()?; | ||
| } |
There was a problem hiding this comment.
A truly overwhelming amount of code diff to review here 😂
* main: Update conformance suite commit hash (#23746) conformance.py: Collapse the summary paragraph when nothing changed (#23745) [ty] Make inferred specializations line up with source types more better (#23715) Bump 0.15.5 (#23743) [ty] Render all changed diagnostics in conformance.py (#23613) [ty] Split deferred checks out of `types/infer/builder.rs` (#23740) Discover markdown files by default in preview mode (#23434) [ty] Use `HasOptionalDefinition` for `except` handlers (#23739) [ty] Fix precedence of `all` selector in TOML configurations (#23723) [ty] Make `all` selector case sensitive (#23713) [ty] Add a diagnostic if a `TypeVar` is used to specialize a `ParamSpec`, or vice versa (#23738) [ty] Override home directory in ty tests (#23724) [ty] More type-variable default validation (#23639) [ty] Validate bare ParamSpec usage in type annotations, and support stringified ParamSpecs as the first argument to `Callable` (#23625) [ty] Add `all` selector to ty.json's `schema` (#23721) [ty] Add quotes to related issues links (#23720) [ty] Fix panic on incomplete except handlers (#23708)
|
I have a stacked draft PR implementing the negative-intersection-element removal, and it looks pretty good. I think that's the highest-priority follow-up, so I'll go ahead and land this. |
## Summary Avoid inferring invariant container types like `list[int & ~AlwaysFalsy]`, preferring `list[int]` instead. Also generally rename "literal promotion" to just "promotion" -- it already promoted e.g. `float` to `int | float`, so it wasn't restricted to literal types. Now it includes even more non-literal cases. ## Test Plan Added an mdtest. Removes ~200 ecosystem hits from #23718
|
Landed #23750 as the highest-priority follow-up here, and filed astral-sh/ty#2971 and astral-sh/ty#2972 as additional follow-ups. Marked both as child issues of astral-sh/ty#1473. |
Summary
Part of astral-sh/ty#1240
Stop unioning
Unknowninto the types of un-annotated container literals.We discussed perhaps continuing to union
Unknownif the inferred type is a singleton type likeNone. I'd like to explore this as a separate change so we can see the ecosystem impact more clearly.Test Plan
Adjusted many mdtest expectations.
There's one test case that regresses with this change, because we don't fully support union type contexts (it can require a lot of repeat inference in pathological cases). So
x10: list[int | str] | list[int | None] = [1, 2, 3]previously passed only because we inferred the RHS aslist[Unknown | int]-- now we infer it aslist[int]and the assignment fails due to invariance. I've kept this test as a TODO since it's not trivial to fix. Mypy errors in the same way we now do, suggesting it's not necessarily a huge priority either.Ecosystem
This change is expected to cause new diagnostics and some false positives, since we are replacing very-forgiving gradual types with non-gradual inference heuristics.
Many of these issues could be solved or significantly mitigated by astral-sh/ty#1473, depending how far we are able to go with that, and particularly whether we can afford to apply it also to container literals which are not empty at construction. The downside of broad application of this approach is that in some cases it could cause us to widen container types when the user actually just made a mistake and added the wrong thing to a container, and would prefer an error at that location.
Some categories of new error that show up in the ecosystem report:
Implicit TypedDicts
These are cases where the dictionary is heterogeneous and would ideally be typed as a
TypedDictbut isn't, for example:We (and pyrefly, and pyright in strict mode) error on the last line here because we already inferred
dict[str, str | int], so we can't add abytesvalue.Mypy prefers common-base joins over union joins, so it infers
dict[str, object], which avoids the error adding abytesvalue. This means the value type is less precise, which theoretically means potentially more errors using values from the dict later. But in practice with this heterogeneous pattern, eitherobjector the union will cause similar problems when using values from the dict -- in either case you'd probably have to cast or narrow.Pyright (in non-strict mode) has a special case where it falls back to
Unknownwhen it sees heterogenous value types, so it infers this asdict[str, Unknown].I think we could consider either the mypy or pyright approaches here, but we don't need to do it in this PR; we can file an issue and consider it as a follow-up.
Another symptom of this same root cause is repetitive diagnostics arising from a large union inferred as value type; the same fixes would address this.
Negative intersections, particularly with e.g.
~AlwaysFalsyor~None.Example:
We error on
return dbecause "expecteddict[str, A], founddict[str, A & ~AlwaysFalsy]". This is an issue specific to intersection types, so no other type checker has this problem.I think when we "promote literals" (we may need to give this operation a broader name -- it's really "type promotion to give a better inferred type when invariance means too-precise is bad") we should also eliminate all negative types from intersections. I would prefer to do this as a separate PR for easier review and better visibility of ecosystem impact, but I think it's high priority to land soon after this PR (ideally before a release).
Overly-precise inference for singleton
NoneThis did show up, to the tune of ~100 new diagnostics (example), so I think it is worth addressing as a follow-up.