[ty] No union with Unknown for module-global symbols
#20664
Merged
+83
−93
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Diagnostic diff on typing conformance testsChanges were detected when running ty on typing conformance tests--- old-output.txt 2025-10-01 14:28:45.546118497 +0000
+++ new-output.txt 2025-10-01 14:28:48.745147662 +0000
@@ -373,7 +373,7 @@
generics_defaults_specialization.py:30:1: error[non-subscriptable] Cannot subscript object of type `<class 'SomethingWithNoDefaults[int, typing.TypeVar]'>` with no `__class_getitem__` method
generics_defaults_specialization.py:45:1: error[type-assertion-failure] Argument does not have asserted type `@Todo(unsupported nested subscript in type[X])`
generics_paramspec_basic.py:27:38: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `int`
-generics_paramspec_components.py:49:20: error[invalid-argument-type] Argument expression after ** must be a mapping type: Found `tuple[Unknown, ...]`
+generics_paramspec_components.py:49:20: error[invalid-argument-type] Argument expression after ** must be a mapping type: Found `tuple[@Todo(Support for `typing.ParamSpec`), ...]`
generics_paramspec_components.py:83:18: error[parameter-already-assigned] Multiple values provided for parameter 1 (`x`) of function `foo`
generics_paramspec_semantics.py:13:56: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `(...) -> str`
generics_paramspec_semantics.py:17:40: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `int`
@@ -731,6 +731,7 @@
protocols_merging.py:54:1: error[invalid-assignment] Object of type `SCConcrete2` is not assignable to `SizedAndClosable3`
protocols_merging.py:67:16: error[invalid-protocol] Protocol class `BadProto` cannot inherit from non-protocol class `SizedAndClosable3`
protocols_merging.py:83:1: error[invalid-assignment] Object of type `SCConcrete1` is not assignable to `SizedAndClosable4`
+protocols_modules.py:25:1: error[invalid-assignment] Object of type `<module '_protocols_modules1'>` is not assignable to `Options1`
protocols_modules.py:26:1: error[invalid-assignment] Object of type `<module '_protocols_modules1'>` is not assignable to `Options2`
protocols_modules.py:47:1: error[invalid-assignment] Object of type `<module '_protocols_modules2'>` is not assignable to `Reporter1`
protocols_modules.py:48:1: error[invalid-assignment] Object of type `<module '_protocols_modules2'>` is not assignable to `Reporter2`
@@ -855,5 +856,5 @@
typeddicts_usage.py:28:17: error[missing-typed-dict-key] Missing required key 'name' in TypedDict `Movie` constructor
typeddicts_usage.py:28:18: error[invalid-key] Invalid key access on TypedDict `Movie`: Unknown key "title"
typeddicts_usage.py:40:24: error[invalid-type-form] The special form `typing.TypedDict` is not allowed in type expressions. Did you mean to use a concrete TypedDict or `collections.abc.Mapping[str, object]` instead?
-Found 856 diagnostics
+Found 857 diagnostics
WARN A fatal error occurred while checking some files. Not all project files were analyzed. See the diagnostics list above for details. |
|
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
unsupported-operator |
15 | 13 | 2,597 |
possibly-missing-attribute |
3 | 1,365 | 150 |
unresolved-attribute |
1,464 | 24 | 24 |
invalid-argument-type |
169 | 63 | 481 |
type-assertion-failure |
12 | 666 | 9 |
invalid-assignment |
53 | 65 | 73 |
unsupported-base |
0 | 0 | 191 |
unused-ignore-comment |
32 | 84 | 0 |
invalid-context-manager |
0 | 0 | 65 |
invalid-return-type |
27 | 6 | 31 |
unresolved-import |
1 | 48 | 0 |
conflicting-argument-forms |
0 | 36 | 0 |
possibly-unresolved-reference |
0 | 29 | 0 |
non-subscriptable |
0 | 1 | 20 |
invalid-type-form |
7 | 5 | 3 |
possibly-missing-import |
0 | 14 | 0 |
index-out-of-bounds |
0 | 9 | 2 |
invalid-exception-caught |
0 | 2 | 9 |
call-non-callable |
0 | 8 | 0 |
deprecated |
7 | 1 | 0 |
possibly-missing-implicit-call |
0 | 4 | 4 |
unresolved-reference |
4 | 3 | 0 |
no-matching-overload |
1 | 5 | 0 |
not-iterable |
1 | 1 | 2 |
invalid-parameter-default |
0 | 0 | 3 |
redundant-cast |
3 | 0 | 0 |
invalid-super-argument |
0 | 0 | 2 |
invalid-syntax-in-forward-annotation |
0 | 2 | 0 |
invalid-attribute-access |
1 | 0 | 0 |
invalid-key |
1 | 0 | 0 |
invalid-raise |
1 | 0 | 0 |
missing-argument |
1 | 0 | 0 |
too-many-positional-arguments |
1 | 0 | 0 |
unknown-argument |
1 | 0 | 0 |
unsupported-bool-conversion |
0 | 1 | 0 |
| Total | 1,805 | 2,455 | 3,666 |
sharkdp
changed the title
Unknown for module-global symbols (no promotion)Unknown for module-global symbols
sharkdp
added a commit
that referenced
this pull request
Oct 1, 2025
## Summary Reformulation of the public symbol type inference test suite to use class scopes instead of module scopes. This is in preparation for an upcoming change to module-global scopes (#20664). ## Test Plan Updated tests
sharkdp
force-pushed
the
david/no-unknown-union-no-promotion
branch
2 times, most recently
from
72c9048 to
23252c4
Compare
sharkdp
commented
Oct 1, 2025
Comment on lines
+843
to
+844
| // We generally trust undeclared places in stubs and do not union with `Unknown`. | ||
| let in_stub_file = scope.file(db).is_stub(db); |
There was a problem hiding this comment.
I noticed that we can now remove this exception here, but we'll evaluate that in a separate PR, to make sure it does not have any effects on the ecosystem.
sharkdp
force-pushed
the
david/no-unknown-union-no-promotion
branch
from
23252c4 to
b6e9af5
Compare
That's... an interesting test. Conceptually it seems to me to be the same as this, where we (correctly, I think) state that from typing import Protocol, Literal, reveal_type
from ty_extensions import is_assignable_to
class Nominal:
x: Literal[True]
class Proto(Protocol):
x: bool
reveal_type(is_assignable_to(Nominal, Proto)) # revealed: ConstraintSet[never]Pyright, mypy and pyrefly all agree with us on the class example, so I wonder why the spec appears to assert that modules should be treated differently here Pyrefly also agrees with us that the module should not be considered an instance of this protocol. |
|
Okay, both mypy and pyright do have internally consistent behaviour here. They both consider the module to inhabit the protocol because they do both promote the type of the module constant to I don't think the purpose of the test here is to assert that type checkers must promote the types of module constants when viewed from external scopes, however. I think it just wasn't considered that other type checkers might have different behaviour here, because the two established type checkers had the same behaviour when the test was written. I think this is easily addressed by adding type annotations to the global-scope attributes in |
sharkdp
force-pushed
the
david/no-unknown-union-no-promotion
branch
from
f762f97 to
b6e9af5
Compare
Hm. This does sound very much like "adding a type annotation to get rid of a type check error". So maybe there is something that we should do about it. Do you think it's something that can wait until after this PR has been merged? (I might want to do the literal-promotion analysis anyway) |
AlexWaygood
approved these changes
Oct 1, 2025
There was a problem hiding this comment.
Great work, thank you! I agree that it makes sense to do the literal-promotion experiments as a followup. This clearly reduces our false positives on user code as it is, so it's clearly a worthwhile change by itself, even without any literal-promotion behaviour.
Hm. This does sound very much like "adding a type annotation to get rid of a type check error". So maybe there is something that we should do about it. Do you think it's something that can wait until after this PR has been merged? (I might want to do the literal-promotion analysis anyway)
Yes, I agree that, strictly speaking, this shows that we're introducing a new violation of the gradual guarantee here. I think our options are:
- Promote all module constants to non-literal types if they're unannotated (this is what mypy does)
- Same as (1), but treat
ALL_CAPS_CONSTANTs as implicitlyFinal, and therefore exclude them fromLiteralpromotions (this is what pyright does) - Accept this violation of the gradual guarantee as an instance where the costs of fixing it would outweigh the benefits
- Some kind of ad-hoc special-casing for module-to-protocol assignability? TBH I'd really rather not do this, but we might need it anyway for astral-sh/ty#931. Though it's interesting that that issue has no upvotes so far (it doesn't seem to be affecting many users that our module-to-protocol subtyping is already stricter than other type checkers for method members)
I think any of these options would be defensible. The gradual guarantee isn't an iron-clad law; it's a guiding principle. It's okay to accept violations of it where we think fixing the violation would have detrimental impacts in other respects for our users.
FWIW: I intend anyway to work at some point on improving our Protocol assignability diagnostics so that we explain why we do not consider a certain type assignable to a given Protocol. Pyright's and mypy's diagnostics (especially mypy's) are really nice here; we can and should do much better than what we currently do.
dcreager
added a commit
that referenced
this pull request
Oct 3, 2025
* origin/main: [`flake8-bugbear`] Include certain guaranteed-mutable expressions: tuples, generators, and assignment expressions (`B006`) (#20024) [`flake8-comprehensions`] Clarify fix safety documentation (`C413`) (#20640) [ty] improve base conda distinction from child conda (#20675) [`ruff`] Extend FA102 with listed PEP 585-compatible APIs (#20659) [`ruff`] Handle argfile expansion errors gracefully (#20691) [`flynt`] Fix f-string quoting for mixed quote joiners (`FLY002`) (#20662) [ty] Fix file root matching for `/` [ruff,ty] Enable tracing's `log` feature [`flake8-annotations`] Fix return type annotations to handle shadowed builtin symbols (`ANN201`, `ANN202`, `ANN204`, `ANN205`, `ANN206`) (#20612) Bump 0.13.3 (#20685) Update benchmarking CI for cargo-codspeed v4 (#20686) [ty] Support single-starred argument for overload call (#20223) [ty] `~T` should never be assignable to `T` (#20606) [`pylint`] Clarify fix safety to include left-hand hashability (`PLR6201`) (#20518) [ty] No union with `Unknown` for module-global symbols (#20664) [`ty`] Reject renaming files to start with slash in Playground (#20666) [ty] Enums: allow multiple aliases to point to the same member (#20669)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Quoting from the newly added comment:
Module-level globals can be mutated externally. A
MY_CONSTANT = 1global might be changed to"some string"from code outside of the module that we're looking at, and so from a gradual-guarantee perspective, it makes sense to infer a type ofLiteral[1] | Unknownfor global symbols. This allows the code that does the mutation to type check correctly, and for code that uses the global, it accurately reflects the lack of knowledge about the type.External modifications (or modifications through
globalstatements) that would require a wider type are relatively rare. From a practical perspective, we can therefore achieve a better user experience by trusting the inferred type. Users who need the external mutation to work can always annotate the global with the wider type. And everyone else benefits from more precise type inference.I initially implemented this by applying literal promotion to the type of the unannotated module globals (as suggested in astral-sh/ty#1069), but the ecosystem impact showed a lot of problems (#20643). I fixed/patched some of these problems, but this PR seems like a good first step, and it seems sensible to apply the literal promotion change in a second step that can be evaluated separately.
closes astral-sh/ty#1069
Ecosystem impact
This seems like an (unexpectedly large) net positive with 650 fewer diagnostics overall.. even though this change will certainly catch more true positives.
type-assertion-failurediagnostics, where we were previously used the correct type already, but removing theUnknownnow leads to an "exact" match.unresolved-attributeerrors, most (1365) of which were previouslypossibly-missing-attributeerrors. So they could also be counted as "changed" diagnostics.Literal[True/False] | Unknown, removing theUnknownnow allows us to do reachability analysis on branches that use these constants, and so we get a lot of favorable ecosystem changes because of that.conflicting-argument-formsdiagnostics on calls to the aliasedassert_type, because its type wasUnknown | def …(and the call toUnknown"used" the type form argument in a non type-form way):invalid-argument-typefalse positives, due to missing**kwargssupport (Support starred/splatted/unpacked arguments in function calls ty#247)Typing conformance
+protocols_modules.py:25:1: error[invalid-assignment] Object of type `<module '_protocols_modules1'>` is not assignable to `Options1`This diagnostic should apparently not be there, but it looks like we also fail other tests in that file, so it seems to be a limitation that was previously hidden by
Unknownsomehow.Test Plan
Updated tests and relatively thorough ecosystem analysis.