diff --git a/crates/ty_python_semantic/resources/mdtest/import/nonstandard_conventions.md b/crates/ty_python_semantic/resources/mdtest/import/nonstandard_conventions.md index e17a026e32..d1dfbbe409 100644 --- a/crates/ty_python_semantic/resources/mdtest/import/nonstandard_conventions.md +++ b/crates/ty_python_semantic/resources/mdtest/import/nonstandard_conventions.md @@ -647,8 +647,8 @@ reveal_type(mypackage.imported.X) # revealed: int ## `from` Import of Other Package's Submodule -`from mypackage import submodule` from outside the package is not modeled as a side-effect on -`mypackage`, even in the importing file (this could be changed!). +`from mypackage import submodule` and `from mypackage.submodule import not_a_submodule` from outside +the package are both modeled as a side-effects on `mypackage`. ### In Stub @@ -663,18 +663,28 @@ reveal_type(mypackage.imported.X) # revealed: int X: int = 42 ``` +`package2/__init__.pyi`: + +```pyi +``` + +`package2/submodule.pyi`: + +```pyi +not_a_submodule: int +``` + `main.py`: ```py import mypackage +import package2 from mypackage import imported +from package2.submodule import not_a_submodule reveal_type(imported.X) # revealed: int - -# TODO: this would be nice to support, but it's dangerous with available_submodule_attributes -# for details, see: https://github.com/astral-sh/ty/issues/1488 -# error: [possibly-missing-attribute] "Submodule `imported` may not be available" -reveal_type(mypackage.imported.X) # revealed: Unknown +reveal_type(mypackage.imported.X) # revealed: int +reveal_type(package2.submodule.not_a_submodule) # revealed: int ``` ### In Non-Stub @@ -690,17 +700,28 @@ reveal_type(mypackage.imported.X) # revealed: Unknown X: int = 42 ``` +`package2/__init__.py`: + +```py +``` + +`package2/submodule.py`: + +```py +not_a_submodule: int +``` + `main.py`: ```py import mypackage +import package2 from mypackage import imported +from package2.submodule import not_a_submodule reveal_type(imported.X) # revealed: int - -# TODO: this would be nice to support, as it works at runtime -# error: [possibly-missing-attribute] "Submodule `imported` may not be available" -reveal_type(mypackage.imported.X) # revealed: Unknown +reveal_type(mypackage.imported.X) # revealed: int +reveal_type(package2.submodule.not_a_submodule) # revealed: int ``` ## `from` Import of Sibling Module @@ -859,6 +880,51 @@ from mypackage import funcmod x = funcmod(1) ``` +## A Tale of Two Modules + +This is a nonsensical regression test for some incredibly cursed interaction in `ty` where we get +confused about `mypackage.conflicted`. The worst part is that resolving an import +`from typing import ` is load-bearing, and `typing` seems to be special +here. There is no known reason why `typing` should be special here. + +### In Stub + +`mypackage/__init__.py`: + +```py +from .conflicted.b import x +``` + +`mypackage/conflicted/__init__.py`: + +`mypackage/conflicted/other1/__init__.py`: + +```py +x: int = 1 +``` + +`mypackage/conflicted/b/__init__.py`: + +```py +x: int = 1 +``` + +`mypackage/conflicted/b/c/__init__.py`: + +```py +y: int = 2 +``` + +`main.py`: + +```py +from typing import TYPE_CHECKING +from mypackage.conflicted.other1 import x as x1 +import mypackage.conflicted.b.c + +reveal_type(mypackage.conflicted.b.c.y) # revealed: int +``` + ## Re-export Nameclash Problems In Functions `from` imports in an `__init__.py` at file scope should be visible to functions defined in the file: diff --git a/crates/ty_python_semantic/resources/mdtest/import/tracking.md b/crates/ty_python_semantic/resources/mdtest/import/tracking.md index f31193b762..16c2473a7e 100644 --- a/crates/ty_python_semantic/resources/mdtest/import/tracking.md +++ b/crates/ty_python_semantic/resources/mdtest/import/tracking.md @@ -116,3 +116,43 @@ b = 1 ```py ``` + +## Submodule is loaded in a non-global scope + +We recognise submodules as being available as attributes even if they are loaded in a function +scope. The function might never be executed, which means that the submodule might never be loaded; +however, we prefer to prioritise avoiding false positives over catching all possible errors here. + +`a/b.py`: + +```py +``` + +`a/c.py`: + +```py +d = 42 +``` + +`a/e/f.py`: + +```py +``` + +`main.py`: + +```py +import a + +def f(): + import a.b + from a.c import d + from a.e import f + +f() + +reveal_type(a.b) # revealed: +reveal_type(a.c) # revealed: +reveal_type(a.e) # revealed: +reveal_type(a.e.f) # revealed: +``` diff --git a/crates/ty_python_semantic/src/semantic_index.rs b/crates/ty_python_semantic/src/semantic_index.rs index 43a0a3d0af..c29e47b0d0 100644 --- a/crates/ty_python_semantic/src/semantic_index.rs +++ b/crates/ty_python_semantic/src/semantic_index.rs @@ -79,7 +79,10 @@ pub(crate) fn place_table<'db>(db: &'db dyn Db, scope: ScopeId<'db>) -> Arc(db: &'db dyn Db, file: File) -> Arc> { +pub(crate) fn imported_modules<'db>( + db: &'db dyn Db, + file: File, +) -> Arc> { semantic_index(db, file).imported_modules.clone() } @@ -246,7 +249,7 @@ pub(crate) struct SemanticIndex<'db> { ast_ids: IndexVec, /// The set of modules that are imported anywhere within this file. - imported_modules: Arc>, + imported_modules: Arc>, /// Flags about the global scope (code usage impacting inference) has_future_annotations: bool, @@ -588,6 +591,12 @@ impl<'db> SemanticIndex<'db> { } } +#[derive(Debug, Clone, Copy, PartialEq, Eq, get_size2::GetSize)] +pub(crate) enum ImportKind { + Import, + ImportFrom, +} + pub(crate) struct AncestorsIter<'a> { scopes: &'a IndexSlice, next_id: Option, diff --git a/crates/ty_python_semantic/src/semantic_index/builder.rs b/crates/ty_python_semantic/src/semantic_index/builder.rs index 373c80f7ee..06d7887d61 100644 --- a/crates/ty_python_semantic/src/semantic_index/builder.rs +++ b/crates/ty_python_semantic/src/semantic_index/builder.rs @@ -47,7 +47,7 @@ use crate::semantic_index::symbol::{ScopedSymbolId, Symbol}; use crate::semantic_index::use_def::{ EnclosingSnapshotKey, FlowSnapshot, ScopedEnclosingSnapshotId, UseDefMapBuilder, }; -use crate::semantic_index::{ExpressionsScopeMap, SemanticIndex, VisibleAncestorsIter}; +use crate::semantic_index::{ExpressionsScopeMap, ImportKind, SemanticIndex, VisibleAncestorsIter}; use crate::semantic_model::HasTrackedScope; use crate::unpack::{EvaluationMode, Unpack, UnpackKind, UnpackPosition, UnpackValue}; use crate::{Db, Program}; @@ -110,7 +110,7 @@ pub(super) struct SemanticIndexBuilder<'db, 'ast> { scopes_by_expression: ExpressionsScopeMapBuilder, definitions_by_node: FxHashMap>, expressions_by_node: FxHashMap>, - imported_modules: FxHashSet, + imported_modules: FxHashMap, seen_submodule_imports: FxHashSet, /// Hashset of all [`FileScopeId`]s that correspond to [generator functions]. /// @@ -150,7 +150,7 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> { expressions_by_node: FxHashMap::default(), seen_submodule_imports: FxHashSet::default(), - imported_modules: FxHashSet::default(), + imported_modules: FxHashMap::default(), generator_functions: FxHashSet::default(), enclosing_snapshots: FxHashMap::default(), @@ -1474,7 +1474,11 @@ impl<'ast> Visitor<'ast> for SemanticIndexBuilder<'_, 'ast> { // Mark the imported module, and all of its parents, as being imported in this // file. if let Some(module_name) = ModuleName::new(&alias.name) { - self.imported_modules.extend(module_name.ancestors()); + self.imported_modules.extend( + module_name + .ancestors() + .zip(std::iter::repeat(ImportKind::Import)), + ); } let (symbol_name, is_reexported) = if let Some(asname) = &alias.asname { @@ -1517,33 +1521,54 @@ impl<'ast> Visitor<'ast> for SemanticIndexBuilder<'_, 'ast> { // that `x` can be freely overwritten, and that we don't assume that an import // in one function is visible in another function. let mut is_self_import = false; - if self.file.is_package(self.db) - && let Ok(module_name) = ModuleName::from_identifier_parts( - self.db, - self.file, - node.module.as_deref(), - node.level, - ) - && let Ok(thispackage) = ModuleName::package_for_file(self.db, self.file) - { + let is_package = self.file.is_package(self.db); + let this_package = ModuleName::package_for_file(self.db, self.file); + + if let Ok(module_name) = ModuleName::from_identifier_parts( + self.db, + self.file, + node.module.as_deref(), + node.level, + ) { // Record whether this is equivalent to `from . import ...` - is_self_import = module_name == thispackage; + if is_package && let Ok(thispackage) = this_package.as_ref() { + is_self_import = &module_name == thispackage; + } - if node.module.is_some() - && let Some(relative_submodule) = module_name.relative_to(&thispackage) - && let Some(direct_submodule) = relative_submodule.components().next() - && !self.seen_submodule_imports.contains(direct_submodule) - && self.current_scope().is_global() - { - self.seen_submodule_imports - .insert(direct_submodule.to_owned()); + if node.module.is_some() { + if is_package + && let Ok(thispackage) = this_package + && self.current_scope().is_global() + && let Some(relative_submodule) = module_name.relative_to(&thispackage) + && let Some(direct_submodule) = relative_submodule.components().next() + && !self.seen_submodule_imports.contains(direct_submodule) + { + self.seen_submodule_imports + .insert(direct_submodule.to_owned()); - let direct_submodule_name = Name::new(direct_submodule); - let symbol = self.add_symbol(direct_submodule_name); - self.add_definition( - symbol.into(), - ImportFromSubmoduleDefinitionNodeRef { node }, - ); + let direct_submodule_name = Name::new(direct_submodule); + let symbol = self.add_symbol(direct_submodule_name); + self.add_definition( + symbol.into(), + ImportFromSubmoduleDefinitionNodeRef { node }, + ); + } else { + for name in module_name.ancestors() { + self.imported_modules + .entry(name) + .or_insert(ImportKind::ImportFrom); + } + for name in &node.names { + let Some(relative_name) = ModuleName::new(&name.name) else { + continue; + }; + let mut full_name = module_name.clone(); + full_name.extend(&relative_name); + self.imported_modules + .entry(full_name) + .or_insert(ImportKind::ImportFrom); + } + } } } diff --git a/crates/ty_python_semantic/src/types.rs b/crates/ty_python_semantic/src/types.rs index e4a0228cd4..a1c32c2b79 100644 --- a/crates/ty_python_semantic/src/types.rs +++ b/crates/ty_python_semantic/src/types.rs @@ -42,7 +42,7 @@ use crate::place::{ use crate::semantic_index::definition::{Definition, DefinitionKind}; use crate::semantic_index::place::ScopedPlaceId; use crate::semantic_index::scope::ScopeId; -use crate::semantic_index::{imported_modules, place_table, semantic_index}; +use crate::semantic_index::{ImportKind, imported_modules, place_table, semantic_index}; use crate::suppression::check_suppressions; use crate::types::bound_super::BoundSuperType; use crate::types::builder::RecursivelyDefined; @@ -13371,12 +13371,18 @@ impl<'db> ModuleLiteralType<'db> { /// /// We instead prefer handling most other import effects as definitions in the scope of /// the current file (i.e. [`crate::semantic_index::definition::ImportFromDefinitionNodeRef`]). - fn available_submodule_attributes(&self, db: &'db dyn Db) -> impl Iterator { + fn available_submodule_attributes( + &self, + db: &'db dyn Db, + ) -> impl Iterator { self.importing_file(db) .into_iter() .flat_map(|file| imported_modules(db, file)) - .filter_map(|submodule_name| submodule_name.relative_to(self.module(db).name(db))) - .filter_map(|relative_submodule| relative_submodule.components().next().map(Name::from)) + .filter_map(|(submodule_name, kind)| { + let relative_name = submodule_name.relative_to(self.module(db).name(db))?; + let available_attribute = relative_name.components().next()?; + Some((Name::from(available_attribute), *kind)) + }) } fn resolve_submodule(self, db: &'db dyn Db, name: &str) -> Option> { @@ -13420,33 +13426,55 @@ impl<'db> ModuleLiteralType<'db> { .member(db, "__dict__"); } - // If the file that originally imported the module has also imported a submodule - // named `name`, then the result is (usually) that submodule, even if the module - // also defines a (non-module) symbol with that name. - // - // Note that technically, either the submodule or the non-module symbol could take - // priority, depending on the ordering of when the submodule is loaded relative to - // the parent module's `__init__.py` file being evaluated. That said, we have - // chosen to always have the submodule take priority. (This matches pyright's - // current behavior, but is the opposite of mypy's current behavior.) - if self.available_submodule_attributes(db).contains(name) { - if let Some(submodule) = self.resolve_submodule(db, name) { - return Place::bound(submodule).into(); - } + let mut submodule_type = None; + + let available_submodule_kind = self + .available_submodule_attributes(db) + .find_map(|(attr, kind)| (attr == name).then_some(kind)); + + if available_submodule_kind.is_some() { + submodule_type = self.resolve_submodule(db, name); + } + + // if we're in a module `foo` and `foo` contains `import a.b`, + // and the package `a` has a submodule `b`, we assume that the + // attribute access `a.b` inside `foo` will resolve to the submodule + // `a.b` *even if* `a/__init__.py` also defines a symbol `b` (e.g. `b = 42`). + // This is a heuristic, but it's almost certainly what will actually happen + // at runtime. However, if `foo` only contains `from a.b import , + // we prioritise the `b` attribute in `a/__init__.py` over the submodule `a.b`. + if available_submodule_kind == Some(ImportKind::Import) + && let Some(submodule) = submodule_type + { + return Place::bound(submodule).into(); } let place_and_qualifiers = self .module(db) .file(db) - .map(|file| imported_symbol(db, file, name, None)) + .map(|file| { + imported_symbol(db, file, name, None).map_type(|ty| { + if let Some(importing) = self.importing_file(db) + && let Type::ModuleLiteral(module) = ty + { + Type::module_literal(db, importing, module.module(db)) + } else { + ty + } + }) + }) .unwrap_or_default(); - // If the normal lookup failed, try to call the module's `__getattr__` function - if place_and_qualifiers.place.is_undefined() { - return self.try_module_getattr(db, name); + if !place_and_qualifiers.is_undefined() { + return place_and_qualifiers; } - place_and_qualifiers + if let Some(submodule) = submodule_type { + return Place::bound(submodule).into(); + } + + // If the normal lookup failed, try to call the module's `__getattr__` function + self.try_module_getattr(db, name) } } diff --git a/crates/ty_python_semantic/src/types/list_members.rs b/crates/ty_python_semantic/src/types/list_members.rs index 4e4a32c294..5c538a5631 100644 --- a/crates/ty_python_semantic/src/types/list_members.rs +++ b/crates/ty_python_semantic/src/types/list_members.rs @@ -371,7 +371,7 @@ impl<'db> AllMembers<'db> { self.members .extend(literal.available_submodule_attributes(db).filter_map( - |submodule_name| { + |(submodule_name, _)| { let ty = literal.resolve_submodule(db, &submodule_name)?; let name = submodule_name.clone(); Some(Member { name, ty }) diff --git a/playground/package-lock.json b/playground/package-lock.json index 3de5b851bf..eeea25fa5a 100644 --- a/playground/package-lock.json +++ b/playground/package-lock.json @@ -1780,7 +1780,6 @@ "integrity": "sha512-AwAfQ2Wa5bCx9WP8nZL2uMZWod7J7/JSplxbTmBQ5ms6QpqNYm672H0Vu9ZVKVngQ+ii4R/byguVEUZQyeg44g==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "csstype": "^3.0.2" } @@ -1841,7 +1840,6 @@ "integrity": "sha512-Zhy8HCvBUEfBECzIl1PKqF4p11+d0aUJS1GeUiuqK9WmOug8YCmC4h4bjyBvMyAMI9sbRczmrYL5lKg/YMbrcQ==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@typescript-eslint/scope-manager": "8.38.0", "@typescript-eslint/types": "8.38.0", @@ -2087,7 +2085,6 @@ "integrity": "sha512-NZyJarBfL7nWwIq+FDL6Zp/yHEhePMNnnJ0y3qfieCrmNvYct8uvtiV41UvlSe6apAfk0fY1FbWx+NwfmpvtTg==", "dev": true, "license": "MIT", - "peer": true, "bin": { "acorn": "bin/acorn" }, @@ -3004,7 +3001,6 @@ "integrity": "sha512-LSehfdpgMeWcTZkWZVIJl+tkZ2nuSkyyB9C27MZqFWXuph7DvaowgcTvKqxvpLW1JZIk8PN7hFY3Rj9LQ7m7lg==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@eslint-community/eslint-utils": "^4.2.0", "@eslint-community/regexpp": "^4.12.1", @@ -4863,7 +4859,6 @@ "resolved": "https://registry.npmjs.org/monaco-editor/-/monaco-editor-0.54.0.tgz", "integrity": "sha512-hx45SEUoLatgWxHKCmlLJH81xBo0uXP4sRkESUpmDQevfi+e7K1VuiSprK6UpQ8u4zOcKNiH0pMvHvlMWA/4cw==", "license": "MIT", - "peer": true, "dependencies": { "dompurify": "3.1.7", "marked": "14.0.0" @@ -5322,7 +5317,6 @@ "resolved": "https://registry.npmjs.org/react/-/react-19.1.1.tgz", "integrity": "sha512-w8nqGImo45dmMIfljjMwOGtbmC/mk4CMYhWIicdSflH91J9TyCyczcPFXJzrZ/ZXcgGRFeP6BU0BEJTw6tZdfQ==", "license": "MIT", - "peer": true, "engines": { "node": ">=0.10.0" } @@ -5332,7 +5326,6 @@ "resolved": "https://registry.npmjs.org/react-dom/-/react-dom-19.1.1.tgz", "integrity": "sha512-Dlq/5LAZgF0Gaz6yiqZCf6VCcZs1ghAJyrsu84Q/GT0gV+mCxbfmKNoGRKBYMJ8IEdGPqu49YWXD02GCknEDkw==", "license": "MIT", - "peer": true, "dependencies": { "scheduler": "^0.26.0" }, @@ -6030,7 +6023,6 @@ "integrity": "sha512-M7BAV6Rlcy5u+m6oPhAPFgJTzAioX/6B0DxyvDlo9l8+T3nLKbrczg2WLUyzd45L8RqfUMyGPzekbMvX2Ldkwg==", "dev": true, "license": "MIT", - "peer": true, "engines": { "node": ">=12" }, @@ -6182,7 +6174,6 @@ "integrity": "sha512-p1diW6TqL9L07nNxvRMM7hMMw4c5XOo/1ibL4aAIGmSAt9slTE1Xgw5KWuof2uTOvCg9BY7ZRi+GaF+7sfgPeQ==", "dev": true, "license": "Apache-2.0", - "peer": true, "bin": { "tsc": "bin/tsc", "tsserver": "bin/tsserver" @@ -6260,7 +6251,6 @@ "integrity": "sha512-cJBdq0/u+8rgstg9t7UkBilf8ipLmeXJO30NxD5HAHOivnj10ocV8YtR/XBvd2wQpN3TmcaxNKaHX3tN7o5F5A==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "esbuild": "^0.25.0", "fdir": "^6.4.6", @@ -6374,7 +6364,6 @@ "integrity": "sha512-5gTmgEY/sqK6gFXLIsQNH19lWb4ebPDLA4SdLP7dsWkIXHWlG66oPuVvXSGFPppYZz8ZDZq0dYYrbHfBCVUb1Q==", "dev": true, "license": "MIT", - "peer": true, "engines": { "node": ">=12" },