mirror of
https://github.com/astral-sh/ruff.git
synced 2025-09-29 13:24:57 +00:00
[ty] Better error message for attempting to assign to a read-only property (#20150)
This commit is contained in:
Alex Waygood
GitHub
parent
04dc223710
commit
f77315776c
8 changed files with 247 additions and 122 deletions
|
|
@ -522,6 +522,22 @@ c.name = None
|
|||
c.name = 42
|
||||
```
|
||||
|
||||
### Properties with no setters
|
||||
|
||||
<!-- snapshot-diagnostics -->
|
||||
|
||||
If a property has no setter, we emit a bespoke error message when a user attempts to set that
|
||||
attribute, since this is a common error.
|
||||
|
||||
```py
|
||||
class DontAssignToMe:
|
||||
@property
|
||||
def immutable(self): ...
|
||||
|
||||
# error: [invalid-assignment]
|
||||
DontAssignToMe().immutable = "the properties, they are a-changing"
|
||||
```
|
||||
|
||||
### Built-in `classmethod` descriptor
|
||||
|
||||
Similarly to `property`, `classmethod` decorator creates an implicit descriptor that binds the first
|
||||
|
|
|
|||
|
|
@ -78,10 +78,7 @@ reveal_type(Person.id) # revealed: property
|
|||
reveal_type(Person.name) # revealed: property
|
||||
reveal_type(Person.age) # revealed: property
|
||||
|
||||
# TODO... the error is correct, but this is not the friendliest error message
|
||||
# for assigning to a read-only property :-)
|
||||
#
|
||||
# error: [invalid-assignment] "Invalid assignment to data descriptor attribute `id` on type `Person` with custom `__set__` method"
|
||||
# error: [invalid-assignment] "Cannot assign to read-only property `id` on object of type `Person`"
|
||||
alice.id = 42
|
||||
# error: [invalid-assignment]
|
||||
bob.age = None
|
||||
|
|
@ -221,10 +218,7 @@ james = SuperUser(0, "James", 42, "Jimmy")
|
|||
# on the subclass
|
||||
james.name = "Robert"
|
||||
|
||||
# TODO: the error is correct (can't assign to the read-only property inherited from the superclass)
|
||||
# but the error message could be friendlier :-)
|
||||
#
|
||||
# error: [invalid-assignment] "Invalid assignment to data descriptor attribute `nickname` on type `SuperUser` with custom `__set__` method"
|
||||
# error: [invalid-assignment] "Cannot assign to read-only property `nickname` on object of type `SuperUser`"
|
||||
james.nickname = "Bob"
|
||||
```
|
||||
|
||||
|
|
|
|||
|
|
@ -0,0 +1,40 @@
|
|||
---
|
||||
source: crates/ty_test/src/lib.rs
|
||||
expression: snapshot
|
||||
---
|
||||
---
|
||||
mdtest name: descriptor_protocol.md - Descriptor protocol - Special descriptors - Properties with no setters
|
||||
mdtest path: crates/ty_python_semantic/resources/mdtest/descriptor_protocol.md
|
||||
---
|
||||
|
||||
# Python source files
|
||||
|
||||
## mdtest_snippet.py
|
||||
|
||||
```
|
||||
1 | class DontAssignToMe:
|
||||
2 | @property
|
||||
3 | def immutable(self): ...
|
||||
4 |
|
||||
5 | # error: [invalid-assignment]
|
||||
6 | DontAssignToMe().immutable = "the properties, they are a-changing"
|
||||
```
|
||||
|
||||
# Diagnostics
|
||||
|
||||
```
|
||||
error[invalid-assignment]: Cannot assign to read-only property `immutable` on object of type `DontAssignToMe`
|
||||
--> src/mdtest_snippet.py:3:9
|
||||
|
|
||||
1 | class DontAssignToMe:
|
||||
2 | @property
|
||||
3 | def immutable(self): ...
|
||||
| --------- Property `DontAssignToMe.immutable` defined here with no setter
|
||||
4 |
|
||||
5 | # error: [invalid-assignment]
|
||||
6 | DontAssignToMe().immutable = "the properties, they are a-changing"
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ Attempted assignment to `DontAssignToMe.immutable` here
|
||||
|
|
||||
info: rule `invalid-assignment` is enabled by default
|
||||
|
||||
```
|
||||
|
|
@ -1,6 +1,8 @@
|
|||
use super::context::InferContext;
|
||||
use super::{Signature, Type};
|
||||
use crate::Db;
|
||||
use crate::types::PropertyInstanceType;
|
||||
use crate::types::call::bind::BindingError;
|
||||
|
||||
mod arguments;
|
||||
pub(crate) mod bind;
|
||||
|
|
@ -14,6 +16,26 @@ pub(super) use bind::{Binding, Bindings, CallableBinding, MatchedArgument};
|
|||
#[derive(Debug)]
|
||||
pub(crate) struct CallError<'db>(pub(crate) CallErrorKind, pub(crate) Box<Bindings<'db>>);
|
||||
|
||||
impl<'db> CallError<'db> {
|
||||
/// Returns `Some(property)` if the call error was caused by an attempt to set a property
|
||||
/// that has no setter, and `None` otherwise.
|
||||
pub(crate) fn as_attempt_to_set_property_with_no_setter(
|
||||
&self,
|
||||
) -> Option<PropertyInstanceType<'db>> {
|
||||
if self.0 != CallErrorKind::BindingError {
|
||||
return None;
|
||||
}
|
||||
self.1
|
||||
.into_iter()
|
||||
.flatten()
|
||||
.flat_map(bind::Binding::errors)
|
||||
.find_map(|error| match error {
|
||||
BindingError::PropertyHasNoSetter(property) => Some(*property),
|
||||
_ => None,
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
/// The reason why calling a type failed.
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
pub(crate) enum CallErrorKind {
|
||||
|
|
|
|||
|
|
@ -426,9 +426,9 @@ impl<'db> Bindings<'db> {
|
|||
overload.set_return_type(Type::unknown());
|
||||
}
|
||||
} else {
|
||||
overload.errors.push(BindingError::InternalCallError(
|
||||
"property has no getter",
|
||||
));
|
||||
overload
|
||||
.errors
|
||||
.push(BindingError::PropertyHasNoSetter(*property));
|
||||
overload.set_return_type(Type::Never);
|
||||
}
|
||||
}
|
||||
|
|
@ -482,9 +482,9 @@ impl<'db> Bindings<'db> {
|
|||
));
|
||||
}
|
||||
} else {
|
||||
overload.errors.push(BindingError::InternalCallError(
|
||||
"property has no setter",
|
||||
));
|
||||
overload
|
||||
.errors
|
||||
.push(BindingError::PropertyHasNoSetter(*property));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -500,9 +500,9 @@ impl<'db> Bindings<'db> {
|
|||
));
|
||||
}
|
||||
} else {
|
||||
overload.errors.push(BindingError::InternalCallError(
|
||||
"property has no setter",
|
||||
));
|
||||
overload
|
||||
.errors
|
||||
.push(BindingError::PropertyHasNoSetter(property));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -2593,6 +2593,10 @@ impl<'db> Binding<'db> {
|
|||
pub(crate) fn argument_matches(&self) -> &[MatchedArgument<'db>] {
|
||||
&self.argument_matches
|
||||
}
|
||||
|
||||
pub(crate) fn errors(&self) -> &[BindingError<'db>] {
|
||||
&self.errors
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Clone, Debug)]
|
||||
|
|
@ -2811,7 +2815,9 @@ pub(crate) enum BindingError<'db> {
|
|||
provided_ty: Type<'db>,
|
||||
},
|
||||
/// One or more required parameters (that is, with no default) is not supplied by any argument.
|
||||
MissingArguments { parameters: ParameterContexts },
|
||||
MissingArguments {
|
||||
parameters: ParameterContexts,
|
||||
},
|
||||
/// A call argument can't be matched to any parameter.
|
||||
UnknownArgument {
|
||||
argument_name: ast::name::Name,
|
||||
|
|
@ -2833,6 +2839,7 @@ pub(crate) enum BindingError<'db> {
|
|||
error: SpecializationError<'db>,
|
||||
argument_index: Option<usize>,
|
||||
},
|
||||
PropertyHasNoSetter(PropertyInstanceType<'db>),
|
||||
/// The call itself might be well constructed, but an error occurred while evaluating the call.
|
||||
/// We use this variant to report errors in `property.__get__` and `property.__set__`, which
|
||||
/// can occur when the call to the underlying getter/setter fails.
|
||||
|
|
@ -3099,6 +3106,17 @@ impl<'db> BindingError<'db> {
|
|||
}
|
||||
}
|
||||
|
||||
Self::PropertyHasNoSetter(_) => {
|
||||
BindingError::InternalCallError("property has no setter").report_diagnostic(
|
||||
context,
|
||||
node,
|
||||
callable_ty,
|
||||
callable_description,
|
||||
union_diag,
|
||||
matching_overload,
|
||||
);
|
||||
}
|
||||
|
||||
Self::InternalCallError(reason) => {
|
||||
let node = Self::get_node(node, None);
|
||||
if let Some(builder) = context.report_lint(&CALL_NON_CALLABLE, node) {
|
||||
|
|
|
|||
|
|
@ -10,6 +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::call::CallError;
|
||||
use crate::types::class::{DisjointBase, DisjointBaseKind, Field};
|
||||
use crate::types::function::KnownFunction;
|
||||
use crate::types::string_annotation::{
|
||||
|
|
@ -26,7 +27,7 @@ use crate::{
|
|||
Db, DisplaySettings, FxIndexMap, FxOrderMap, Module, ModuleName, Program, declare_lint,
|
||||
};
|
||||
use itertools::Itertools;
|
||||
use ruff_db::diagnostic::{Annotation, Diagnostic, SubDiagnostic, SubDiagnosticSeverity};
|
||||
use ruff_db::diagnostic::{Annotation, Diagnostic, Span, SubDiagnostic, SubDiagnosticSeverity};
|
||||
use ruff_python_ast::name::Name;
|
||||
use ruff_python_ast::{self as ast, AnyNodeRef};
|
||||
use ruff_text_size::{Ranged, TextRange};
|
||||
|
|
@ -1979,6 +1980,45 @@ pub(super) fn report_invalid_attribute_assignment(
|
|||
);
|
||||
}
|
||||
|
||||
pub(super) fn report_bad_dunder_set_call<'db>(
|
||||
context: &InferContext<'db, '_>,
|
||||
dunder_set_failure: &CallError<'db>,
|
||||
attribute: &str,
|
||||
object_type: Type<'db>,
|
||||
target: &ast::ExprAttribute,
|
||||
) {
|
||||
let Some(builder) = context.report_lint(&INVALID_ASSIGNMENT, target) else {
|
||||
return;
|
||||
};
|
||||
let db = context.db();
|
||||
if let Some(property) = dunder_set_failure.as_attempt_to_set_property_with_no_setter() {
|
||||
let object_type = object_type.display(db);
|
||||
let mut diagnostic = builder.into_diagnostic(format_args!(
|
||||
"Cannot assign to read-only property `{attribute}` on object of type `{object_type}`",
|
||||
));
|
||||
if let Some(file_range) = property
|
||||
.getter(db)
|
||||
.and_then(|getter| getter.definition(db))
|
||||
.and_then(|definition| definition.focus_range(db))
|
||||
{
|
||||
diagnostic.annotate(Annotation::secondary(Span::from(file_range)).message(
|
||||
format_args!("Property `{object_type}.{attribute}` defined here with no setter"),
|
||||
));
|
||||
diagnostic.set_primary_message(format_args!(
|
||||
"Attempted assignment to `{object_type}.{attribute}` here"
|
||||
));
|
||||
}
|
||||
} else {
|
||||
// TODO: Here, it would be nice to emit an additional diagnostic
|
||||
// that explains why the call failed
|
||||
builder.into_diagnostic(format_args!(
|
||||
"Invalid assignment to data descriptor attribute \
|
||||
`{attribute}` on type `{}` with custom `__set__` method",
|
||||
object_type.display(db)
|
||||
));
|
||||
}
|
||||
}
|
||||
|
||||
pub(super) fn report_invalid_return_type(
|
||||
context: &InferContext,
|
||||
object_range: impl Ranged,
|
||||
|
|
|
|||
|
|
@ -102,7 +102,7 @@ use crate::types::diagnostic::{
|
|||
INVALID_TYPE_VARIABLE_CONSTRAINTS, IncompatibleBases, POSSIBLY_UNBOUND_IMPLICIT_CALL,
|
||||
POSSIBLY_UNBOUND_IMPORT, TypeCheckDiagnostics, UNDEFINED_REVEAL, UNRESOLVED_ATTRIBUTE,
|
||||
UNRESOLVED_GLOBAL, UNRESOLVED_IMPORT, UNRESOLVED_REFERENCE, UNSUPPORTED_OPERATOR,
|
||||
report_implicit_return_type, report_instance_layout_conflict,
|
||||
report_bad_dunder_set_call, report_implicit_return_type, report_instance_layout_conflict,
|
||||
report_invalid_argument_number_to_special_form, report_invalid_arguments_to_annotated,
|
||||
report_invalid_arguments_to_callable, report_invalid_assignment,
|
||||
report_invalid_attribute_assignment, report_invalid_generator_function_return_type,
|
||||
|
|
@ -4217,32 +4217,30 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
|
|||
if let Place::Type(meta_dunder_set, _) =
|
||||
meta_attr_ty.class_member(db, "__set__".into()).place
|
||||
{
|
||||
let successful_call = meta_dunder_set
|
||||
.try_call(
|
||||
db,
|
||||
&CallArguments::positional([
|
||||
meta_attr_ty,
|
||||
object_ty,
|
||||
value_ty,
|
||||
]),
|
||||
)
|
||||
.is_ok();
|
||||
let dunder_set_result = meta_dunder_set.try_call(
|
||||
db,
|
||||
&CallArguments::positional([
|
||||
meta_attr_ty,
|
||||
object_ty,
|
||||
value_ty,
|
||||
]),
|
||||
);
|
||||
|
||||
if !successful_call && emit_diagnostics {
|
||||
if let Some(builder) = self
|
||||
.context
|
||||
.report_lint(&INVALID_ASSIGNMENT, target)
|
||||
if emit_diagnostics {
|
||||
if let Err(dunder_set_failure) =
|
||||
dunder_set_result.as_ref()
|
||||
{
|
||||
// TODO: Here, it would be nice to emit an additional diagnostic that explains why the call failed
|
||||
builder.into_diagnostic(format_args!(
|
||||
"Invalid assignment to data descriptor attribute \
|
||||
`{attribute}` on type `{}` with custom `__set__` method",
|
||||
object_ty.display(db)
|
||||
));
|
||||
report_bad_dunder_set_call(
|
||||
&self.context,
|
||||
dunder_set_failure,
|
||||
attribute,
|
||||
object_ty,
|
||||
target,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
successful_call
|
||||
dunder_set_result.is_ok()
|
||||
} else {
|
||||
ensure_assignable_to(meta_attr_ty)
|
||||
};
|
||||
|
|
@ -4343,27 +4341,24 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
|
|||
let assignable_to_meta_attr = if let Place::Type(meta_dunder_set, _) =
|
||||
meta_attr_ty.class_member(db, "__set__".into()).place
|
||||
{
|
||||
let successful_call = meta_dunder_set
|
||||
.try_call(
|
||||
db,
|
||||
&CallArguments::positional([meta_attr_ty, object_ty, value_ty]),
|
||||
)
|
||||
.is_ok();
|
||||
let dunder_set_result = meta_dunder_set.try_call(
|
||||
db,
|
||||
&CallArguments::positional([meta_attr_ty, object_ty, value_ty]),
|
||||
);
|
||||
|
||||
if !successful_call && emit_diagnostics {
|
||||
if let Some(builder) =
|
||||
self.context.report_lint(&INVALID_ASSIGNMENT, target)
|
||||
{
|
||||
// TODO: Here, it would be nice to emit an additional diagnostic that explains why the call failed
|
||||
builder.into_diagnostic(format_args!(
|
||||
"Invalid assignment to data descriptor attribute \
|
||||
`{attribute}` on type `{}` with custom `__set__` method",
|
||||
object_ty.display(db)
|
||||
));
|
||||
if emit_diagnostics {
|
||||
if let Err(dunder_set_failure) = dunder_set_result.as_ref() {
|
||||
report_bad_dunder_set_call(
|
||||
&self.context,
|
||||
dunder_set_failure,
|
||||
attribute,
|
||||
object_ty,
|
||||
target,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
successful_call
|
||||
dunder_set_result.is_ok()
|
||||
} else {
|
||||
ensure_assignable_to(meta_attr_ty)
|
||||
};
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue