Skip to content

[ty] Reject using properties with Never setters or deleters#24510

Merged
charliermarsh merged 3 commits intomainfrom
charlie/deleter-iv
Apr 16, 2026
Merged

[ty] Reject using properties with Never setters or deleters#24510
charliermarsh merged 3 commits intomainfrom
charlie/deleter-iv

Conversation

@charliermarsh
Copy link
Copy Markdown
Member

Summary

We currently error if __setattr__ returns Never and the user attempts to assign to an attribute, and same for deletion. This PR extends that behavior to properties with setters and deleters.

For example, this is now forbidden:

from typing import NoReturn

class NoReturnSetter:
    @property
    def x(self) -> int:
        return 1

    @x.setter
    def x(self, value: int) -> NoReturn:
        raise RuntimeError

no_return_setter = NoReturnSetter()
# error: [invalid-assignment] "Cannot assign to attribute `x` on type `NoReturnSetter` whose `__set__` method returns `Never`/`NoReturn`"
no_return_setter.x = 1

@astral-sh-bot astral-sh-bot Bot added the ty Multi-file analysis & type inference label Apr 9, 2026
@astral-sh-bot
Copy link
Copy Markdown

astral-sh-bot Bot commented Apr 9, 2026

Typing conformance results

No changes detected ✅

Current numbers
The percentage of diagnostics emitted that were expected errors held steady at 87.94%. The percentage of expected errors that received a diagnostic held steady at 83.36%. The number of fully passing files held steady at 79/133.

@astral-sh-bot
Copy link
Copy Markdown

astral-sh-bot Bot commented Apr 9, 2026

Memory usage report

Memory usage unchanged ✅

@astral-sh-bot
Copy link
Copy Markdown

astral-sh-bot Bot commented Apr 9, 2026

ecosystem-analyzer results

Lint rule Added Removed Changed
invalid-assignment 2 0 0
Total 2 0 0

Flaky changes detected. This PR summary excludes flaky changes; see the HTML report for details.

Raw diff:

xarray (https://github.com/pydata/xarray)
+ xarray/tests/test_dataarray.py:284:13 error[invalid-assignment] Cannot assign to attribute `dims` on type `DataArray` whose `__set__` method returns `Never`/`NoReturn`
+ xarray/tests/test_variable.py:2601:13 error[invalid-assignment] Cannot assign to attribute `name` on type `IndexVariable` whose `__set__` method returns `Never`/`NoReturn`

Full report with detailed diff (timing results)

@charliermarsh charliermarsh force-pushed the charlie/deleter-iii branch 2 times, most recently from 067475b to 0955ad4 Compare April 10, 2026 21:50
@charliermarsh charliermarsh marked this pull request as ready for review April 10, 2026 21:56
@astral-sh-bot astral-sh-bot Bot requested a review from oconnor663 April 10, 2026 21:56
@AlexWaygood AlexWaygood removed their request for review April 10, 2026 21:59
@carljm carljm removed their request for review April 10, 2026 22:24
@oconnor663
Copy link
Copy Markdown
Contributor

Claude suggests that the new helper functions are kind of redundant, since we already have dunder_set_result/delete_dunder_call_result at all the callsites. This shorter approach also passes tests, though I'm not sure if it's missing anything:

diff --git i/crates/ty_python_semantic/src/types/call.rs w/crates/ty_python_semantic/src/types/call.rs
index 6a1f82da27..133481e80f 100644
--- i/crates/ty_python_semantic/src/types/call.rs
+++ w/crates/ty_python_semantic/src/types/call.rs
@@ -104,10 +104,6 @@ impl<'db> Type<'db> {
 pub(crate) struct CallError<'db>(pub(crate) CallErrorKind, pub(crate) Box<Bindings<'db>>);
 
 impl<'db> CallError<'db> {
-    pub(crate) fn return_type(&self, db: &'db dyn Db) -> Type<'db> {
-        self.1.return_type(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(
diff --git i/crates/ty_python_semantic/src/types/infer/builder.rs w/crates/ty_python_semantic/src/types/infer/builder.rs
index 7abd69073c..609c073036 100644
--- i/crates/ty_python_semantic/src/types/infer/builder.rs
+++ w/crates/ty_python_semantic/src/types/infer/builder.rs
@@ -2029,39 +2029,6 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
         }
     }
 
-    /// Returns `true` if `property_ty` is a property whose setter returns `Never`/`NoReturn`
-    /// when called for an assignment to `object_ty` with `value_ty`.
-    fn property_setter_returns_never(
-        &self,
-        property_ty: Type<'db>,
-        object_ty: Type<'db>,
-        value_ty: Type<'db>,
-    ) -> bool {
-        let db = self.db();
-        property_ty.as_property_instance().is_some_and(|property| {
-            property.setter(db).is_some_and(|setter| {
-                match setter.try_call(db, &CallArguments::positional([object_ty, value_ty])) {
-                    Ok(result) => result.return_type(db).is_never(),
-                    Err(err) => err.return_type(db).is_never(),
-                }
-            })
-        })
-    }
-
-    /// Returns `true` if `property_ty` is a property whose deleter returns `Never`/`NoReturn`
-    /// when called for deletion on `object_ty`.
-    fn property_deleter_returns_never(&self, property_ty: Type<'db>, object_ty: Type<'db>) -> bool {
-        let db = self.db();
-        property_ty.as_property_instance().is_some_and(|property| {
-            property.deleter(db).is_some_and(|deleter| {
-                match deleter.try_call(db, &CallArguments::positional([object_ty])) {
-                    Ok(result) => result.return_type(db).is_never(),
-                    Err(err) => err.return_type(db).is_never(),
-                }
-            })
-        })
-    }
-
     /// Make sure that the attribute assignment `obj.attribute = value` is valid.
     ///
     /// `target` is the node for the left-hand side, `object_ty` is the type of `obj`, `attribute` is
@@ -2375,7 +2342,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
                                 &CallArguments::positional([meta_attr_ty, object_ty, value_ty]),
                             );
 
-                            if self.property_setter_returns_never(meta_attr_ty, object_ty, value_ty)
+                            if matches!(&dunder_set_result, Ok(b) if b.return_type(db).is_never())
                             {
                                 if emit_diagnostics
                                     && let Some(builder) =
@@ -2586,7 +2553,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
                                 &CallArguments::positional([meta_attr_ty, object_ty, value_ty]),
                             );
 
-                            if self.property_setter_returns_never(meta_attr_ty, object_ty, value_ty)
+                            if matches!(&dunder_set_result, Ok(b) if b.return_type(db).is_never())
                             {
                                 if emit_diagnostics
                                     && let Some(builder) =
@@ -2933,7 +2900,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
                         TypeContext::default(),
                     );
 
-                    if self.property_deleter_returns_never(attr_ty, object_ty) {
+                    if matches!(&delete_dunder_call_result, Ok(b) if b.return_type(db).is_never()) {
                         if emit_diagnostics
                             && let Some(builder) =
                                 self.context.report_lint(&INVALID_ASSIGNMENT, target)

What do you think?

@charliermarsh
Copy link
Copy Markdown
Member Author

Looks right to me!

@charliermarsh
Copy link
Copy Markdown
Member Author

Oh, hmm... That refactor may have regressed the conformance suite.

Base automatically changed from charlie/deleter-iii to main April 16, 2026 01:00
@charliermarsh charliermarsh enabled auto-merge (squash) April 16, 2026 01:43
@charliermarsh charliermarsh merged commit e9986d8 into main Apr 16, 2026
68 of 83 checks passed
@charliermarsh charliermarsh deleted the charlie/deleter-iv branch April 16, 2026 01:44
@oconnor663
Copy link
Copy Markdown
Contributor

What was the story with the regression there?

@charliermarsh
Copy link
Copy Markdown
Member Author

We were raising a diagnostic here, in contrast to the conformance tests:

class ClassA:
    x: NoReturn
    y: list[NoReturn]

    def __init__(self, x: NoReturn, y: list[NoReturn]) -> None:
        self.x = x   # ← false positive with the refactored check
        self.y = y

I think this is somewhat debatable? But for now we're limiting to __setattr__ and properties.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants