[ty] No union with `Unknown` for module-global symbols

crates/ty_python_semantic/resources/mdtest/del.md

@@ -108,7 +108,7 @@ def foo():
     global x
     def bar():
         # allowed, refers to `x` in the global scope
-        reveal_type(x)  # revealed: Unknown | Literal[1]
+        reveal_type(x)  # revealed: Literal[1]
     bar()
     del x  # allowed, deletes `x` in the global scope (though we don't track that)
 ```

crates/ty_python_semantic/resources/mdtest/import/conditional.md

@@ -25,8 +25,8 @@ reveal_type(y)
 # error: [possibly-missing-import] "Member `y` of module `maybe_unbound` may be missing"
 from maybe_unbound import x, y
 
-reveal_type(x)  # revealed: Unknown | Literal[3]
+reveal_type(x)  # revealed: Literal[3]
-reveal_type(y)  # revealed: Unknown | Literal[3]
+reveal_type(y)  # revealed: Literal[3]
 ```
 
 ## Maybe unbound annotated
@@ -56,7 +56,7 @@ Importing an annotated name prefers the declared type over the inferred type:
 # error: [possibly-missing-import] "Member `y` of module `maybe_unbound_annotated` may be missing"
 from maybe_unbound_annotated import x, y
 
-reveal_type(x)  # revealed: Unknown | Literal[3]
+reveal_type(x)  # revealed: Literal[3]
 reveal_type(y)  # revealed: int
 ```
 

crates/ty_python_semantic/resources/mdtest/import/dunder_all.md

@@ -783,8 +783,7 @@ class A: ...
 ```py
 from subexporter import *
 
-# TODO: Should we avoid including `Unknown` for this case?
+reveal_type(__all__)  # revealed: list[Unknown | str]
-reveal_type(__all__)  # revealed: Unknown | list[Unknown | str]
 
 __all__.append("B")
 

crates/ty_python_semantic/resources/mdtest/import/module_getattr.md

@@ -40,7 +40,7 @@ def __getattr__(name: str) -> int:
 import mixed_module
 
 # Explicit attribute should take precedence
-reveal_type(mixed_module.explicit_attr)  # revealed: Unknown | Literal["explicit"]
+reveal_type(mixed_module.explicit_attr)  # revealed: Literal["explicit"]
 
 # `__getattr__` should handle unknown attributes
 reveal_type(mixed_module.dynamic_attr)  # revealed: str

crates/ty_python_semantic/resources/mdtest/import/namespace.md

@@ -103,7 +103,7 @@ x = "namespace"
 ```py
 from foo import x
 
-reveal_type(x)  # revealed: Unknown | Literal["module"]
+reveal_type(x)  # revealed: Literal["module"]
 
 import foo.bar  # error: [unresolved-import]
 ```

crates/ty_python_semantic/resources/mdtest/import/star.md

@@ -144,8 +144,8 @@ X = (Y := 3) + 4
 ```py
 from exporter import *
 
-reveal_type(X)  # revealed: Unknown | Literal[7]
+reveal_type(X)  # revealed: Literal[7]
-reveal_type(Y)  # revealed: Unknown | Literal[3]
+reveal_type(Y)  # revealed: Literal[3]
 ```
 
 ### Global-scope symbols defined in many other ways
@@ -781,9 +781,9 @@ else:
 from exporter import *
 
 # error: [possibly-unresolved-reference]
-reveal_type(A)  # revealed: Unknown | Literal[1]
+reveal_type(A)  # revealed: Literal[1]
 
-reveal_type(B)  # revealed: Unknown | Literal[2, 3]
+reveal_type(B)  # revealed: Literal[2, 3]
 ```
 
 ### Reachability constraints in the importing module
@@ -804,7 +804,7 @@ if coinflip():
     from exporter import *
 
 # error: [possibly-unresolved-reference]
-reveal_type(A)  # revealed: Unknown | Literal[1]
+reveal_type(A)  # revealed: Literal[1]
 ```
 
 ### Reachability constraints in the exporting module *and* the importing module

crates/ty_python_semantic/resources/mdtest/narrow/assignment.md

@@ -34,7 +34,7 @@ class _:
 [reveal_type(a.z) for _ in range(1)]  # revealed: Literal[0]
 
 def _():
-    reveal_type(a.x)  # revealed: Unknown | int | None
+    reveal_type(a.x)  # revealed: int | None
     reveal_type(a.y)  # revealed: Unknown | None
     reveal_type(a.z)  # revealed: Unknown | None
 
@@ -75,7 +75,7 @@ class _:
 
     if cond():
         a = A()
-    reveal_type(a.x)  # revealed: int | None | Unknown
+    reveal_type(a.x)  # revealed: int | None
     reveal_type(a.y)  # revealed: Unknown | None
     reveal_type(a.z)  # revealed: Unknown | None
 
@@ -295,10 +295,10 @@ class C:
 
 def _():
     # error: [possibly-missing-attribute]
-    reveal_type(b.a.x[0])  # revealed: Unknown | int | None
+    reveal_type(b.a.x[0])  # revealed: int | None
     # error: [possibly-missing-attribute]
-    reveal_type(b.a.x)  # revealed: Unknown | list[int | None]
+    reveal_type(b.a.x)  # revealed: list[int | None]
-    reveal_type(b.a)  # revealed: Unknown | A | None
+    reveal_type(b.a)  # revealed: A | None
 ```
 
 ## Invalid assignments are not used for narrowing

crates/ty_python_semantic/resources/mdtest/narrow/complex_target.md

@@ -167,11 +167,11 @@ if c.x is not None:
 
 if c.x is not None:
     def _():
-        reveal_type(c.x)  # revealed: Unknown | int | None
+        reveal_type(c.x)  # revealed: int | None
 
 def _():
     if c.x is not None:
-        reveal_type(c.x)  # revealed: (Unknown & ~None) | int
+        reveal_type(c.x)  # revealed: int
 ```
 
 ## Subscript narrowing

crates/ty_python_semantic/resources/mdtest/narrow/conditionals/nested.md

@@ -86,7 +86,7 @@ class B:
     reveal_type(a.x)  # revealed: Literal["a"]
 
 def f():
-    reveal_type(a.x)  # revealed: Unknown | str | None
+    reveal_type(a.x)  # revealed: str | None
 
 [reveal_type(a.x) for _ in range(1)]  # revealed: Literal["a"]
 
@@ -96,7 +96,7 @@ class C:
     reveal_type(a.x)  # revealed: str | None
 
 def g():
-    reveal_type(a.x)  # revealed: Unknown | str | None
+    reveal_type(a.x)  # revealed: str | None
 
 [reveal_type(a.x) for _ in range(1)]  # revealed: str | None
 
@@ -109,7 +109,7 @@ class D:
     reveal_type(a.x)  # revealed: Literal["a"]
 
 def h():
-    reveal_type(a.x)  # revealed: Unknown | str | None
+    reveal_type(a.x)  # revealed: str | None
 
 # TODO: should be `str | None`
 [reveal_type(a.x) for _ in range(1)]  # revealed: Literal["a"]
@@ -190,7 +190,7 @@ def f(x: str | None):
             reveal_type(g)  # revealed: str
 
         if a.x is not None:
-            reveal_type(a.x)  # revealed: (Unknown & ~None) | str
+            reveal_type(a.x)  # revealed: str
 
         if l[0] is not None:
             reveal_type(l[0])  # revealed: str
@@ -206,7 +206,7 @@ def f(x: str | None):
             reveal_type(g)  # revealed: str
 
         if a.x is not None:
-            reveal_type(a.x)  # revealed: (Unknown & ~None) | str
+            reveal_type(a.x)  # revealed: str
 
         if l[0] is not None:
             reveal_type(l[0])  # revealed: str
@@ -382,12 +382,12 @@ def f():
     if a.x is not None:
         def _():
             # Lazy nested scope narrowing is not performed on attributes/subscripts because it's difficult to track their changes.
-            reveal_type(a.x)  # revealed: Unknown | str | None
+            reveal_type(a.x)  # revealed: str | None
 
         class D:
-            reveal_type(a.x)  # revealed: (Unknown & ~None) | str
+            reveal_type(a.x)  # revealed: str
 
-        [reveal_type(a.x) for _ in range(1)]  # revealed: (Unknown & ~None) | str
+        [reveal_type(a.x) for _ in range(1)]  # revealed: str
 
     if l[0] is not None:
         def _():
@@ -473,11 +473,11 @@ def f():
         if a.x is not None:
             def _():
                 if a.x != 1:
-                    reveal_type(a.x)  # revealed: (Unknown & ~Literal[1]) | str | None
+                    reveal_type(a.x)  # revealed: str | None
 
             class D:
                 if a.x != 1:
-                    reveal_type(a.x)  # revealed: (Unknown & ~Literal[1] & ~None) | str
+                    reveal_type(a.x)  # revealed: str
 
         if l[0] is not None:
             def _():

crates/ty_python_semantic/resources/mdtest/public_types.md

@@ -263,7 +263,7 @@ if flag():
     x = 1
 
     def f() -> None:
-        reveal_type(x)  # revealed: Unknown | Literal[1, 2]
+        reveal_type(x)  # revealed: Literal[1, 2]
     # Function only used inside this branch
     f()
 

crates/ty_python_semantic/resources/mdtest/scopes/builtin.md

@@ -29,8 +29,8 @@ if flag():
     chr: int = 1
 
 def _():
-    # TODO: Should ideally be `Unknown | Literal[1] | (def abs(x: SupportsAbs[_T], /) -> _T)`
+    # TODO: Should ideally be `Literal[1] | (def abs(x: SupportsAbs[_T], /) -> _T)`
-    reveal_type(abs)  # revealed: Unknown | Literal[1]
+    reveal_type(abs)  # revealed: Literal[1]
     # TODO: Should ideally be `int | (def chr(i: SupportsIndex, /) -> str)`
     reveal_type(chr)  # revealed: int
 ```

crates/ty_python_semantic/resources/mdtest/scopes/eager.md

@@ -12,7 +12,7 @@ Function definitions are evaluated lazily.
 x = 1
 
 def f():
-    reveal_type(x)  # revealed: Unknown | Literal[1, 2]
+    reveal_type(x)  # revealed: Literal[1, 2]
 
 x = 2
 ```
@@ -283,7 +283,7 @@ x = 1
 
 def _():
     class C:
-        # revealed: Unknown | Literal[1]
+        # revealed: Literal[1]
         [reveal_type(x) for _ in [1]]
         x = 2
 ```
@@ -389,7 +389,7 @@ x = int
 class C:
     var: ClassVar[x]
 
-reveal_type(C.var)  # revealed: Unknown | int | str
+reveal_type(C.var)  # revealed: int | str
 
 x = str
 ```

crates/ty_python_semantic/resources/mdtest/scopes/global.md

@@ -8,7 +8,7 @@ A name reference to a never-defined symbol in a function is implicitly a global
 x = 1
 
 def f():
-    reveal_type(x)  # revealed: Unknown | Literal[1]
+    reveal_type(x)  # revealed: Literal[1]
 ```
 
 ## Explicit global in function
@@ -18,7 +18,7 @@ x = 1
 
 def f():
     global x
-    reveal_type(x)  # revealed: Unknown | Literal[1]
+    reveal_type(x)  # revealed: Literal[1]
 ```
 
 ## Unassignable type in function
@@ -201,7 +201,7 @@ x = 42
 
 def f():
     global x
-    reveal_type(x)  # revealed: Unknown | Literal[42]
+    reveal_type(x)  # revealed: Literal[42]
     x = "56"
     reveal_type(x)  # revealed: Literal["56"]
 ```

crates/ty_python_semantic/resources/mdtest/scopes/moduletype_attrs.md

@@ -73,10 +73,10 @@ __spec__ = 42  # error: [invalid-assignment] "Object of type `Literal[42]` is no
 ```py
 import module
 
-reveal_type(module.__file__)  # revealed: Unknown | None
+reveal_type(module.__file__)  # revealed: None
 reveal_type(module.__path__)  # revealed: list[str]
 reveal_type(module.__doc__)  # revealed: Unknown
-reveal_type(module.__spec__)  # revealed: Unknown | ModuleSpec | None
+reveal_type(module.__spec__)  # revealed: ModuleSpec | None
 # error: [unresolved-attribute]
 reveal_type(module.__warningregistry__)  # revealed: Unknown
 

crates/ty_python_semantic/src/place.rs

@@ -810,7 +810,7 @@ fn place_by_id<'db>(
             // modified externally, but those changes do not take effect. We therefore issue
             // a diagnostic if we see it being modified externally. In type inference, we
             // can assign a "narrow" type to it even if it is not *declared*. This means, we
-            // do not have to call [`widen_type_for_undeclared_public_symbol`].
+            // do not have to union with `Unknown`.
             //
             // `TYPE_CHECKING` is a special variable that should only be assigned `False`
             // at runtime, but is always considered `True` in type checking.
@@ -822,18 +822,37 @@ fn place_by_id<'db>(
                 )
             });
 
-            if scope.file(db).is_stub(db) || scope.scope(db).visibility().is_private() {
+            // Module-level globals can be mutated externally. A `MY_CONSTANT = 1` global might
-                // We generally trust module-level undeclared places in stubs and do not union
+            // be changed to `"some string"` from code outside of the module that we're looking
-                // with `Unknown`. If we don't do this, simple aliases like `IOError = OSError` in
+            // at, and so from a gradual-guarantee perspective, it makes sense to infer a type
-                // stubs would result in `IOError` being a union of `OSError` and `Unknown`, which
+            // of `Literal[1] | Unknown` for global symbols. This allows the code that does the
-                // leads to all sorts of downstream problems. Similarly, type variables are often
+            // mutation to type check correctly, and for code that uses the global, it accurately
-                // defined as `_T = TypeVar("_T")`, without being declared.
+            // reflects the lack of knowledge about the type.
-                // Also, if the scope is private, such as a function scope,
+            //
-                // meaning that the place cannot be rewritten from elsewhere, we do not union with `Unknown`.
+            // External modifications (or modifications through `global` statements) 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.
+            let is_module_global = scope.node(db).scope_kind().is_module();
 
+            // If the visibility of the scope is private (like for a function scope), we also do
+            // not union with `Unknown`, because the symbol cannot be modified externally.
+            let scope_has_private_visibility = scope.scope(db).visibility().is_private();
+
+            // We generally trust undeclared places in stubs and do not union with `Unknown`.
+            let in_stub_file = scope.file(db).is_stub(db);
+
+            if is_considered_non_modifiable
+                || is_module_global
+                || scope_has_private_visibility
+                || in_stub_file
+            {
                 inferred.into()
             } else {
-                widen_type_for_undeclared_public_symbol(db, inferred, is_considered_non_modifiable)
+                // Widen the inferred type of undeclared public symbols by unioning with `Unknown`
+                inferred
+                    .map_type(|ty| UnionType::from_elements(db, [Type::unknown(), ty]))
                     .into()
             }
         }
@@ -1585,29 +1604,6 @@ pub(crate) enum BoundnessAnalysis {
     BasedOnUnboundVisibility,
 }
 
-/// Computes a possibly-widened type `Unknown | T_inferred` from the inferred type `T_inferred`
-/// of a symbol, unless the type is a known-instance type (e.g. `typing.Any`) or the symbol is
-/// considered non-modifiable (e.g. when the symbol is `@Final`). We need this for public uses
-/// of symbols that have no declared type.
-fn widen_type_for_undeclared_public_symbol<'db>(
-    db: &'db dyn Db,
-    inferred: Place<'db>,
-    is_considered_non_modifiable: bool,
-) -> Place<'db> {
-    // We special-case known-instance types here since symbols like `typing.Any` are typically
-    // not declared in the stubs (e.g. `Any = object()`), but we still want to treat them as
-    // such.
-    let is_known_instance = inferred
-        .ignore_possibly_unbound()
-        .is_some_and(|ty| matches!(ty, Type::SpecialForm(_) | Type::KnownInstance(_)));
-
-    if is_considered_non_modifiable || is_known_instance {
-        inferred
-    } else {
-        inferred.map_type(|ty| UnionType::from_elements(db, [Type::unknown(), ty]))
-    }
-}
-
 #[cfg(test)]
 mod tests {
     use super::*;

crates/ty_python_semantic/src/types/infer/tests.rs

@@ -5,7 +5,7 @@ use crate::place::{ConsideredDefinitions, Place, global_symbol};
 use crate::semantic_index::definition::Definition;
 use crate::semantic_index::scope::FileScopeId;
 use crate::semantic_index::{global_scope, place_table, semantic_index, use_def_map};
-use crate::types::{KnownClass, KnownInstanceType, UnionType, check_types};
+use crate::types::{KnownClass, KnownInstanceType, check_types};
 use ruff_db::diagnostic::Diagnostic;
 use ruff_db::files::{File, system_path_to_file};
 use ruff_db::system::DbWithWritableSystem as _;
@@ -424,7 +424,7 @@ fn dependency_implicit_instance_attribute() -> anyhow::Result<()> {
 
     let file_main = system_path_to_file(&db, "/src/main.py").unwrap();
     let attr_ty = global_symbol(&db, file_main, "x").place.expect_type();
-    assert_eq!(attr_ty.display(&db).to_string(), "Unknown | int | None");
+    assert_eq!(attr_ty.display(&db).to_string(), "int | None");
 
     // Change the type of `attr` to `str | None`; this should trigger the type of `x` to be re-inferred
     db.write_dedented(
@@ -439,7 +439,7 @@ fn dependency_implicit_instance_attribute() -> anyhow::Result<()> {
     let events = {
         db.clear_salsa_events();
         let attr_ty = global_symbol(&db, file_main, "x").place.expect_type();
-        assert_eq!(attr_ty.display(&db).to_string(), "Unknown | str | None");
+        assert_eq!(attr_ty.display(&db).to_string(), "str | None");
         db.take_salsa_events()
     };
     assert_function_query_was_run(
@@ -463,7 +463,7 @@ fn dependency_implicit_instance_attribute() -> anyhow::Result<()> {
     let events = {
         db.clear_salsa_events();
         let attr_ty = global_symbol(&db, file_main, "x").place.expect_type();
-        assert_eq!(attr_ty.display(&db).to_string(), "Unknown | str | None");
+        assert_eq!(attr_ty.display(&db).to_string(), "str | None");
         db.take_salsa_events()
     };
 
@@ -514,7 +514,7 @@ fn dependency_own_instance_member() -> anyhow::Result<()> {
 
     let file_main = system_path_to_file(&db, "/src/main.py").unwrap();
     let attr_ty = global_symbol(&db, file_main, "x").place.expect_type();
-    assert_eq!(attr_ty.display(&db).to_string(), "Unknown | int | None");
+    assert_eq!(attr_ty.display(&db).to_string(), "int | None");
 
     // Change the type of `attr` to `str | None`; this should trigger the type of `x` to be re-inferred
     db.write_dedented(
@@ -531,7 +531,7 @@ fn dependency_own_instance_member() -> anyhow::Result<()> {
     let events = {
         db.clear_salsa_events();
         let attr_ty = global_symbol(&db, file_main, "x").place.expect_type();
-        assert_eq!(attr_ty.display(&db).to_string(), "Unknown | str | None");
+        assert_eq!(attr_ty.display(&db).to_string(), "str | None");
         db.take_salsa_events()
     };
     assert_function_query_was_run(
@@ -557,7 +557,7 @@ fn dependency_own_instance_member() -> anyhow::Result<()> {
     let events = {
         db.clear_salsa_events();
         let attr_ty = global_symbol(&db, file_main, "x").place.expect_type();
-        assert_eq!(attr_ty.display(&db).to_string(), "Unknown | str | None");
+        assert_eq!(attr_ty.display(&db).to_string(), "str | None");
         db.take_salsa_events()
     };
 
@@ -609,7 +609,7 @@ fn dependency_implicit_class_member() -> anyhow::Result<()> {
 
     let file_main = system_path_to_file(&db, "/src/main.py").unwrap();
     let attr_ty = global_symbol(&db, file_main, "x").place.expect_type();
-    assert_eq!(attr_ty.display(&db).to_string(), "Unknown | int");
+    assert_eq!(attr_ty.display(&db).to_string(), "int");
 
     // Change the type of `class_attr` to `str`; this should trigger the type of `x` to be re-inferred
     db.write_dedented(
@@ -628,7 +628,7 @@ fn dependency_implicit_class_member() -> anyhow::Result<()> {
     let events = {
         db.clear_salsa_events();
         let attr_ty = global_symbol(&db, file_main, "x").place.expect_type();
-        assert_eq!(attr_ty.display(&db).to_string(), "Unknown | str");
+        assert_eq!(attr_ty.display(&db).to_string(), "str");
         db.take_salsa_events()
     };
     assert_function_query_was_run(
@@ -656,7 +656,7 @@ fn dependency_implicit_class_member() -> anyhow::Result<()> {
     let events = {
         db.clear_salsa_events();
         let attr_ty = global_symbol(&db, file_main, "x").place.expect_type();
-        assert_eq!(attr_ty.display(&db).to_string(), "Unknown | str");
+        assert_eq!(attr_ty.display(&db).to_string(), "str");
         db.take_salsa_events()
     };
 
@@ -695,10 +695,7 @@ fn call_type_doesnt_rerun_when_only_callee_changed() -> anyhow::Result<()> {
     let bar = system_path_to_file(&db, "src/bar.py")?;
     let a = global_symbol(&db, bar, "a").place;
 
-    assert_eq!(
+    assert_eq!(a.expect_type(), KnownClass::Int.to_instance(&db));
-        a.expect_type(),
-        UnionType::from_elements(&db, [Type::unknown(), KnownClass::Int.to_instance(&db)])
-    );
     let events = db.take_salsa_events();
 
     let module = parsed_module(&db, bar).load(&db);
@@ -726,10 +723,7 @@ fn call_type_doesnt_rerun_when_only_callee_changed() -> anyhow::Result<()> {
 
     let a = global_symbol(&db, bar, "a").place;
 
-    assert_eq!(
+    assert_eq!(a.expect_type(), KnownClass::Int.to_instance(&db));
-        a.expect_type(),
-        UnionType::from_elements(&db, [Type::unknown(), KnownClass::Int.to_instance(&db)])
-    );
     let events = db.take_salsa_events();
 
     let module = parsed_module(&db, bar).load(&db);