mirror of
https://github.com/astral-sh/ruff.git
synced 2025-09-29 13:24:57 +00:00
[ty] print diagnostics with fully qualified name to disambiguate some cases (#19850)
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 / mkdocs (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 / python package (push) Waiting to run
CI / pre-commit (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 (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 / mkdocs (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 / python package (push) Waiting to run
CI / pre-commit (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 (push) Blocked by required conditions
CI / benchmarks-walltime (push) Blocked by required conditions
[ty Playground] Release / publish (push) Waiting to run
There are some situations that we have a confusing diagnostics due to identical class names. ## Class with same name from different modules ```python import pandas import polars df: pandas.DataFrame = polars.DataFrame() ``` This yields the following error: **Actual:** error: [invalid-assignment] "Object of type `DataFrame` is not assignable to `DataFrame`" **Expected**: error: [invalid-assignment] "Object of type `polars.DataFrame` is not assignable to `pandas.DataFrame`" ## Nested classes ```python from enum import Enum class A: class B(Enum): ACTIVE = "active" INACTIVE = "inactive" class C: class B(Enum): ACTIVE = "active" INACTIVE = "inactive" ``` **Actual**: error: [invalid-assignment] "Object of type `Literal[B.ACTIVE]` is not assignable to `B`" **Expected**: error: [invalid-assignment] "Object of type `Literal[my_module.C.B.ACTIVE]` is not assignable to `my_module.A.B`" ## Solution In this MR we added an heuristics to detect when to use a fully qualified name: - There is an invalid assignment and; - They are two different classes and; - They have the same name The fully qualified name always includes: - module name - nested classes name - actual class name There was no `QualifiedDisplay` so I had to implement it from scratch. I'm very new to the codebase, so I might have done things inefficiently, so I appreciate feedback. Should we pre-compute the fully qualified name or do it on demand? ## Not implemented ### Function-local classes Should we approach this in a different PR? **Example**: ```python # t.py from __future__ import annotations def function() -> A: class A: pass return A() class A: pass a: A = function() ``` #### mypy ```console t.py:8: error: Incompatible return value type (got "t.A@5", expected "t.A") [return-value] ``` From my testing the 5 in `A@5` comes from the like number. #### ty ```console error[invalid-return-type]: Return type does not match returned value --> t.py:4:19 | 4 | def function() -> A: | - Expected `A` because of return type 5 | class A: 6 | pass 7 | 8 | return A() | ^^^ expected `A`, found `A` | info: rule `invalid-return-type` is enabled by default ``` Fixes https://github.com/astral-sh/ty/issues/848 --------- Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com> Co-authored-by: Carl Meyer <carl@astral.sh>
This commit is contained in:
parent
89ca493fd9
commit
d75ef3823c
4 changed files with 440 additions and 81 deletions
|
@ -10,7 +10,7 @@ use crate::semantic_index::SemanticIndex;
|
|||
use crate::semantic_index::definition::Definition;
|
||||
use crate::semantic_index::place::{PlaceTable, ScopedPlaceId};
|
||||
use crate::suppression::FileSuppressionId;
|
||||
use crate::types::class::{DisjointBase, DisjointBaseKind, Field};
|
||||
use crate::types::class::{ClassType, DisjointBase, DisjointBaseKind, Field};
|
||||
use crate::types::function::KnownFunction;
|
||||
use crate::types::string_annotation::{
|
||||
BYTE_STRING_TYPE_ANNOTATION, ESCAPE_CHARACTER_IN_FORWARD_ANNOTATION, FSTRING_TYPE_ANNOTATION,
|
||||
|
@ -22,7 +22,9 @@ use crate::types::{
|
|||
};
|
||||
use crate::types::{SpecialFormType, Type, protocol_class::ProtocolClass};
|
||||
use crate::util::diagnostics::format_enumeration;
|
||||
use crate::{Db, FxIndexMap, FxOrderMap, Module, ModuleName, Program, declare_lint};
|
||||
use crate::{
|
||||
Db, DisplaySettings, FxIndexMap, FxOrderMap, Module, ModuleName, Program, declare_lint,
|
||||
};
|
||||
use itertools::Itertools;
|
||||
use ruff_db::diagnostic::{Annotation, Diagnostic, SubDiagnostic, SubDiagnosticSeverity};
|
||||
use ruff_python_ast::name::Name;
|
||||
|
@ -1943,18 +1945,61 @@ pub(super) fn report_invalid_assignment(
|
|||
target_ty: Type,
|
||||
source_ty: Type,
|
||||
) {
|
||||
let mut settings = DisplaySettings::default();
|
||||
// Handles the situation where the report naming is confusing, such as class with the same Name,
|
||||
// but from different scopes.
|
||||
if let Some(target_class) = type_to_class_literal(target_ty, context.db()) {
|
||||
if let Some(source_class) = type_to_class_literal(source_ty, context.db()) {
|
||||
if target_class != source_class
|
||||
&& target_class.name(context.db()) == source_class.name(context.db())
|
||||
{
|
||||
settings = settings.qualified();
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
report_invalid_assignment_with_message(
|
||||
context,
|
||||
node,
|
||||
target_ty,
|
||||
format_args!(
|
||||
"Object of type `{}` is not assignable to `{}`",
|
||||
source_ty.display(context.db()),
|
||||
target_ty.display(context.db()),
|
||||
source_ty.display_with(context.db(), settings),
|
||||
target_ty.display_with(context.db(), settings)
|
||||
),
|
||||
);
|
||||
}
|
||||
|
||||
// TODO: generalize this to a method that takes any two types, walks them recursively, and returns
|
||||
// a set of types with ambiguous names whose display should be qualified. Then we can use this in
|
||||
// any diagnostic that displays two types.
|
||||
fn type_to_class_literal<'db>(ty: Type<'db>, db: &'db dyn crate::Db) -> Option<ClassLiteral<'db>> {
|
||||
match ty {
|
||||
Type::ClassLiteral(class) => Some(class),
|
||||
Type::NominalInstance(instance) => match instance.class(db) {
|
||||
crate::types::class::ClassType::NonGeneric(class) => Some(class),
|
||||
crate::types::class::ClassType::Generic(alias) => Some(alias.origin(db)),
|
||||
},
|
||||
Type::EnumLiteral(enum_literal) => Some(enum_literal.enum_class(db)),
|
||||
Type::GenericAlias(alias) => Some(alias.origin(db)),
|
||||
Type::ProtocolInstance(ProtocolInstanceType {
|
||||
inner: Protocol::FromClass(class),
|
||||
..
|
||||
}) => match class {
|
||||
ClassType::NonGeneric(class) => Some(class),
|
||||
ClassType::Generic(alias) => Some(alias.origin(db)),
|
||||
},
|
||||
Type::TypedDict(typed_dict) => match typed_dict.defining_class() {
|
||||
ClassType::NonGeneric(class) => Some(class),
|
||||
ClassType::Generic(alias) => Some(alias.origin(db)),
|
||||
},
|
||||
Type::SubclassOf(subclass_of) => {
|
||||
type_to_class_literal(Type::from(subclass_of.subclass_of().into_class()?), db)
|
||||
}
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
|
||||
pub(super) fn report_invalid_attribute_assignment(
|
||||
context: &InferContext,
|
||||
node: AnyNodeRef,
|
||||
|
|
|
@ -8,6 +8,8 @@ use ruff_python_literal::escape::AsciiEscape;
|
|||
use ruff_text_size::{TextRange, TextSize};
|
||||
|
||||
use crate::Db;
|
||||
use crate::module_resolver::file_to_module;
|
||||
use crate::semantic_index::{scope::ScopeKind, semantic_index};
|
||||
use crate::types::class::{ClassLiteral, ClassType, GenericAlias};
|
||||
use crate::types::function::{FunctionType, OverloadLiteral};
|
||||
use crate::types::generics::{GenericContext, Specialization};
|
||||
|
@ -17,23 +19,40 @@ use crate::types::{
|
|||
CallableType, IntersectionType, KnownClass, MethodWrapperKind, Protocol, StringLiteralType,
|
||||
SubclassOfInner, Type, UnionType, WrapperDescriptorKind,
|
||||
};
|
||||
use ruff_db::parsed::parsed_module;
|
||||
|
||||
/// Settings for displaying types and signatures
|
||||
#[derive(Debug, Copy, Clone, Default)]
|
||||
pub struct DisplaySettings {
|
||||
/// Whether rendering can be multiline
|
||||
pub multiline: bool,
|
||||
/// Whether rendering will show qualified display (e.g., module.class)
|
||||
pub qualified: bool,
|
||||
}
|
||||
|
||||
impl DisplaySettings {
|
||||
#[must_use]
|
||||
pub fn multiline(self) -> Self {
|
||||
Self { multiline: true }
|
||||
Self {
|
||||
multiline: true,
|
||||
..self
|
||||
}
|
||||
}
|
||||
|
||||
#[must_use]
|
||||
pub fn singleline(self) -> Self {
|
||||
Self { multiline: false }
|
||||
Self {
|
||||
multiline: false,
|
||||
..self
|
||||
}
|
||||
}
|
||||
|
||||
#[must_use]
|
||||
pub fn qualified(self) -> Self {
|
||||
Self {
|
||||
qualified: true,
|
||||
..self
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
@ -103,6 +122,65 @@ struct DisplayRepresentation<'db> {
|
|||
settings: DisplaySettings,
|
||||
}
|
||||
|
||||
impl DisplayRepresentation<'_> {
|
||||
fn class_parents(&self, class: ClassLiteral) -> Vec<String> {
|
||||
let body_scope = 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);
|
||||
|
||||
let mut name_parts = vec![];
|
||||
|
||||
// Skips itself
|
||||
for (ancestor_file_scope_id, ancestor_scope) in index.ancestor_scopes(file_scope_id).skip(1)
|
||||
{
|
||||
let ancestor_scope_id = ancestor_file_scope_id.to_scope_id(self.db, file);
|
||||
let node = ancestor_scope_id.node(self.db);
|
||||
|
||||
match ancestor_scope.kind() {
|
||||
ScopeKind::Class => {
|
||||
if let Some(class_def) = node.as_class(&module_ast) {
|
||||
name_parts.push(class_def.name.as_str().to_string());
|
||||
}
|
||||
}
|
||||
ScopeKind::Function => {
|
||||
if let Some(function_def) = node.as_function(&module_ast) {
|
||||
name_parts.push(format!(
|
||||
"<locals of function '{}'>",
|
||||
function_def.name.as_str()
|
||||
));
|
||||
}
|
||||
}
|
||||
_ => {}
|
||||
}
|
||||
}
|
||||
|
||||
if let Some(module) = file_to_module(self.db, file) {
|
||||
let module_name = module.name(self.db);
|
||||
name_parts.push(module_name.as_str().to_string());
|
||||
}
|
||||
|
||||
name_parts.reverse();
|
||||
name_parts
|
||||
}
|
||||
|
||||
fn write_maybe_qualified_class(
|
||||
&self,
|
||||
f: &mut Formatter<'_>,
|
||||
class: ClassLiteral,
|
||||
) -> fmt::Result {
|
||||
if self.settings.qualified {
|
||||
let parents = self.class_parents(class);
|
||||
if !parents.is_empty() {
|
||||
f.write_str(&parents.join("."))?;
|
||||
f.write_char('.')?;
|
||||
}
|
||||
}
|
||||
f.write_str(class.name(self.db))
|
||||
}
|
||||
}
|
||||
|
||||
impl Display for DisplayRepresentation<'_> {
|
||||
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
|
||||
match self.ty {
|
||||
|
@ -120,13 +198,15 @@ impl Display for DisplayRepresentation<'_> {
|
|||
.expect("Specialization::tuple() should always return `Some()` for `KnownClass::Tuple`")
|
||||
.display_with(self.db, self.settings)
|
||||
.fmt(f),
|
||||
(ClassType::NonGeneric(class), _) => f.write_str(class.name(self.db)),
|
||||
(ClassType::NonGeneric(class), _) => {
|
||||
self.write_maybe_qualified_class(f, class)
|
||||
},
|
||||
(ClassType::Generic(alias), _) => alias.display_with(self.db, self.settings).fmt(f),
|
||||
}
|
||||
}
|
||||
Type::ProtocolInstance(protocol) => match protocol.inner {
|
||||
Protocol::FromClass(ClassType::NonGeneric(class)) => {
|
||||
f.write_str(class.name(self.db))
|
||||
self.write_maybe_qualified_class(f, class)
|
||||
}
|
||||
Protocol::FromClass(ClassType::Generic(alias)) => {
|
||||
alias.display_with(self.db, self.settings).fmt(f)
|
||||
|
@ -151,7 +231,9 @@ impl Display for DisplayRepresentation<'_> {
|
|||
write!(f, "<module '{}'>", module.module(self.db).name(self.db))
|
||||
}
|
||||
Type::ClassLiteral(class) => {
|
||||
write!(f, "<class '{}'>", class.name(self.db))
|
||||
write!(f, "<class '")?;
|
||||
self.write_maybe_qualified_class(f, class)?;
|
||||
write!(f, "'>")
|
||||
}
|
||||
Type::GenericAlias(generic) => write!(
|
||||
f,
|
||||
|
@ -160,7 +242,9 @@ impl Display for DisplayRepresentation<'_> {
|
|||
),
|
||||
Type::SubclassOf(subclass_of_ty) => match subclass_of_ty.subclass_of() {
|
||||
SubclassOfInner::Class(ClassType::NonGeneric(class)) => {
|
||||
write!(f, "type[{}]", class.name(self.db))
|
||||
write!(f, "type[")?;
|
||||
self.write_maybe_qualified_class(f, class)?;
|
||||
write!(f, "]")
|
||||
}
|
||||
SubclassOfInner::Class(ClassType::Generic(alias)) => {
|
||||
write!(
|
||||
|
@ -271,12 +355,9 @@ impl Display for DisplayRepresentation<'_> {
|
|||
escape.bytes_repr(TripleQuotes::No).write(f)
|
||||
}
|
||||
Type::EnumLiteral(enum_literal) => {
|
||||
write!(
|
||||
f,
|
||||
"{enum_class}.{name}",
|
||||
enum_class = enum_literal.enum_class(self.db).name(self.db),
|
||||
name = enum_literal.name(self.db),
|
||||
)
|
||||
self.write_maybe_qualified_class(f, enum_literal.enum_class(self.db))?;
|
||||
f.write_char('.')?;
|
||||
f.write_str(enum_literal.name(self.db))
|
||||
}
|
||||
Type::NonInferableTypeVar(bound_typevar) | Type::TypeVar(bound_typevar) => {
|
||||
f.write_str(bound_typevar.typevar(self.db).name(self.db))?;
|
||||
|
@ -312,7 +393,10 @@ impl Display for DisplayRepresentation<'_> {
|
|||
}
|
||||
f.write_str("]")
|
||||
}
|
||||
Type::TypedDict(typed_dict) => f.write_str(typed_dict.defining_class().name(self.db)),
|
||||
Type::TypedDict(typed_dict) => self.write_maybe_qualified_class(
|
||||
f,
|
||||
typed_dict.defining_class().class_literal(self.db).0,
|
||||
),
|
||||
Type::TypeAlias(alias) => f.write_str(alias.name(self.db)),
|
||||
}
|
||||
}
|
||||
|
|
Loading…
Add table
Add a link
Reference in a new issue