diff --git a/crates/basilisk-checker/src/imports/apply.rs b/crates/basilisk-checker/src/imports/apply.rs index e70cdfc0..29608649 100644 --- a/crates/basilisk-checker/src/imports/apply.rs +++ b/crates/basilisk-checker/src/imports/apply.rs @@ -7,7 +7,9 @@ use basilisk_resolver::scope::{ImportKind, ImportedModuleApi, PackageDepKind}; use super::builtins::populate_builtin_classes; use super::fs_cache::FsCache; -use super::resolve::{classify_unresolved, resolve_module_with_importer_cached}; +use super::resolve::{ + classify_unresolved, resolve_module_with_importer_cached, resolve_relative_import_cached, +}; use super::ImportSearchPaths; /// Resolve every import in a single module against the search paths, in place. @@ -36,21 +38,32 @@ pub fn resolve_module_imports( let mut captured: Vec<(String, ImportedModuleApi)> = Vec::new(); for import in &mut resolved.imports { - let result = match import.kind { - ImportKind::Plain | ImportKind::From | ImportKind::Star => { - resolve_module_with_importer_cached( - &import.module, - search_paths, - Some(&importing_file), - &fs, - ) - } + // A `from`-import with leading dots resolves against the importing + // file's package, never the search paths (GitHub #369). The absolute + // path's importer-directory fallback only ever reached single-dot + // siblings by accident; parent packages need the level walk. + let result = if import.relative_level > 0 { + resolve_relative_import_cached( + &importing_file, + import.relative_level, + &import.module, + &fs, + ) + } else { + resolve_module_with_importer_cached( + &import.module, + search_paths, + Some(&importing_file), + &fs, + ) }; if let Some(r) = result { import.resolution = r.resolution; import.resolved_path = Some(r.path); - } else { + } else if import.relative_level == 0 { // Classify why the import is unresolved for actionable diagnostics. + // Relative imports are exempt: they name a workspace-local package, + // so the registry's "not installed" classification would mislead. import.unresolved_reason = Some(classify_unresolved(&import.module, search_paths)); } @@ -69,11 +82,15 @@ pub fn resolve_module_imports( captured.push((binding, api)); } - import.stub_distribution = - stub_distribution(&import.module, search_paths, Some(importing_file.as_path())); + // A relative import names a workspace-local module; it can never be a + // typeshed distribution or a uv-managed package. + if import.relative_level == 0 { + import.stub_distribution = + stub_distribution(&import.module, search_paths, Some(importing_file.as_path())); - // Annotate with package metadata from the uv registry. - enrich_package_metadata(import, search_paths); + // Annotate with package metadata from the uv registry. + enrich_package_metadata(import, search_paths); + } } for (binding, api) in captured { diff --git a/crates/basilisk-checker/src/imports/resolve.rs b/crates/basilisk-checker/src/imports/resolve.rs index 3c2ee435..d9da4d30 100644 --- a/crates/basilisk-checker/src/imports/resolve.rs +++ b/crates/basilisk-checker/src/imports/resolve.rs @@ -372,15 +372,26 @@ pub fn resolve_relative_import( module_name: &str, _search_paths: &ImportSearchPaths, ) -> Option { - let fs = FsCache::new(); + resolve_relative_import_cached(importing_file, level, module_name, &FsCache::new()) +} + +/// [`resolve_relative_import`] with a caller-supplied [`FsCache`]; the +/// per-module import loop in [`super::resolve_module_imports`] uses this so +/// every import of a file shares one set of directory listings. +pub(crate) fn resolve_relative_import_cached( + importing_file: &Path, + level: u32, + module_name: &str, + fs: &FsCache, +) -> Option { let mut base = importing_file.parent()?.to_path_buf(); for _ in 1..level { base = base.parent()?.to_path_buf(); } if module_name.is_empty() { - return try_resolve_init(&base, &fs); + return try_resolve_init(&base, fs); } - try_resolve_in_dir(module_name, &base, &fs) + try_resolve_in_dir(module_name, &base, fs) } /// Try resolving a dotted module name within a single directory. diff --git a/crates/basilisk-checker/src/rules/imports_unresolved.rs b/crates/basilisk-checker/src/rules/imports_unresolved.rs index 230fcb98..18d8d007 100644 --- a/crates/basilisk-checker/src/rules/imports_unresolved.rs +++ b/crates/basilisk-checker/src/rules/imports_unresolved.rs @@ -141,6 +141,7 @@ mod tests { fn make_import(module: &str, reason: Option) -> ImportInfo { ImportInfo { module: module.to_owned(), + relative_level: 0, names: vec![], span: Span::new(0, 15), name_spans: Vec::new(), diff --git a/crates/basilisk-checker/src/rules/missing_type_stubs/tests.rs b/crates/basilisk-checker/src/rules/missing_type_stubs/tests.rs index bdf4307a..65dea7bb 100644 --- a/crates/basilisk-checker/src/rules/missing_type_stubs/tests.rs +++ b/crates/basilisk-checker/src/rules/missing_type_stubs/tests.rs @@ -36,6 +36,7 @@ fn make_import( ) -> ImportInfo { ImportInfo { module: module.to_owned(), + relative_level: 0, names: vec![], span: Span::new(0, span_end), name_spans: Vec::new(), @@ -230,6 +231,7 @@ fn skips_site_packages_package_with_py_typed_marker() -> Result<(), Box Result<(), Box ImportInfo { ImportInfo { module: module.to_owned(), + relative_level: 0, names: vec![], span: Span::new(0, span_end), name_spans: Vec::new(), diff --git a/crates/basilisk-checker/tests/checker/assignment_compatibility_2_tests.rs b/crates/basilisk-checker/tests/checker/assignment_compatibility_2_tests.rs new file mode 100644 index 00000000..47e3cff7 --- /dev/null +++ b/crates/basilisk-checker/tests/checker/assignment_compatibility_2_tests.rs @@ -0,0 +1,85 @@ +//! Tests for [`assignment_compatibility`] from [CHKARCH-DIAG-TYPESAFETY]. See docs/specs/CHECKER-ARCHITECTURE-SPEC.md#CHKARCH-DIAG-TYPESAFETY +// Edge cases for the enum literal-expansion equivalence of +// [TYPEINF-SUBTYPING-UNION] (`assignment_compatibility/enum_expand.rs`, +// GitHub #374): membership subtleties the base complete/partial-union tests +// in `assignment_compatibility_tests.rs` do not cover — single-member enums, +// `nonmember(...)` attributes, and unions naming a different enum's members. + +use super::common::*; + +#[test] +fn single_member_enum_assigned_to_its_literal_no_diagnostic( +) -> Result<(), Box> { + // A one-member enum's single literal IS the complete union. + let source = r#" +from enum import Enum +from typing import Literal + +class Single(Enum): + ONLY = 1 + +def test(s: Single) -> None: + x: Literal[Single.ONLY] = s +"#; + let diags = run(source)?; + let msgs = messages_for(&diags, "assignment_compatibility"); + assert!( + msgs.is_empty(), + "Literal[Single.ONLY] is the complete union of a one-member enum; \ + should not fire, got: {msgs:?}" + ); + Ok(()) +} + +#[test] +fn nonmember_attribute_does_not_count_toward_completeness() -> Result<(), Box> +{ + // `nonmember(...)` attributes and sunder/dunder names are not enum + // members, so the union of the two real members is still complete. + let source = r#" +from enum import Enum, nonmember +from typing import Literal + +class Answer(Enum): + Yes = 1 + No = 2 + helper = nonmember(3) + +def test(a: Answer) -> None: + x: Literal[Answer.Yes, Answer.No] = a +"#; + let diags = run(source)?; + let msgs = messages_for(&diags, "assignment_compatibility"); + assert!( + msgs.is_empty(), + "nonmember attributes are not members; Literal[Yes, No] is complete, got: {msgs:?}" + ); + Ok(()) +} + +#[test] +fn literal_union_of_wrong_enum_still_fires() -> Result<(), Box> { + // The union names another enum's members; assigning Answer must fire. + let source = r#" +from enum import Enum +from typing import Literal + +class Answer(Enum): + Yes = 1 + No = 2 + +class Other(Enum): + A = 1 + B = 2 + +def test(a: Answer) -> None: + x: Literal[Other.A, Other.B] = a +"#; + let diags = run(source)?; + let msgs = messages_for(&diags, "assignment_compatibility"); + assert!( + !msgs.is_empty(), + "a literal union of a DIFFERENT enum's members must reject the value, got no diagnostic" + ); + Ok(()) +} diff --git a/crates/basilisk-checker/tests/checker_rules_a_tests.rs b/crates/basilisk-checker/tests/checker_rules_a_tests.rs index 60c866c8..4cb0f695 100644 --- a/crates/basilisk-checker/tests/checker_rules_a_tests.rs +++ b/crates/basilisk-checker/tests/checker_rules_a_tests.rs @@ -22,6 +22,8 @@ mod annotations_typeexpr; mod assignment_call_synthesis; #[path = "checker/assignment_compatibility_tests.rs"] mod assignment_compatibility; +#[path = "checker/assignment_compatibility_2_tests.rs"] +mod assignment_compatibility_2; #[path = "checker/callables_annotation_tests.rs"] mod callables_annotation; #[path = "checker/calls_argument_type_tests.rs"] diff --git a/crates/basilisk-checker/tests/import_support/mod.rs b/crates/basilisk-checker/tests/import_support/mod.rs index b9cbe4af..1cea6e8b 100644 --- a/crates/basilisk-checker/tests/import_support/mod.rs +++ b/crates/basilisk-checker/tests/import_support/mod.rs @@ -117,6 +117,7 @@ pub fn module_with_plain_imports(modules: &[&str]) -> basilisk_resolver::Resolve .iter() .map(|module| basilisk_resolver::ImportInfo { module: (*module).to_owned(), + relative_level: 0, names: vec![], span: basilisk_resolver::Span::new(0, 0), name_spans: Vec::new(), diff --git a/crates/basilisk-checker/tests/relative_import_resolution_tests.rs b/crates/basilisk-checker/tests/relative_import_resolution_tests.rs new file mode 100644 index 00000000..e06ae765 --- /dev/null +++ b/crates/basilisk-checker/tests/relative_import_resolution_tests.rs @@ -0,0 +1,254 @@ +//! Tests for [ANALYSIS-INCR-IMPORTS]. See docs/specs/LSP-ANALYSIS-MODES-SPEC.md#ANALYSIS-INCR-IMPORTS +#![allow( + clippy::allow_attributes, + clippy::unwrap_used, + clippy::expect_used, + clippy::indexing_slicing, + missing_docs +)] +//! Relative-import resolution through the full `resolve_module_imports` +//! pipeline (GitHub #369): `from ..sub import mod` must resolve by walking up +//! from the importing file's package, not by treating `sub` as an absolute +//! module. The standalone `resolve_relative_import` unit tests in +//! `import_resolution_tests.rs` pass in isolation — these tests pin the wiring +//! from the parsed AST (`StmtImportFrom.level`) through `ImportInfo` to the +//! resolver dispatch in `basilisk_checker::imports::resolve_module_imports`. + +use std::fs; +use std::path::Path; + +use basilisk_resolver::scope::ImportResolution; + +mod import_support; +use import_support::{make_search_paths, unique_tmp}; + +/// The issue #369 package layout: +/// +/// ```text +/// /src/pkg/__init__.py +/// /src/pkg/sub/__init__.py +/// /src/pkg/sub/mod.py +/// /src/pkg/dev/__init__.py +/// /src/pkg/dev/other.py +/// ``` +/// +/// Returns the workspace root; the importing file lives in `src/pkg/dev/`. +fn make_issue_369_layout(prefix: &str) -> std::path::PathBuf { + let root = unique_tmp(prefix); + let dev = root.join("src").join("pkg").join("dev"); + let sub = root.join("src").join("pkg").join("sub"); + fs::create_dir_all(&dev).unwrap(); + fs::create_dir_all(&sub).unwrap(); + fs::write(root.join("src").join("pkg").join("__init__.py"), "").unwrap(); + fs::write(dev.join("__init__.py"), "").unwrap(); + fs::write(dev.join("other.py"), "class Bar: pass\n").unwrap(); + fs::write(sub.join("__init__.py"), "").unwrap(); + fs::write(sub.join("mod.py"), "class Foo: pass\n").unwrap(); + root +} + +/// Parse `source` as a file at `importing_file` and run the real pipeline: +/// visitor capture (`basilisk_resolver::resolve`) followed by +/// `resolve_module_imports` — the exact path the CLI and the salsa +/// `resolved_module` query share. +fn resolve_imports_of( + source: &str, + importing_file: &Path, + root: &Path, +) -> basilisk_resolver::ResolvedModule { + let parsed = basilisk_parser::parse_source( + source.to_owned(), + importing_file.to_string_lossy().into_owned(), + ) + .expect("fixture parses"); + let mut resolved = basilisk_resolver::resolve(&parsed).expect("fixture resolves"); + let paths = make_search_paths(vec![root.to_path_buf()]); + basilisk_checker::imports::resolve_module_imports(&mut resolved, &paths); + resolved +} + +/// Issue #369 case 2: `from ..sub import mod` — double dot, bare module +/// target. Must resolve to the sibling package's `__init__.py`, one level up +/// from the importing file's package. +#[test] +fn double_dot_bare_module_target_resolves_to_parent_sibling_package() { + let root = make_issue_369_layout("bsk_rel369_dotdot"); + let importing = root.join("src").join("pkg").join("dev").join("user.py"); + + let resolved = resolve_imports_of("from ..sub import mod\n", &importing, &root); + + let import = &resolved.imports[0]; + assert_ne!( + import.resolution, + ImportResolution::Unresolved, + "`from ..sub import mod` must resolve `..sub` relative to the \ + importing package (GitHub #369); got unresolved_reason {:?}", + import.unresolved_reason + ); + let path = import.resolved_path.as_ref().unwrap(); + assert!( + path.ends_with(Path::new("src/pkg/sub/__init__.py")), + "`..sub` must resolve to src/pkg/sub/__init__.py, got {path:?}" + ); + + let _ = fs::remove_dir_all(&root); +} + +/// Issue #369 case 3: `from ..sub.mod import Foo` — double dot, dotted +/// attribute target. Must resolve to `src/pkg/sub/mod.py`. +#[test] +fn double_dot_dotted_module_target_resolves_through_parent_package() { + let root = make_issue_369_layout("bsk_rel369_dotted"); + let importing = root.join("src").join("pkg").join("dev").join("user.py"); + + let resolved = resolve_imports_of("from ..sub.mod import Foo\n", &importing, &root); + + let import = &resolved.imports[0]; + assert_ne!( + import.resolution, + ImportResolution::Unresolved, + "`from ..sub.mod import Foo` must resolve relative to the importing \ + package (GitHub #369); got unresolved_reason {:?}", + import.unresolved_reason + ); + let path = import.resolved_path.as_ref().unwrap(); + assert!( + path.ends_with(Path::new("src/pkg/sub/mod.py")), + "`..sub.mod` must resolve to src/pkg/sub/mod.py, got {path:?}" + ); + + let _ = fs::remove_dir_all(&root); +} + +/// `from . import other` — bare single dot with no module path. The import +/// target is the importing file's own package `__init__.py`; the bound name +/// `other` is a sibling submodule. +#[test] +fn bare_dot_import_resolves_to_own_package_init() { + let root = make_issue_369_layout("bsk_rel369_bare"); + let importing = root.join("src").join("pkg").join("dev").join("user.py"); + + let resolved = resolve_imports_of("from . import other\n", &importing, &root); + + let import = &resolved.imports[0]; + assert_ne!( + import.resolution, + ImportResolution::Unresolved, + "`from . import other` must resolve to the importing file's package \ + __init__.py (GitHub #369); got unresolved_reason {:?}", + import.unresolved_reason + ); + let path = import.resolved_path.as_ref().unwrap(); + assert!( + path.ends_with(Path::new("src/pkg/dev/__init__.py")), + "`from . import other` must resolve to src/pkg/dev/__init__.py, got {path:?}" + ); + + let _ = fs::remove_dir_all(&root); +} + +/// `from .. import sub` — bare double dot binding a subpackage of the parent. +#[test] +fn bare_double_dot_import_resolves_to_parent_package_init() { + let root = make_issue_369_layout("bsk_rel369_bare2"); + let importing = root.join("src").join("pkg").join("dev").join("user.py"); + + let resolved = resolve_imports_of("from .. import sub\n", &importing, &root); + + let import = &resolved.imports[0]; + assert_ne!( + import.resolution, + ImportResolution::Unresolved, + "`from .. import sub` must resolve to the parent package __init__.py \ + (GitHub #369); got unresolved_reason {:?}", + import.unresolved_reason + ); + let path = import.resolved_path.as_ref().unwrap(); + assert!( + path.ends_with(Path::new("src/pkg/__init__.py")), + "`from .. import sub` must resolve to src/pkg/__init__.py, got {path:?}" + ); + + let _ = fs::remove_dir_all(&root); +} + +/// `from ..sub import *` — the star-import form must dispatch through the same +/// relative resolution as the named form. +#[test] +fn double_dot_star_import_resolves_to_parent_sibling_package() { + let root = make_issue_369_layout("bsk_rel369_star"); + let importing = root.join("src").join("pkg").join("dev").join("user.py"); + + let resolved = resolve_imports_of("from ..sub import *\n", &importing, &root); + + let import = &resolved.imports[0]; + assert_ne!( + import.resolution, + ImportResolution::Unresolved, + "`from ..sub import *` must resolve `..sub` relative to the importing \ + package (GitHub #369); got unresolved_reason {:?}", + import.unresolved_reason + ); + let path = import.resolved_path.as_ref().unwrap(); + assert!( + path.ends_with(Path::new("src/pkg/sub/__init__.py")), + "`..sub` must resolve to src/pkg/sub/__init__.py, got {path:?}" + ); + + let _ = fs::remove_dir_all(&root); +} + +/// Issue #369 case 1: `from .other import Bar` — single dot, sibling module. +/// Passes today only by accident (the absolute resolver's importer-directory +/// fallback); pinned here so the relative dispatch keeps it working on purpose. +#[test] +fn single_dot_sibling_module_resolves() { + let root = make_issue_369_layout("bsk_rel369_dot"); + let importing = root.join("src").join("pkg").join("dev").join("user.py"); + + let resolved = resolve_imports_of("from .other import Bar\n", &importing, &root); + + let import = &resolved.imports[0]; + assert_ne!(import.resolution, ImportResolution::Unresolved); + let path = import.resolved_path.as_ref().unwrap(); + assert!( + path.ends_with(Path::new("src/pkg/dev/other.py")), + "`.other` must resolve to src/pkg/dev/other.py, got {path:?}" + ); + + let _ = fs::remove_dir_all(&root); +} + +/// A relative import whose target genuinely does not exist stays unresolved — +/// the fix must add resolution, not blanket-accept every nonzero level. +#[test] +fn missing_relative_target_stays_unresolved() { + let root = make_issue_369_layout("bsk_rel369_missing"); + let importing = root.join("src").join("pkg").join("dev").join("user.py"); + + let resolved = resolve_imports_of("from ..nonexistent import x\n", &importing, &root); + + let import = &resolved.imports[0]; + assert_eq!( + import.resolution, + ImportResolution::Unresolved, + "a relative import of a module that does not exist must stay unresolved" + ); + + let _ = fs::remove_dir_all(&root); +} + +/// An absolute import is untouched by the relative dispatch: level 0 keeps the +/// full search-path walk (here: workspace-root resolution of `src`-less +/// absolute names still fails, exactly as before). +#[test] +fn absolute_import_still_walks_search_paths() { + let root = make_issue_369_layout("bsk_rel369_abs"); + let importing = root.join("src").join("pkg").join("dev").join("user.py"); + // `src` is not on the search path as a package root, so the absolute name + // `pkg.sub` must NOT resolve — only the relative form reaches it. + let resolved = resolve_imports_of("from pkg.sub import mod\n", &importing, &root); + assert_eq!(resolved.imports[0].resolution, ImportResolution::Unresolved); + + let _ = fs::remove_dir_all(&root); +} diff --git a/crates/basilisk-resolver/src/scope/import_types.rs b/crates/basilisk-resolver/src/scope/import_types.rs index 6d6adb4c..3c86abbc 100644 --- a/crates/basilisk-resolver/src/scope/import_types.rs +++ b/crates/basilisk-resolver/src/scope/import_types.rs @@ -71,7 +71,12 @@ pub enum UnresolvedReason { #[derive(Debug, Clone, PartialEq)] pub struct ImportInfo { /// The dotted module name being imported (e.g. `"os.path"`, `"requests"`). + /// Empty for a bare relative import (`from . import x`). pub module: String, + /// Number of leading dots on a `from`-import (`from ..sub import x` → 2); + /// `0` for absolute imports. A nonzero level resolves `module` relative to + /// the importing file's package instead of the search paths (GitHub #369). + pub relative_level: u32, /// Locally-bound names introduced by the import. /// `from X import A, B` → `["A", "B"]` (alias-aware: `import C as D` → `["D"]`). /// Plain `import X` / `import X.Y` is empty — the bound name is the top-level diff --git a/crates/basilisk-resolver/src/visitor/class_info_ext.rs b/crates/basilisk-resolver/src/visitor/class_info_ext.rs index c5169e57..b84c356f 100644 --- a/crates/basilisk-resolver/src/visitor/class_info_ext.rs +++ b/crates/basilisk-resolver/src/visitor/class_info_ext.rs @@ -359,6 +359,8 @@ pub(super) fn import_infos_from(node: &StmtImport) -> Vec { .iter() .map(|alias| ImportInfo { module: alias.name.to_string(), + // Plain `import X` is always absolute; Python has no relative form. + relative_level: 0, // `import X as Y` binds `Y`, not the module name — capture the alias so // scope-resolution rules (e.g. names_undefined) see the real binding. Plain // `import X` / `import X.Y` keeps `names` empty; its bound name is the @@ -410,6 +412,7 @@ pub(super) fn import_from_infos_from(node: &StmtImportFrom) -> Vec { if is_star { return vec![ImportInfo { module, + relative_level: node.level, names: Vec::new(), span: text_range_to_span(node.range), name_spans: module_span.into_iter().collect(), @@ -431,6 +434,7 @@ pub(super) fn import_from_infos_from(node: &StmtImportFrom) -> Vec { .collect(); vec![ImportInfo { module, + relative_level: node.level, names, span: text_range_to_span(node.range), name_spans,