mirror of
https://github.com/astral-sh/ruff.git
synced 2025-09-29 13:24:57 +00:00
[ty] Fix __setattr__ call check precedence during attribute assignment (#18347)
Some checks are pending
CI / cargo build (msrv) (push) Blocked by required conditions
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 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 / 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 (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 / cargo build (msrv) (push) Blocked by required conditions
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 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 / 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 (push) Blocked by required conditions
CI / benchmarks-walltime (push) Blocked by required conditions
[ty Playground] Release / publish (push) Waiting to run
## Summary Related: - https://github.com/astral-sh/ty/issues/111 - https://github.com/astral-sh/ruff/pull/17974#discussion_r2108527106 Previously, when validating an attribute assignment, a `__setattr__` call check was only done if the attribute wasn't found as either a class member or instance member This PR changes the `__setattr__` call check to be attempted first, prior to the "[normal mechanism](https://docs.python.org/3/reference/datamodel.html#object.__setattr__)", as a defined `__setattr__` should take precedence over setting an attribute on the instance dictionary directly. if the return type of `__setattr__` is `Never`, an `invalid-assignment` diagnostic is emitted Once this is merged, a subsequent PR will synthesize a `__setattr__` method with a `Never` return type for frozen dataclasses. ## Test Plan Existing tests + mypy_primer --------- Co-authored-by: David Peter <mail@david-peter.de>
This commit is contained in:
justin
GitHub
parent
9a4b85d845
commit
738692baff
2 changed files with 231 additions and 131 deletions
|
|
@ -1791,6 +1791,80 @@ date.year = 2025
|
|||
date.tz = "UTC"
|
||||
```
|
||||
|
||||
### Return type of `__setattr__`
|
||||
|
||||
If the return type of the `__setattr__` method is `Never`, we do not allow any attribute assignments
|
||||
on instances of that class:
|
||||
|
||||
```py
|
||||
from typing_extensions import Never
|
||||
|
||||
class Frozen:
|
||||
existing: int = 1
|
||||
|
||||
def __setattr__(self, name, value) -> Never:
|
||||
raise AttributeError("Attributes can not be modified")
|
||||
|
||||
instance = Frozen()
|
||||
instance.non_existing = 2 # error: [invalid-assignment] "Cannot assign to attribute `non_existing` on type `Frozen` whose `__setattr__` method returns `Never`/`NoReturn`"
|
||||
instance.existing = 2 # error: [invalid-assignment] "Cannot assign to attribute `existing` on type `Frozen` whose `__setattr__` method returns `Never`/`NoReturn`"
|
||||
```
|
||||
|
||||
### `__setattr__` on `object`
|
||||
|
||||
`object` has a custom `__setattr__` implementation, but we still emit an error if a non-existing
|
||||
attribute is assigned on an `object` instance.
|
||||
|
||||
```py
|
||||
obj = object()
|
||||
obj.non_existing = 1 # error: [unresolved-attribute]
|
||||
```
|
||||
|
||||
### Setting attributes on `Never` / `Any`
|
||||
|
||||
Setting attributes on `Never` itself should be allowed (even though it has a `__setattr__` attribute
|
||||
of type `Never`):
|
||||
|
||||
```py
|
||||
from typing_extensions import Never, Any
|
||||
|
||||
def _(n: Never):
|
||||
reveal_type(n.__setattr__) # revealed: Never
|
||||
|
||||
# No error:
|
||||
n.non_existing = 1
|
||||
```
|
||||
|
||||
And similarly for `Any`:
|
||||
|
||||
```py
|
||||
def _(a: Any):
|
||||
reveal_type(a.__setattr__) # revealed: Any
|
||||
|
||||
# No error:
|
||||
a.non_existing = 1
|
||||
```
|
||||
|
||||
### Possibly unbound `__setattr__` method
|
||||
|
||||
If a `__setattr__` method is only partially bound, the behavior is still the same:
|
||||
|
||||
```py
|
||||
from typing_extensions import Never
|
||||
|
||||
def flag() -> bool:
|
||||
return True
|
||||
|
||||
class Frozen:
|
||||
if flag():
|
||||
def __setattr__(self, name, value) -> Never:
|
||||
raise AttributeError("Attributes can not be modified")
|
||||
|
||||
instance = Frozen()
|
||||
instance.non_existing = 2 # error: [invalid-assignment]
|
||||
instance.existing = 2 # error: [invalid-assignment]
|
||||
```
|
||||
|
||||
### `argparse.Namespace`
|
||||
|
||||
A standard library example of a class with a custom `__setattr__` method is `argparse.Namespace`:
|
||||
|
|
|
|||
|
|
@ -3345,167 +3345,193 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
|
|||
dataclass_params.is_some_and(|params| params.contains(DataclassParams::FROZEN))
|
||||
};
|
||||
|
||||
match object_ty.class_member(db, attribute.into()) {
|
||||
meta_attr @ PlaceAndQualifiers { .. } if meta_attr.is_class_var() => {
|
||||
if emit_diagnostics {
|
||||
if let Some(builder) =
|
||||
self.context.report_lint(&INVALID_ATTRIBUTE_ACCESS, target)
|
||||
{
|
||||
builder.into_diagnostic(format_args!(
|
||||
"Cannot assign to ClassVar `{attribute}` \
|
||||
from an instance of type `{ty}`",
|
||||
ty = object_ty.display(self.db()),
|
||||
));
|
||||
}
|
||||
}
|
||||
false
|
||||
}
|
||||
PlaceAndQualifiers {
|
||||
place: Place::Type(meta_attr_ty, meta_attr_boundness),
|
||||
qualifiers: _,
|
||||
} => {
|
||||
if is_read_only() {
|
||||
// First, try to call the `__setattr__` dunder method. If this is present/defined, overrides
|
||||
// assigning the attributed by the normal mechanism.
|
||||
let setattr_dunder_call_result = object_ty.try_call_dunder_with_policy(
|
||||
db,
|
||||
"__setattr__",
|
||||
&mut CallArgumentTypes::positional([
|
||||
Type::StringLiteral(StringLiteralType::new(db, Box::from(attribute))),
|
||||
value_ty,
|
||||
]),
|
||||
MemberLookupPolicy::MRO_NO_OBJECT_FALLBACK,
|
||||
);
|
||||
|
||||
let check_setattr_return_type = |result: Bindings<'db>| -> bool {
|
||||
match result.return_type(db) {
|
||||
Type::Never => {
|
||||
if emit_diagnostics {
|
||||
if let Some(builder) =
|
||||
self.context.report_lint(&INVALID_ASSIGNMENT, target)
|
||||
{
|
||||
builder.into_diagnostic(format_args!(
|
||||
"Property `{attribute}` defined in `{ty}` is read-only",
|
||||
ty = object_ty.display(self.db()),
|
||||
));
|
||||
"Cannot assign to attribute `{attribute}` on type `{}` \
|
||||
whose `__setattr__` method returns `Never`/`NoReturn`",
|
||||
object_ty.display(db)
|
||||
));
|
||||
}
|
||||
}
|
||||
false
|
||||
} else {
|
||||
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,
|
||||
&CallArgumentTypes::positional([
|
||||
meta_attr_ty,
|
||||
object_ty,
|
||||
value_ty,
|
||||
]),
|
||||
)
|
||||
.is_ok();
|
||||
|
||||
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)
|
||||
));
|
||||
}
|
||||
}
|
||||
|
||||
successful_call
|
||||
} else {
|
||||
ensure_assignable_to(meta_attr_ty)
|
||||
};
|
||||
|
||||
let assignable_to_instance_attribute = if meta_attr_boundness
|
||||
== Boundness::PossiblyUnbound
|
||||
{
|
||||
let (assignable, boundness) =
|
||||
if let Place::Type(instance_attr_ty, instance_attr_boundness) =
|
||||
object_ty.instance_member(db, attribute).place
|
||||
{
|
||||
(
|
||||
ensure_assignable_to(instance_attr_ty),
|
||||
instance_attr_boundness,
|
||||
)
|
||||
} else {
|
||||
(true, Boundness::PossiblyUnbound)
|
||||
};
|
||||
|
||||
if boundness == Boundness::PossiblyUnbound {
|
||||
report_possibly_unbound_attribute(
|
||||
&self.context,
|
||||
target,
|
||||
attribute,
|
||||
object_ty,
|
||||
);
|
||||
}
|
||||
|
||||
assignable
|
||||
} else {
|
||||
true
|
||||
};
|
||||
|
||||
assignable_to_meta_attr && assignable_to_instance_attribute
|
||||
}
|
||||
_ => true,
|
||||
}
|
||||
};
|
||||
|
||||
PlaceAndQualifiers {
|
||||
place: Place::Unbound,
|
||||
..
|
||||
} => {
|
||||
if let Place::Type(instance_attr_ty, instance_attr_boundness) =
|
||||
object_ty.instance_member(db, attribute).place
|
||||
{
|
||||
if instance_attr_boundness == Boundness::PossiblyUnbound {
|
||||
report_possibly_unbound_attribute(
|
||||
&self.context,
|
||||
target,
|
||||
attribute,
|
||||
object_ty,
|
||||
);
|
||||
match setattr_dunder_call_result {
|
||||
Ok(result) => check_setattr_return_type(result),
|
||||
Err(CallDunderError::PossiblyUnbound(result)) => {
|
||||
check_setattr_return_type(*result)
|
||||
}
|
||||
Err(CallDunderError::CallError(..)) => {
|
||||
if emit_diagnostics {
|
||||
if let Some(builder) =
|
||||
self.context.report_lint(&UNRESOLVED_ATTRIBUTE, target)
|
||||
{
|
||||
builder.into_diagnostic(format_args!(
|
||||
"Can not assign object of type `{}` to attribute \
|
||||
`{attribute}` on type `{}` with \
|
||||
custom `__setattr__` method.",
|
||||
value_ty.display(db),
|
||||
object_ty.display(db)
|
||||
));
|
||||
}
|
||||
|
||||
if is_read_only() {
|
||||
}
|
||||
false
|
||||
}
|
||||
Err(CallDunderError::MethodNotAvailable) => {
|
||||
match object_ty.class_member(db, attribute.into()) {
|
||||
meta_attr @ PlaceAndQualifiers { .. } if meta_attr.is_class_var() => {
|
||||
if emit_diagnostics {
|
||||
if let Some(builder) =
|
||||
self.context.report_lint(&INVALID_ASSIGNMENT, target)
|
||||
self.context.report_lint(&INVALID_ATTRIBUTE_ACCESS, target)
|
||||
{
|
||||
builder.into_diagnostic(format_args!(
|
||||
"Property `{attribute}` defined in `{ty}` is read-only",
|
||||
"Cannot assign to ClassVar `{attribute}` \
|
||||
from an instance of type `{ty}`",
|
||||
ty = object_ty.display(self.db()),
|
||||
));
|
||||
}
|
||||
}
|
||||
false
|
||||
} else {
|
||||
ensure_assignable_to(instance_attr_ty)
|
||||
}
|
||||
} else {
|
||||
let result = object_ty.try_call_dunder_with_policy(
|
||||
db,
|
||||
"__setattr__",
|
||||
&mut CallArgumentTypes::positional([
|
||||
Type::StringLiteral(StringLiteralType::new(
|
||||
db,
|
||||
Box::from(attribute),
|
||||
)),
|
||||
value_ty,
|
||||
]),
|
||||
MemberLookupPolicy::MRO_NO_OBJECT_FALLBACK,
|
||||
);
|
||||
|
||||
match result {
|
||||
Ok(_) | Err(CallDunderError::PossiblyUnbound(_)) => true,
|
||||
Err(CallDunderError::CallError(..)) => {
|
||||
PlaceAndQualifiers {
|
||||
place: Place::Type(meta_attr_ty, meta_attr_boundness),
|
||||
qualifiers: _,
|
||||
} => {
|
||||
if is_read_only() {
|
||||
if emit_diagnostics {
|
||||
if let Some(builder) =
|
||||
self.context.report_lint(&UNRESOLVED_ATTRIBUTE, target)
|
||||
self.context.report_lint(&INVALID_ASSIGNMENT, target)
|
||||
{
|
||||
builder.into_diagnostic(format_args!(
|
||||
"Can not assign object of type `{}` to attribute \
|
||||
`{attribute}` on type `{}` with \
|
||||
custom `__setattr__` method.",
|
||||
value_ty.display(db),
|
||||
object_ty.display(db)
|
||||
));
|
||||
"Property `{attribute}` defined in `{ty}` is read-only",
|
||||
ty = object_ty.display(self.db()),
|
||||
));
|
||||
}
|
||||
}
|
||||
false
|
||||
} else {
|
||||
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,
|
||||
&CallArgumentTypes::positional([
|
||||
meta_attr_ty,
|
||||
object_ty,
|
||||
value_ty,
|
||||
]),
|
||||
)
|
||||
.is_ok();
|
||||
|
||||
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)
|
||||
));
|
||||
}
|
||||
}
|
||||
|
||||
successful_call
|
||||
} else {
|
||||
ensure_assignable_to(meta_attr_ty)
|
||||
};
|
||||
|
||||
let assignable_to_instance_attribute =
|
||||
if meta_attr_boundness == Boundness::PossiblyUnbound {
|
||||
let (assignable, boundness) = if let Place::Type(
|
||||
instance_attr_ty,
|
||||
instance_attr_boundness,
|
||||
) =
|
||||
object_ty.instance_member(db, attribute).place
|
||||
{
|
||||
(
|
||||
ensure_assignable_to(instance_attr_ty),
|
||||
instance_attr_boundness,
|
||||
)
|
||||
} else {
|
||||
(true, Boundness::PossiblyUnbound)
|
||||
};
|
||||
|
||||
if boundness == Boundness::PossiblyUnbound {
|
||||
report_possibly_unbound_attribute(
|
||||
&self.context,
|
||||
target,
|
||||
attribute,
|
||||
object_ty,
|
||||
);
|
||||
}
|
||||
|
||||
assignable
|
||||
} else {
|
||||
true
|
||||
};
|
||||
|
||||
assignable_to_meta_attr && assignable_to_instance_attribute
|
||||
}
|
||||
Err(CallDunderError::MethodNotAvailable) => {
|
||||
}
|
||||
|
||||
PlaceAndQualifiers {
|
||||
place: Place::Unbound,
|
||||
..
|
||||
} => {
|
||||
if let Place::Type(instance_attr_ty, instance_attr_boundness) =
|
||||
object_ty.instance_member(db, attribute).place
|
||||
{
|
||||
if instance_attr_boundness == Boundness::PossiblyUnbound {
|
||||
report_possibly_unbound_attribute(
|
||||
&self.context,
|
||||
target,
|
||||
attribute,
|
||||
object_ty,
|
||||
);
|
||||
}
|
||||
|
||||
if is_read_only() {
|
||||
if emit_diagnostics {
|
||||
if let Some(builder) = self
|
||||
.context
|
||||
.report_lint(&INVALID_ASSIGNMENT, target)
|
||||
{
|
||||
builder.into_diagnostic(format_args!(
|
||||
"Property `{attribute}` defined in `{ty}` is read-only",
|
||||
ty = object_ty.display(self.db()),
|
||||
));
|
||||
}
|
||||
}
|
||||
false
|
||||
} else {
|
||||
ensure_assignable_to(instance_attr_ty)
|
||||
}
|
||||
} else {
|
||||
if emit_diagnostics {
|
||||
if let Some(builder) =
|
||||
self.context.report_lint(&UNRESOLVED_ATTRIBUTE, target)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue