mirror of
https://github.com/astral-sh/ruff.git
synced 2025-09-29 05:14:52 +00:00
[ty] Add backreferences to TypedDict items in diagnostics (#20262)
## Summary Add backreferences to the original item declaration in TypedDict diagnostics. Thanks to @AlexWaygood for the suggestion. ## Test Plan Updated snapshots
This commit is contained in:
parent
9e45bfa9fd
commit
8ade6c4eaf
7 changed files with 146 additions and 149 deletions
|
@ -108,6 +108,16 @@ error[invalid-assignment]: Invalid assignment to key "age" with declared type `i
|
|||
20 |
|
||||
21 | def write_to_non_existing_key(person: Person):
|
||||
|
|
||||
info: Item declaration
|
||||
--> src/mdtest_snippet.py:5:5
|
||||
|
|
||||
3 | class Person(TypedDict):
|
||||
4 | name: str
|
||||
5 | age: int | None
|
||||
| --------------- Item declared here
|
||||
6 |
|
||||
7 | def access_invalid_literal_string_key(person: Person):
|
||||
|
|
||||
info: rule `invalid-assignment` is enabled by default
|
||||
|
||||
```
|
||||
|
@ -151,6 +161,14 @@ error[invalid-assignment]: Cannot assign to key "id" on TypedDict `Employee`
|
|||
| |
|
||||
| TypedDict `Employee`
|
||||
|
|
||||
info: Item declaration
|
||||
--> src/mdtest_snippet.py:29:5
|
||||
|
|
||||
28 | class Employee(TypedDict):
|
||||
29 | id: ReadOnly[int]
|
||||
| ----------------- Read-only item declared here
|
||||
30 | name: str
|
||||
|
|
||||
info: rule `invalid-assignment` is enabled by default
|
||||
|
||||
```
|
||||
|
|
|
@ -486,6 +486,9 @@ type DeclaredTypeAndConflictingTypes<'db> = (
|
|||
pub(crate) struct PlaceFromDeclarationsResult<'db> {
|
||||
place_and_quals: PlaceAndQualifiers<'db>,
|
||||
conflicting_types: Option<Box<indexmap::set::Slice<Type<'db>>>>,
|
||||
/// Contains `Some(declaration)` if the declared type originates from exactly one declaration.
|
||||
/// This field is used for backreferences in diagnostics.
|
||||
pub(crate) single_declaration: Option<Definition<'db>>,
|
||||
}
|
||||
|
||||
impl<'db> PlaceFromDeclarationsResult<'db> {
|
||||
|
@ -496,6 +499,7 @@ impl<'db> PlaceFromDeclarationsResult<'db> {
|
|||
PlaceFromDeclarationsResult {
|
||||
place_and_quals,
|
||||
conflicting_types: Some(conflicting_types),
|
||||
single_declaration: None,
|
||||
}
|
||||
}
|
||||
|
||||
|
@ -513,21 +517,6 @@ impl<'db> PlaceFromDeclarationsResult<'db> {
|
|||
}
|
||||
}
|
||||
|
||||
impl<'db> From<PlaceAndQualifiers<'db>> for PlaceFromDeclarationsResult<'db> {
|
||||
fn from(place_and_quals: PlaceAndQualifiers<'db>) -> Self {
|
||||
PlaceFromDeclarationsResult {
|
||||
place_and_quals,
|
||||
conflicting_types: None,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl<'db> From<Place<'db>> for PlaceFromDeclarationsResult<'db> {
|
||||
fn from(place: Place<'db>) -> Self {
|
||||
PlaceFromDeclarationsResult::from(PlaceAndQualifiers::from(place))
|
||||
}
|
||||
}
|
||||
|
||||
/// A type with declaredness information, and a set of type qualifiers.
|
||||
///
|
||||
/// This is used to represent the result of looking up the declared type. Consider this
|
||||
|
@ -1216,6 +1205,8 @@ fn place_from_declarations_impl<'db>(
|
|||
let reachability_constraints = declarations.reachability_constraints;
|
||||
let boundness_analysis = declarations.boundness_analysis;
|
||||
let mut declarations = declarations.peekable();
|
||||
let mut first_declaration = None;
|
||||
let mut exactly_one_declaration = false;
|
||||
|
||||
let is_non_exported = |declaration: Definition<'db>| {
|
||||
requires_explicit_reexport.is_yes() && !is_reexported(db, declaration)
|
||||
|
@ -1246,6 +1237,13 @@ fn place_from_declarations_impl<'db>(
|
|||
return None;
|
||||
}
|
||||
|
||||
if first_declaration.is_none() {
|
||||
first_declaration = Some(declaration);
|
||||
exactly_one_declaration = true;
|
||||
} else {
|
||||
exactly_one_declaration = false;
|
||||
}
|
||||
|
||||
let static_reachability =
|
||||
reachability_constraints.evaluate(db, predicates, reachability_constraint);
|
||||
|
||||
|
@ -1302,10 +1300,18 @@ fn place_from_declarations_impl<'db>(
|
|||
if let Some(conflicting) = conflicting {
|
||||
PlaceFromDeclarationsResult::conflict(place_and_quals, conflicting)
|
||||
} else {
|
||||
place_and_quals.into()
|
||||
PlaceFromDeclarationsResult {
|
||||
place_and_quals,
|
||||
conflicting_types: None,
|
||||
single_declaration: first_declaration.filter(|_| exactly_one_declaration),
|
||||
}
|
||||
}
|
||||
} else {
|
||||
Place::Unbound.into()
|
||||
PlaceFromDeclarationsResult {
|
||||
place_and_quals: Place::Unbound.into(),
|
||||
conflicting_types: None,
|
||||
single_declaration: None,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
@ -9,12 +9,10 @@ use super::{
|
|||
use crate::FxOrderMap;
|
||||
use crate::module_resolver::KnownModule;
|
||||
use crate::semantic_index::definition::{Definition, DefinitionState};
|
||||
use crate::semantic_index::place::ScopedPlaceId;
|
||||
use crate::semantic_index::scope::NodeWithScopeKind;
|
||||
use crate::semantic_index::symbol::Symbol;
|
||||
use crate::semantic_index::{
|
||||
BindingWithConstraints, DeclarationWithConstraint, SemanticIndex, attribute_declarations,
|
||||
attribute_scopes,
|
||||
DeclarationWithConstraint, SemanticIndex, attribute_declarations, attribute_scopes,
|
||||
};
|
||||
use crate::types::constraints::{ConstraintSet, Constraints, IteratorConstraintsExtension};
|
||||
use crate::types::context::InferContext;
|
||||
|
@ -1283,6 +1281,9 @@ pub(crate) struct Field<'db> {
|
|||
pub(crate) declared_ty: Type<'db>,
|
||||
/// Kind-specific metadata for this field
|
||||
pub(crate) kind: FieldKind<'db>,
|
||||
/// The original declaration of this field, if there is exactly one.
|
||||
/// This field is used for backreferences in diagnostics.
|
||||
pub(crate) single_declaration: Option<Definition<'db>>,
|
||||
}
|
||||
|
||||
impl Field<'_> {
|
||||
|
@ -2666,7 +2667,9 @@ impl<'db> ClassLiteral<'db> {
|
|||
|
||||
let symbol = table.symbol(symbol_id);
|
||||
|
||||
let attr = place_from_declarations(db, declarations).ignore_conflicting_declarations();
|
||||
let result = place_from_declarations(db, declarations.clone());
|
||||
let single_declaration = result.single_declaration;
|
||||
let attr = result.ignore_conflicting_declarations();
|
||||
if attr.is_class_var() {
|
||||
continue;
|
||||
}
|
||||
|
@ -2728,6 +2731,7 @@ impl<'db> ClassLiteral<'db> {
|
|||
let mut field = Field {
|
||||
declared_ty: attr_ty.apply_optional_specialization(db, specialization),
|
||||
kind,
|
||||
single_declaration,
|
||||
};
|
||||
|
||||
// Check if this is a KW_ONLY sentinel and mark subsequent fields as keyword-only
|
||||
|
@ -3330,54 +3334,6 @@ impl<'db> ClassLiteral<'db> {
|
|||
.unwrap_or_else(|| class_name.end()),
|
||||
)
|
||||
}
|
||||
|
||||
pub(super) fn declarations_of_name(
|
||||
self,
|
||||
db: &'db dyn Db,
|
||||
name: &str,
|
||||
index: &'db SemanticIndex<'db>,
|
||||
) -> Option<impl Iterator<Item = DeclarationWithConstraint<'db>>> {
|
||||
let class_body_scope = self.body_scope(db).file_scope_id(db);
|
||||
let symbol_id = index.place_table(class_body_scope).symbol_id(name)?;
|
||||
let use_def = index.use_def_map(class_body_scope);
|
||||
Some(use_def.end_of_scope_declarations(ScopedPlaceId::Symbol(symbol_id)))
|
||||
}
|
||||
|
||||
pub(super) fn first_declaration_of_name(
|
||||
self,
|
||||
db: &'db dyn Db,
|
||||
name: &str,
|
||||
index: &'db SemanticIndex<'db>,
|
||||
) -> Option<DeclarationWithConstraint<'db>> {
|
||||
self.declarations_of_name(db, name, index)
|
||||
.into_iter()
|
||||
.flatten()
|
||||
.next()
|
||||
}
|
||||
|
||||
pub(super) fn bindings_of_name(
|
||||
self,
|
||||
db: &'db dyn Db,
|
||||
name: &str,
|
||||
index: &'db SemanticIndex<'db>,
|
||||
) -> Option<impl Iterator<Item = BindingWithConstraints<'db, 'db>>> {
|
||||
let class_body_scope = self.body_scope(db).file_scope_id(db);
|
||||
let symbol_id = index.place_table(class_body_scope).symbol_id(name)?;
|
||||
let use_def = index.use_def_map(class_body_scope);
|
||||
Some(use_def.end_of_scope_bindings(ScopedPlaceId::Symbol(symbol_id)))
|
||||
}
|
||||
|
||||
pub(super) fn first_binding_of_name(
|
||||
self,
|
||||
db: &'db dyn Db,
|
||||
name: &str,
|
||||
index: &'db SemanticIndex<'db>,
|
||||
) -> Option<BindingWithConstraints<'db, 'db>> {
|
||||
self.bindings_of_name(db, name, index)
|
||||
.into_iter()
|
||||
.flatten()
|
||||
.next()
|
||||
}
|
||||
}
|
||||
|
||||
impl<'db> From<ClassLiteral<'db>> for Type<'db> {
|
||||
|
|
|
@ -6,7 +6,6 @@ use super::{
|
|||
add_inferred_python_version_hint_to_diagnostic,
|
||||
};
|
||||
use crate::lint::{Level, LintRegistryBuilder, LintStatus};
|
||||
use crate::semantic_index::SemanticIndex;
|
||||
use crate::semantic_index::definition::Definition;
|
||||
use crate::semantic_index::place::{PlaceTable, ScopedPlaceId};
|
||||
use crate::suppression::FileSuppressionId;
|
||||
|
@ -2887,16 +2886,13 @@ pub(crate) fn report_invalid_key_on_typed_dict<'db>(
|
|||
pub(super) fn report_namedtuple_field_without_default_after_field_with_default<'db>(
|
||||
context: &InferContext<'db, '_>,
|
||||
class: ClassLiteral<'db>,
|
||||
index: &'db SemanticIndex<'db>,
|
||||
field_name: &str,
|
||||
field_with_default: &str,
|
||||
(field, field_def): &(Name, Option<Definition<'db>>),
|
||||
(field_with_default, field_with_default_def): &(Name, Option<Definition<'db>>),
|
||||
) {
|
||||
let db = context.db();
|
||||
let module = context.module();
|
||||
|
||||
let diagnostic_range = class
|
||||
.first_declaration_of_name(db, field_name, index)
|
||||
.and_then(|definition| definition.declaration.definition())
|
||||
let diagnostic_range = field_def
|
||||
.map(|definition| definition.kind(db).full_range(module))
|
||||
.unwrap_or_else(|| class.header_range(db));
|
||||
|
||||
|
@ -2908,13 +2904,11 @@ pub(super) fn report_namedtuple_field_without_default_after_field_with_default<'
|
|||
));
|
||||
|
||||
diagnostic.set_primary_message(format_args!(
|
||||
"Field `{field_name}` defined here without a default value"
|
||||
"Field `{field}` defined here without a default value",
|
||||
));
|
||||
|
||||
let Some(field_with_default_range) = class
|
||||
.first_binding_of_name(db, field_with_default, index)
|
||||
.and_then(|definition| definition.binding.definition())
|
||||
.map(|definition| definition.kind(db).full_range(module))
|
||||
let Some(field_with_default_range) =
|
||||
field_with_default_def.map(|definition| definition.kind(db).full_range(module))
|
||||
else {
|
||||
return;
|
||||
};
|
||||
|
@ -2933,7 +2927,7 @@ pub(super) fn report_namedtuple_field_without_default_after_field_with_default<'
|
|||
);
|
||||
} else {
|
||||
diagnostic.info(format_args!(
|
||||
"Earlier field `{field_with_default}` was defined with a default value"
|
||||
"Earlier field `{field_with_default}` was defined with a default value",
|
||||
));
|
||||
}
|
||||
}
|
||||
|
|
|
@ -1191,14 +1191,14 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
|
|||
default_ty: Some(_)
|
||||
}
|
||||
) {
|
||||
field_with_default_encountered = Some(field_name);
|
||||
field_with_default_encountered =
|
||||
Some((field_name, field.single_declaration));
|
||||
} else if let Some(field_with_default) = field_with_default_encountered.as_ref()
|
||||
{
|
||||
report_namedtuple_field_without_default_after_field_with_default(
|
||||
&self.context,
|
||||
class,
|
||||
self.index,
|
||||
&field_name,
|
||||
&(field_name, field.single_declaration),
|
||||
field_with_default,
|
||||
);
|
||||
}
|
||||
|
|
|
@ -1,6 +1,9 @@
|
|||
use bitflags::bitflags;
|
||||
use ruff_db::diagnostic::{Annotation, Diagnostic, Span, SubDiagnostic, SubDiagnosticSeverity};
|
||||
use ruff_db::parsed::parsed_module;
|
||||
use ruff_python_ast::Arguments;
|
||||
use ruff_python_ast::{self as ast, AnyNodeRef, StmtClassDef, name::Name};
|
||||
use ruff_text_size::Ranged;
|
||||
|
||||
use super::class::{ClassType, CodeGeneratorKind, Field};
|
||||
use super::context::InferContext;
|
||||
|
@ -157,6 +160,22 @@ pub(super) fn validate_typed_dict_key_assignment<'db, 'ast>(
|
|||
return false;
|
||||
};
|
||||
|
||||
let add_item_definition_subdiagnostic = |diagnostic: &mut Diagnostic, message| {
|
||||
if let Some(declaration) = item.single_declaration {
|
||||
let file = declaration.file(db);
|
||||
let module = parsed_module(db, file).load(db);
|
||||
|
||||
let mut sub = SubDiagnostic::new(SubDiagnosticSeverity::Info, "Item declaration");
|
||||
sub.annotate(
|
||||
Annotation::secondary(
|
||||
Span::from(file).with_range(declaration.full_range(db, &module).range()),
|
||||
)
|
||||
.message(message),
|
||||
);
|
||||
diagnostic.sub(sub);
|
||||
}
|
||||
};
|
||||
|
||||
if assignment_kind.is_subscript() && item.is_read_only() {
|
||||
if let Some(builder) =
|
||||
context.report_lint(assignment_kind.diagnostic_type(), key_node.into())
|
||||
|
@ -175,6 +194,8 @@ pub(super) fn validate_typed_dict_key_assignment<'db, 'ast>(
|
|||
.secondary(typed_dict_node.into())
|
||||
.message(format_args!("TypedDict `{typed_dict_d}`")),
|
||||
);
|
||||
|
||||
add_item_definition_subdiagnostic(&mut diagnostic, "Read-only item declared here");
|
||||
}
|
||||
|
||||
return false;
|
||||
|
@ -211,6 +232,8 @@ pub(super) fn validate_typed_dict_key_assignment<'db, 'ast>(
|
|||
.secondary(key_node.into())
|
||||
.message(format_args!("key has declared type `{item_type_d}`")),
|
||||
);
|
||||
|
||||
add_item_definition_subdiagnostic(&mut diagnostic, "Item declared here");
|
||||
}
|
||||
|
||||
false
|
||||
|
|
Loading…
Add table
Add a link
Reference in a new issue