[ty] Better error message for attempting to assign to a read-only property

crates/ty_python_semantic/resources/mdtest/descriptor_protocol.md

@@ -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

crates/ty_python_semantic/resources/mdtest/named_tuple.md

@@ -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"
 ```
 

crates/ty_python_semantic/resources/mdtest/snapshots/descriptor_protocol.…_-_Descriptor_protocol_-_Special_descriptors_-_Properties_with_no_s…_(176795bc1727dda7).snap

@@ -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
+
+```

crates/ty_python_semantic/src/types/call.rs

@@ -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 {

crates/ty_python_semantic/src/types/call/bind.rs

@@ -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) {

crates/ty_python_semantic/src/types/diagnostic.rs

@@ -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,

crates/ty_python_semantic/src/types/infer.rs

@@ -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();
-
-                                        if !successful_call && emit_diagnostics {
-                                            if let Some(builder) = self
-                                                .context
-                                                .report_lint(&INVALID_ASSIGNMENT, target)
+                                        let dunder_set_result = meta_dunder_set.try_call(
+                                            db,
+                                            &CallArguments::positional([
+                                                meta_attr_ty,
+                                                object_ty,
+                                                value_ty,
+                                            ]),
+                                        );
+
+                                        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)
                         };