mirror of
https://github.com/astral-sh/ruff.git
synced 2025-10-16 21:38:11 +00:00
[ty] Disambiguate classes that live in different modules but have the same fully qualified names (#20756)
Some checks are pending
CI / Determine changes (push) Waiting to run
CI / cargo fmt (push) Waiting to run
CI / cargo clippy (push) Blocked by required conditions
CI / cargo test (linux) (push) Blocked by required conditions
CI / cargo test (linux, release) (push) Blocked by required conditions
CI / cargo test (windows) (push) Blocked by required conditions
CI / cargo test (wasm) (push) Blocked by required conditions
CI / cargo build (release) (push) Waiting to run
CI / cargo build (msrv) (push) Blocked by required conditions
CI / cargo fuzz build (push) Blocked by required conditions
CI / fuzz parser (push) Blocked by required conditions
CI / test scripts (push) Blocked by required conditions
CI / ecosystem (push) Blocked by required conditions
CI / Fuzz for new ty panics (push) Blocked by required conditions
CI / cargo shear (push) Blocked by required conditions
CI / ty completion evaluation (push) Blocked by required conditions
CI / python package (push) Waiting to run
CI / pre-commit (push) Waiting to run
CI / mkdocs (push) Waiting to run
CI / formatter instabilities and black similarity (push) Blocked by required conditions
CI / test ruff-lsp (push) Blocked by required conditions
CI / check playground (push) Blocked by required conditions
CI / benchmarks instrumented (ruff) (push) Blocked by required conditions
CI / benchmarks instrumented (ty) (push) Blocked by required conditions
CI / benchmarks-walltime (push) Blocked by required conditions
[ty Playground] Release / publish (push) Waiting to run
Some checks are pending
CI / Determine changes (push) Waiting to run
CI / cargo fmt (push) Waiting to run
CI / cargo clippy (push) Blocked by required conditions
CI / cargo test (linux) (push) Blocked by required conditions
CI / cargo test (linux, release) (push) Blocked by required conditions
CI / cargo test (windows) (push) Blocked by required conditions
CI / cargo test (wasm) (push) Blocked by required conditions
CI / cargo build (release) (push) Waiting to run
CI / cargo build (msrv) (push) Blocked by required conditions
CI / cargo fuzz build (push) Blocked by required conditions
CI / fuzz parser (push) Blocked by required conditions
CI / test scripts (push) Blocked by required conditions
CI / ecosystem (push) Blocked by required conditions
CI / Fuzz for new ty panics (push) Blocked by required conditions
CI / cargo shear (push) Blocked by required conditions
CI / ty completion evaluation (push) Blocked by required conditions
CI / python package (push) Waiting to run
CI / pre-commit (push) Waiting to run
CI / mkdocs (push) Waiting to run
CI / formatter instabilities and black similarity (push) Blocked by required conditions
CI / test ruff-lsp (push) Blocked by required conditions
CI / check playground (push) Blocked by required conditions
CI / benchmarks instrumented (ruff) (push) Blocked by required conditions
CI / benchmarks instrumented (ty) (push) Blocked by required conditions
CI / benchmarks-walltime (push) Blocked by required conditions
[ty Playground] Release / publish (push) Waiting to run
## Summary Even disambiguating classes using their fully qualified names is not enough for some diagnostics. We've seen real-world examples in the ecosystem (and https://github.com/astral-sh/ruff/pull/20368 introduces some more!) where two types can be different, but can still have the same fully qualified name. In these cases, our disambiguation machinery needs to print the file path and line number of the class in order to disambiguate classes with similar names in our diagnostics. Helps with https://github.com/astral-sh/ty/issues/1306 ## Test Plan Mdtests
This commit is contained in:
parent
150ea92d03
commit
b9c84add07
7 changed files with 159 additions and 42 deletions
|
@ -61,6 +61,35 @@ class DataFrame:
|
|||
pass
|
||||
```
|
||||
|
||||
## Class from different module with the same qualified name
|
||||
|
||||
`package/__init__.py`:
|
||||
|
||||
```py
|
||||
from .foo import MyClass
|
||||
|
||||
def make_MyClass() -> MyClass:
|
||||
return MyClass()
|
||||
```
|
||||
|
||||
`package/foo.pyi`:
|
||||
|
||||
```pyi
|
||||
class MyClass: ...
|
||||
```
|
||||
|
||||
`package/foo.py`:
|
||||
|
||||
```py
|
||||
class MyClass: ...
|
||||
|
||||
def get_MyClass() -> MyClass:
|
||||
from . import make_MyClass
|
||||
|
||||
# error: [invalid-return-type] "Return type does not match returned value: expected `package.foo.MyClass @ src/package/foo.py:1`, found `package.foo.MyClass @ src/package/foo.pyi:1`"
|
||||
return make_MyClass()
|
||||
```
|
||||
|
||||
## Enum from different modules
|
||||
|
||||
```py
|
||||
|
|
|
@ -339,7 +339,7 @@ class A: ...
|
|||
|
||||
def f(x: A):
|
||||
# TODO: no error
|
||||
# error: [invalid-assignment] "Object of type `mdtest_snippet.A | mdtest_snippet.A` is not assignable to `mdtest_snippet.A`"
|
||||
# error: [invalid-assignment] "Object of type `mdtest_snippet.A @ src/mdtest_snippet.py:12 | mdtest_snippet.A @ src/mdtest_snippet.py:13` is not assignable to `mdtest_snippet.A @ src/mdtest_snippet.py:13`"
|
||||
x = A()
|
||||
```
|
||||
|
||||
|
|
|
@ -1462,7 +1462,7 @@ impl<'db> ClassLiteral<'db> {
|
|||
.map(|generic_context| generic_context.promote_literals(db))
|
||||
}
|
||||
|
||||
fn file(self, db: &dyn Db) -> File {
|
||||
pub(super) fn file(self, db: &dyn Db) -> File {
|
||||
self.body_scope(db).file(db)
|
||||
}
|
||||
|
||||
|
|
|
@ -1,11 +1,14 @@
|
|||
//! Display implementations for types.
|
||||
|
||||
use std::borrow::Cow;
|
||||
use std::cell::RefCell;
|
||||
use std::collections::hash_map::Entry;
|
||||
use std::fmt::{self, Display, Formatter, Write};
|
||||
use std::rc::Rc;
|
||||
|
||||
use ruff_db::display::FormatterJoinExtension;
|
||||
use ruff_db::files::FilePath;
|
||||
use ruff_db::source::line_index;
|
||||
use ruff_python_ast::str::{Quote, TripleQuotes};
|
||||
use ruff_python_literal::escape::AsciiEscape;
|
||||
use ruff_text_size::{TextRange, TextSize};
|
||||
|
@ -34,7 +37,7 @@ pub struct DisplaySettings<'db> {
|
|||
pub multiline: bool,
|
||||
/// Class names that should be displayed fully qualified
|
||||
/// (e.g., `module.ClassName` instead of just `ClassName`)
|
||||
pub qualified: Rc<FxHashSet<&'db str>>,
|
||||
pub qualified: Rc<FxHashMap<&'db str, QualificationLevel>>,
|
||||
/// Whether long unions are displayed in full
|
||||
pub preserve_full_unions: bool,
|
||||
}
|
||||
|
@ -88,7 +91,9 @@ impl<'db> DisplaySettings<'db> {
|
|||
.class_names
|
||||
.borrow()
|
||||
.iter()
|
||||
.filter_map(|(name, ambiguity)| ambiguity.is_ambiguous().then_some(*name))
|
||||
.filter_map(|(name, ambiguity)| {
|
||||
Some((*name, QualificationLevel::from_ambiguity_state(ambiguity)?))
|
||||
})
|
||||
.collect(),
|
||||
),
|
||||
..Self::default()
|
||||
|
@ -96,6 +101,22 @@ impl<'db> DisplaySettings<'db> {
|
|||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
pub enum QualificationLevel {
|
||||
ModuleName,
|
||||
FileAndLineNumber,
|
||||
}
|
||||
|
||||
impl QualificationLevel {
|
||||
const fn from_ambiguity_state(state: &AmbiguityState) -> Option<Self> {
|
||||
match state {
|
||||
AmbiguityState::Unambiguous(_) => None,
|
||||
AmbiguityState::RequiresFullyQualifiedName { .. } => Some(Self::ModuleName),
|
||||
AmbiguityState::RequiresFileAndLineNumber => Some(Self::FileAndLineNumber),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Default)]
|
||||
struct AmbiguousClassCollector<'db> {
|
||||
visited_types: RefCell<FxHashSet<Type<'db>>>,
|
||||
|
@ -110,10 +131,32 @@ impl<'db> AmbiguousClassCollector<'db> {
|
|||
}
|
||||
Entry::Occupied(mut entry) => {
|
||||
let value = entry.get_mut();
|
||||
if let AmbiguityState::Unambiguous(existing) = value
|
||||
&& *existing != class
|
||||
{
|
||||
*value = AmbiguityState::Ambiguous;
|
||||
match value {
|
||||
AmbiguityState::Unambiguous(existing) => {
|
||||
if *existing != class {
|
||||
let qualified_name_components = class.qualified_name_components(db);
|
||||
if existing.qualified_name_components(db) == qualified_name_components {
|
||||
*value = AmbiguityState::RequiresFileAndLineNumber;
|
||||
} else {
|
||||
*value = AmbiguityState::RequiresFullyQualifiedName {
|
||||
class,
|
||||
qualified_name_components,
|
||||
};
|
||||
}
|
||||
}
|
||||
}
|
||||
AmbiguityState::RequiresFullyQualifiedName {
|
||||
class: existing,
|
||||
qualified_name_components,
|
||||
} => {
|
||||
if *existing != class {
|
||||
let new_components = class.qualified_name_components(db);
|
||||
if *qualified_name_components == new_components {
|
||||
*value = AmbiguityState::RequiresFileAndLineNumber;
|
||||
}
|
||||
}
|
||||
}
|
||||
AmbiguityState::RequiresFileAndLineNumber => {}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
@ -122,18 +165,18 @@ impl<'db> AmbiguousClassCollector<'db> {
|
|||
|
||||
/// Whether or not a class can be unambiguously identified by its *unqualified* name
|
||||
/// given the other types that are present in the same context.
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
enum AmbiguityState<'db> {
|
||||
/// The class can be displayed unambiguously using its unqualified name
|
||||
Unambiguous(ClassLiteral<'db>),
|
||||
/// The class must be displayed using its fully qualified name to avoid ambiguity.
|
||||
Ambiguous,
|
||||
}
|
||||
|
||||
impl AmbiguityState<'_> {
|
||||
const fn is_ambiguous(self) -> bool {
|
||||
matches!(self, AmbiguityState::Ambiguous)
|
||||
}
|
||||
RequiresFullyQualifiedName {
|
||||
class: ClassLiteral<'db>,
|
||||
qualified_name_components: Vec<String>,
|
||||
},
|
||||
/// Even the class's fully qualified name is not sufficient;
|
||||
/// we must also include the file and line number.
|
||||
RequiresFileAndLineNumber,
|
||||
}
|
||||
|
||||
impl<'db> super::visitor::TypeVisitor<'db> for AmbiguousClassCollector<'db> {
|
||||
|
@ -232,21 +275,19 @@ impl<'db> ClassLiteral<'db> {
|
|||
settings,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
struct ClassDisplay<'db> {
|
||||
db: &'db dyn Db,
|
||||
class: ClassLiteral<'db>,
|
||||
settings: DisplaySettings<'db>,
|
||||
}
|
||||
|
||||
impl ClassDisplay<'_> {
|
||||
fn class_parents(&self) -> Vec<String> {
|
||||
let body_scope = self.class.body_scope(self.db);
|
||||
let file = body_scope.file(self.db);
|
||||
let module_ast = parsed_module(self.db, file).load(self.db);
|
||||
let index = semantic_index(self.db, file);
|
||||
let file_scope_id = body_scope.file_scope_id(self.db);
|
||||
/// Returns the components of the qualified name of this class, excluding this class itself.
|
||||
///
|
||||
/// For example, calling this method on a class `C` in the module `a.b` would return
|
||||
/// `["a", "b"]`. Calling this method on a class `D` inside the namespace of a method
|
||||
/// `m` inside the namespace of a class `C` in the module `a.b` would return
|
||||
/// `["a", "b", "C", "<locals of function 'm'>"]`.
|
||||
fn qualified_name_components(self, db: &'db dyn Db) -> Vec<String> {
|
||||
let body_scope = self.body_scope(db);
|
||||
let file = body_scope.file(db);
|
||||
let module_ast = parsed_module(db, file).load(db);
|
||||
let index = semantic_index(db, file);
|
||||
let file_scope_id = body_scope.file_scope_id(db);
|
||||
|
||||
let mut name_parts = vec![];
|
||||
|
||||
|
@ -272,8 +313,8 @@ impl ClassDisplay<'_> {
|
|||
}
|
||||
}
|
||||
|
||||
if let Some(module) = file_to_module(self.db, file) {
|
||||
let module_name = module.name(self.db);
|
||||
if let Some(module) = file_to_module(db, file) {
|
||||
let module_name = module.name(db);
|
||||
name_parts.push(module_name.as_str().to_string());
|
||||
}
|
||||
|
||||
|
@ -282,19 +323,39 @@ impl ClassDisplay<'_> {
|
|||
}
|
||||
}
|
||||
|
||||
struct ClassDisplay<'db> {
|
||||
db: &'db dyn Db,
|
||||
class: ClassLiteral<'db>,
|
||||
settings: DisplaySettings<'db>,
|
||||
}
|
||||
|
||||
impl Display for ClassDisplay<'_> {
|
||||
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
|
||||
if self
|
||||
.settings
|
||||
.qualified
|
||||
.contains(&**self.class.name(self.db))
|
||||
{
|
||||
for parent in self.class_parents() {
|
||||
let qualification_level = self.settings.qualified.get(&**self.class.name(self.db));
|
||||
if qualification_level.is_some() {
|
||||
for parent in self.class.qualified_name_components(self.db) {
|
||||
f.write_str(&parent)?;
|
||||
f.write_char('.')?;
|
||||
}
|
||||
}
|
||||
f.write_str(self.class.name(self.db))
|
||||
f.write_str(self.class.name(self.db))?;
|
||||
if qualification_level == Some(&QualificationLevel::FileAndLineNumber) {
|
||||
let file = self.class.file(self.db);
|
||||
let path = file.path(self.db);
|
||||
let path = match path {
|
||||
FilePath::System(path) => Cow::Owned(FilePath::System(
|
||||
path.strip_prefix(self.db.system().current_directory())
|
||||
.unwrap_or(path)
|
||||
.to_path_buf(),
|
||||
)),
|
||||
FilePath::Vendored(_) | FilePath::SystemVirtual(_) => Cow::Borrowed(path),
|
||||
};
|
||||
let line_index = line_index(self.db, file);
|
||||
let class_offset = self.class.header_range(self.db).start();
|
||||
let line_number = line_index.line_index(class_offset);
|
||||
write!(f, " @ {path}:{line_number}")?;
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
|
|
Loading…
Add table
Add a link
Reference in a new issue