Add instance-var-assign diagnostic for class-object writes (#3750)

Assigning to an instance variable via the class (`C.instance_var = 3`,
where the class body declares `instance_var: int` without `ClassVar`)
silently succeeded, even though such fields are meant to be assigned on
instances — the mirror image of the existing rule that rejects instance
writes to a ClassVar.

Add a top-level, addressable ErrorKind::InstanceVarAssign, off by default
(Severity::Ignore) and enabled in strict mode (Preset::Strict), so default
checking behavior is unchanged (matching ty) while the diagnostic remains
independently suppressible via `# pyrefly: ignore[instance-var-assign]`.

Detection keys off `annotation.is_some() && !is_classvar`, threaded as a
provenance flag on `ClassAttribute::ReadWrite(Type, bool)` from the two
ClassAttribute conversion sites to the single attribute-write chokepoint,
which alone knows whether the write goes through the class object rather
than an instance. The error is emitted before the type-compat check, so
an ill-typed value reports both this kind and bad-assignment.

Author: nitishagar

crates/pyrefly_config/src/base.rs

@@ -179,6 +179,7 @@ impl Preset {
                 let errors = HashMap::from([
                     (ErrorKind::DirectAbstractBaseInstantiation, Severity::Error),
                     (ErrorKind::ImplicitAny, Severity::Error),
+                    (ErrorKind::InstanceVarAssign, Severity::Error),
                     (ErrorKind::MissingOverrideDecorator, Severity::Error),
                     (ErrorKind::OpenUnpacking, Severity::Error),
                     (ErrorKind::PotentialBadKeywordArgument, Severity::Error),

crates/pyrefly_config/src/error_kind.rs

@@ -228,6 +228,8 @@ pub enum ErrorKind {
     /// An inconsistency between a function parameter's type in an overload signature and its
     /// default value in the implementation.
     InconsistentOverloadDefault,
+    /// Cannot assign to a class-body-declared instance variable via the class (e.g. `C.instance_var = ...`).
+    InstanceVarAssign,
     /// Internal Pyrefly error.
     InternalError,
     /// An `@abstractmethod` is defined in a class that is not abstract.
@@ -563,6 +565,7 @@ impl ErrorKind {
             ErrorKind::ImplicitReexport => Severity::Ignore,
             ErrorKind::ImplicitlyDefinedAttribute => Severity::Ignore,
             ErrorKind::IncompatibleComparison => Severity::Ignore,
+            ErrorKind::InstanceVarAssign => Severity::Ignore,
             ErrorKind::InvalidAbstractMethod => Severity::Ignore,
             ErrorKind::InvalidCast => Severity::Ignore,
             ErrorKind::InvalidDecorator => Severity::Warn,

pyrefly/lib/alt/attr.rs

@@ -1350,7 +1350,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
             return None;
         };
         let ty = match class_attr {
-            ClassAttribute::ReadWrite(ty) | ClassAttribute::ReadOnly(ty, _) => ty,
+            ClassAttribute::ReadWrite(ty, _) | ClassAttribute::ReadOnly(ty, _) => ty,
             _ => return None,
         };
         let Type::BoundMethod(bound_method) = ty else {
@@ -1373,7 +1373,9 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
         new_bound_method.func = BoundMethodType::Overload(filtered_overload);
         let new_method = self.heap.mk_bound_method(new_bound_method);
         Some(Attribute::ClassAttribute(match class_attr {
-            ClassAttribute::ReadWrite(_) => ClassAttribute::ReadWrite(new_method),
+            ClassAttribute::ReadWrite(_, is_instance_var) => {
+                ClassAttribute::ReadWrite(new_method, *is_instance_var)
+            }
             ClassAttribute::ReadOnly(_, reason) => {
                 ClassAttribute::ReadOnly(new_method, reason.clone())
             }
@@ -3122,7 +3124,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
                     .into_iter()
                     .filter_map(|(attr, _)| {
                         match &attr {
-                            Attribute::ClassAttribute(ClassAttribute::ReadWrite(ty))
+                            Attribute::ClassAttribute(ClassAttribute::ReadWrite(ty, _))
                             | Attribute::ClassAttribute(ClassAttribute::ReadOnly(ty, _))
                             | Attribute::Simple(ty)
                             | Attribute::ClassAttribute(ClassAttribute::Property(ty, _, _))

pyrefly/lib/alt/class/class_field.rs

@@ -118,7 +118,9 @@ use crate::types::types::Union;
 #[derive(Debug, Clone)]
 pub enum ClassAttribute {
     /// A read-write attribute with a closed form type for both get and set actions.
-    ReadWrite(Type),
+    /// The flag is `true` when the attribute is a class-body-declared instance
+    /// variable (annotated, not `ClassVar`), used to flag writes via the class.
+    ReadWrite(Type, bool),
     /// A read-only attribute with a closed form type for get actions.
     ReadOnly(Type, ReadOnlyReason),
     /// A `NoAccess` attribute indicates that the attribute is well-defined, but does
@@ -143,7 +145,13 @@ pub enum ClassAttribute {
 
 impl ClassAttribute {
     pub fn read_write(ty: Type) -> Self {
-        Self::ReadWrite(ty)
+        Self::ReadWrite(ty, false)
+    }
+
+    /// Like [`read_write`](Self::read_write) but marks the attribute as a
+    /// class-body-declared instance variable (annotated, not `ClassVar`).
+    pub fn read_write_instance_var(ty: Type) -> Self {
+        Self::ReadWrite(ty, true)
     }
 
     pub fn read_only(ty: Type, reason: ReadOnlyReason) -> Self {
@@ -183,7 +191,7 @@ impl ClassAttribute {
 
     pub fn read_only_equivalent(self, reason: ReadOnlyReason) -> Self {
         match self {
-            Self::ReadWrite(ty) => Self::ReadOnly(ty, reason),
+            Self::ReadWrite(ty, _) => Self::ReadOnly(ty, reason),
             Self::Property(getter, _, cls) => Self::Property(getter, None, cls),
             Self::Descriptor(descriptor, base) => Self::Descriptor(
                 Descriptor {
@@ -215,7 +223,7 @@ impl ClassAttribute {
         match self {
             // TODO(stroxler): ReadWrite attributes are not actually methods but limiting access to
             // ReadOnly breaks unit tests; we should investigate callsites to understand this better.
-            ClassAttribute::ReadWrite(ty) | ClassAttribute::ReadOnly(ty, _) => Some(ty),
+            ClassAttribute::ReadWrite(ty, _) | ClassAttribute::ReadOnly(ty, _) => Some(ty),
             ClassAttribute::NoAccess(..)
             | ClassAttribute::Property(..)
             | ClassAttribute::Descriptor(..)
@@ -1289,10 +1297,13 @@ fn bind_class_attribute(
     cls: &ClassBase,
     attr: Type,
     read_only_reason: Option<ReadOnlyReason>,
+    is_instance_var: bool,
 ) -> ClassAttribute {
     let ty = make_bound_classmethod(heap, cls, attr).into_inner();
     if let Some(reason) = read_only_reason {
         ClassAttribute::read_only(ty, reason)
+    } else if is_instance_var {
+        ClassAttribute::read_write_instance_var(ty)
     } else {
         ClassAttribute::read_write(ty)
     }
@@ -3569,12 +3580,16 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
             }
             ClassFieldInner::ClassAttribute {
                 mut ty,
+                annotation,
                 is_classvar,
                 read_only_reason,
                 descriptor_range,
                 ..
             } => {
                 ty = self.normalize_attr_ty(ty);
+                // A class-body-declared instance variable (annotated, not `ClassVar`);
+                // writing it via the class object is flagged at the set chokepoint.
+                let is_instance_var = annotation.is_some() && !is_classvar;
                 let read_only_reason = if is_classvar {
                     Some(ReadOnlyReason::ClassVar)
                 } else {
@@ -3599,6 +3614,8 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
                     (_, ty) => {
                         if let Some(reason) = read_only_reason {
                             ClassAttribute::read_only(ty, reason)
+                        } else if is_instance_var {
+                            ClassAttribute::read_write_instance_var(ty)
                         } else {
                             ClassAttribute::read_write(ty)
                         }
@@ -3654,7 +3671,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
             ClassFieldInner::Property { mut ty, .. } => {
                 // When accessing a property on a class (not instance), you get the property object itself
                 ty = self.normalize_attr_ty(ty);
-                bind_class_attribute(self.heap, cls, ty, None)
+                bind_class_attribute(self.heap, cls, ty, None, false)
             }
             ClassFieldInner::Descriptor { descriptor, .. } => {
                 ClassAttribute::descriptor(descriptor, DescriptorBase::ClassDef(cls.clone()))
@@ -3676,7 +3693,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
                 if let Some(quantified) = self_quantified {
                     ty = self.wrap_with_quantified(ty, quantified);
                 }
-                bind_class_attribute(self.heap, cls, ty, None)
+                bind_class_attribute(self.heap, cls, ty, None, false)
             }
             ClassFieldInner::ProxyMethod { .. } => ClassAttribute::no_access(
                 NoAccessReason::ProxyMethodClassAccess(cls.class_object().dupe()),
@@ -3689,6 +3706,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
                     cls,
                     ty,
                     Some(ReadOnlyReason::ClassObjectInitializedOnBody),
+                    false,
                 )
             }
             ClassFieldInner::ClassAttribute {
@@ -3699,11 +3717,16 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
             )),
             ClassFieldInner::ClassAttribute {
                 mut ty,
+                annotation,
+                is_classvar,
                 read_only_reason,
                 descriptor_range,
                 ..
             } => {
                 ty = self.normalize_attr_ty(ty);
+                // A class-body-declared instance variable (annotated, not `ClassVar`);
+                // writing it via the class object is flagged at the set chokepoint.
+                let is_instance_var = annotation.is_some() && !is_classvar;
                 if ambiguous && !matches!(cls, ClassBase::SelfType(_)) {
                     ClassAttribute::no_access(NoAccessReason::ClassAttributeIsGeneric(
                         cls.class_object().dupe(),
@@ -3718,7 +3741,13 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
                             base: DescriptorBase::ClassDef(cls.clone()),
                             read_only_reason,
                         },
-                        (_, ty) => bind_class_attribute(self.heap, cls, ty, read_only_reason),
+                        (_, ty) => bind_class_attribute(
+                            self.heap,
+                            cls,
+                            ty,
+                            read_only_reason,
+                            is_instance_var,
+                        ),
                     }
                 }
             }
@@ -4242,7 +4271,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
                     });
                 };
                 match &mut attr {
-                    ClassAttribute::ReadOnly(ty, _) | ClassAttribute::ReadWrite(ty)
+                    ClassAttribute::ReadOnly(ty, _) | ClassAttribute::ReadWrite(ty, _)
                         if ty.has_toplevel_func_metadata() =>
                     {
                         relax_ty(ty);
@@ -5338,7 +5367,26 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
                     *should_narrow = false;
                 }
             }
-            ClassAttribute::ReadWrite(attr_ty) => {
+            ClassAttribute::ReadWrite(attr_ty, is_instance_var) => {
+                // Writing a class-body-declared instance variable via the class object
+                // misuses it (it should be assigned on instances). Emitted before the
+                // type-compat check below so this kind and bad-assignment fire on
+                // separate paths, and before the narrowing decision so the check is
+                // purely additive.
+                if is_instance_var && instance_class.is_none() && class_base.is_some() {
+                    errors
+                        .error_builder(
+                            range,
+                            ErrorKind::InstanceVarAssign,
+                            format!("Cannot set instance variable `{attr_name}` from the class"),
+                        )
+                        .with_detail(
+                            "instance variables declared in a class body (without ClassVar) should be assigned on instances"
+                                .to_owned(),
+                        )
+                        .emit();
+                    *should_narrow = false;
+                }
                 // If the attribute has a converter, then `want` should be the type expected by the converter.
                 let attr_ty = match instance_class {
                     Some(cls) => match self.get_dataclass_member(cls.class_object(), attr_name) {
@@ -5553,7 +5601,9 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
             }
         };
         match attr {
-            ClassAttribute::ReadWrite(ty) => Some(ClassAttribute::ReadWrite(filter_type(ty)?)),
+            ClassAttribute::ReadWrite(ty, is_instance_var) => {
+                Some(ClassAttribute::ReadWrite(filter_type(ty)?, is_instance_var))
+            }
             ClassAttribute::ReadOnly(ty, reason) => {
                 Some(ClassAttribute::ReadOnly(filter_type(ty)?, reason))
             }
@@ -5607,13 +5657,13 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
             ) => Err(Box::new(AttrSubsetError::Property)),
             (
                 ClassAttribute::ReadOnly(..),
-                ClassAttribute::Property(_, Some(_), _) | ClassAttribute::ReadWrite(_),
+                ClassAttribute::Property(_, Some(_), _) | ClassAttribute::ReadWrite(..),
             ) => Err(Box::new(AttrSubsetError::ReadOnly)),
             (
                 // TODO(stroxler): Investigate this case more: methods should be ReadOnly, but
                 // in some cases for unknown reasons they wind up being ReadWrite.
-                ClassAttribute::ReadWrite(got),
-                ClassAttribute::ReadWrite(want),
+                ClassAttribute::ReadWrite(got, _),
+                ClassAttribute::ReadWrite(want, _),
             ) if got.has_toplevel_func_metadata() && want.has_toplevel_func_metadata() => {
                 is_subset(got, want).map_err(|subset_error| {
                     Box::new(AttrSubsetError::Covariant {
@@ -5625,7 +5675,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
                     })
                 })
             }
-            (ClassAttribute::ReadWrite(got), ClassAttribute::ReadWrite(want)) => {
+            (ClassAttribute::ReadWrite(got, _), ClassAttribute::ReadWrite(want, _)) => {
                 let subset_error = is_subset(got, want)
                     .map_or_else(Some, |_| is_subset(want, got).map_or_else(Some, |_| None));
                 if let Some(subset_error) = subset_error {
@@ -5639,7 +5689,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
                 }
             }
             (
-                ClassAttribute::ReadWrite(got) | ClassAttribute::ReadOnly(got, ..),
+                ClassAttribute::ReadWrite(got, _) | ClassAttribute::ReadOnly(got, ..),
                 ClassAttribute::ReadOnly(want, _),
             ) => is_subset(got, want).map_err(|subset_error| {
                 Box::new(AttrSubsetError::Covariant {
@@ -5666,7 +5716,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
                     })
                 })
             }
-            (ClassAttribute::ReadWrite(got), ClassAttribute::Property(want, want_setter, _)) => {
+            (ClassAttribute::ReadWrite(got, _), ClassAttribute::Property(want, want_setter, _)) => {
                 is_subset(
                     // Synthesize a getter method
                     &self.heap.mk_callable_ellipsis(got.clone()),
@@ -5767,7 +5817,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
     ) -> Result<Type, NoAccessReason> {
         match class_attr {
             ClassAttribute::NoAccess(reason) => Err(reason),
-            ClassAttribute::ReadWrite(ty) | ClassAttribute::ReadOnly(ty, _) => Ok(ty),
+            ClassAttribute::ReadWrite(ty, _) | ClassAttribute::ReadOnly(ty, _) => Ok(ty),
             ClassAttribute::Property(getter, ..) => {
                 self.record_property_getter(range, &getter);
                 Ok(self.call_property_getter(getter, range, errors, context))