diff --git a/crates/pyrefly_config/src/base.rs b/crates/pyrefly_config/src/base.rs index 345af53372..df5c07f3fe 100644 --- a/crates/pyrefly_config/src/base.rs +++ b/crates/pyrefly_config/src/base.rs @@ -197,6 +197,7 @@ impl Preset { let errors = HashMap::from([ (ErrorKind::DirectAbstractBaseInstantiation, Severity::Error), (ErrorKind::ImplicitAny, Severity::Error), + (ErrorKind::MethodAssign, Severity::Error), (ErrorKind::MissingOverrideDecorator, Severity::Error), (ErrorKind::OpenUnpacking, Severity::Error), (ErrorKind::PotentialBadKeywordArgument, Severity::Error), diff --git a/crates/pyrefly_config/src/config.rs b/crates/pyrefly_config/src/config.rs index c433c8e0dd..9047b9f599 100644 --- a/crates/pyrefly_config/src/config.rs +++ b/crates/pyrefly_config/src/config.rs @@ -3430,6 +3430,7 @@ output-format = "omit-errors" errors.severity(ErrorKind::MissingOverrideDecorator), Severity::Error ); + assert_eq!(errors.severity(ErrorKind::MethodAssign), Severity::Error); assert_eq!(errors.severity(ErrorKind::OpenUnpacking), Severity::Error); // Pyrefly infers concrete return types in most cases, so we don't // ask users for an explicit annotation in strict mode. diff --git a/crates/pyrefly_config/src/error_kind.rs b/crates/pyrefly_config/src/error_kind.rs index 92fe11d9c4..461a12417e 100644 --- a/crates/pyrefly_config/src/error_kind.rs +++ b/crates/pyrefly_config/src/error_kind.rs @@ -289,6 +289,8 @@ pub enum ErrorKind { /// Attempting to use `yield` in a way that is not allowed. /// e.g. `yield from` with something that's not an iterable. InvalidYield, + /// Assigning to a method on a class object or instance. + MethodAssign, /// A file-level `# pyrefly: ignore-errors` (or `ignore-errors[code]`) directive /// appears after the first line of code, where it is silently inert. File-level /// suppressions are only honored in the preamble, at the top of the file. @@ -585,6 +587,7 @@ impl ErrorKind { ErrorKind::InvalidAbstractMethod => Severity::Ignore, ErrorKind::InvalidCast => Severity::Ignore, ErrorKind::InvalidDecorator => Severity::Warn, + ErrorKind::MethodAssign => Severity::Ignore, ErrorKind::MisplacedIgnore => Severity::Warn, ErrorKind::MissingAttributePatchTarget => Severity::Warn, ErrorKind::MissingOverrideDecorator => Severity::Ignore, diff --git a/crates/pyrefly_config/src/migration/mypy/pyproject.rs b/crates/pyrefly_config/src/migration/mypy/pyproject.rs index 80d32b6ed0..88d01def4d 100644 --- a/crates/pyrefly_config/src/migration/mypy/pyproject.rs +++ b/crates/pyrefly_config/src/migration/mypy/pyproject.rs @@ -487,6 +487,31 @@ allow_redefinition = true Ok(()) } + #[test] + fn test_strict_enables_method_assign() -> anyhow::Result<()> { + let mut cfg = parse_pyproject_config("[tool.mypy]\nstrict = true\n")?; + cfg.configure(); + assert_eq!( + cfg.errors(Path::new(".")).severity(ErrorKind::MethodAssign), + Severity::Error + ); + Ok(()) + } + + #[test] + fn test_enable_method_assign_error() -> anyhow::Result<()> { + let src = r#"[tool.mypy] +enable_error_code = ["method-assign"] +"#; + let mut cfg = parse_pyproject_config(src)?; + cfg.configure(); + assert_eq!( + cfg.errors(Path::new(".")).severity(ErrorKind::MethodAssign), + Severity::Error + ); + Ok(()) + } + #[test] fn test_ignore_imports() -> anyhow::Result<()> { let src = r#"[tool.mypy] diff --git a/crates/pyrefly_config/src/migration/mypy/util.rs b/crates/pyrefly_config/src/migration/mypy/util.rs index ee078677ea..a6a3099bdd 100644 --- a/crates/pyrefly_config/src/migration/mypy/util.rs +++ b/crates/pyrefly_config/src/migration/mypy/util.rs @@ -171,6 +171,9 @@ pub fn make_error_config( if disallow_any_generics || strict { errors.insert(ErrorKind::ImplicitAny, Severity::Error); } + if strict { + errors.insert(ErrorKind::MethodAssign, Severity::Error); + } if disallow_any_explicit { errors.insert(ErrorKind::ExplicitAny, Severity::Error); } @@ -210,6 +213,7 @@ fn code_to_kind(errors: HashMap) -> HashMap add(severity, ErrorKind::MissingAttribute), "arg-type" => add(severity, ErrorKind::BadArgumentType), "assignment" => add(severity, ErrorKind::BadAssignment), + "method-assign" => add(severity, ErrorKind::MethodAssign), "call-arg" => add(severity, ErrorKind::BadArgumentCount), "call-overload" => add(severity, ErrorKind::NoMatchingOverload), "index" => { diff --git a/pyrefly/lib/alt/attr.rs b/pyrefly/lib/alt/attr.rs index d03220caa5..adfca7da4b 100644 --- a/pyrefly/lib/alt/attr.rs +++ b/pyrefly/lib/alt/attr.rs @@ -71,8 +71,9 @@ use crate::types::types::Type; /// since each union member is looked up separately. #[derive(Debug)] struct LookupResult { - /// The lookup was successful and an attribute was found. - pub found: Vec<(Attribute, AttributeBase1)>, + /// The lookup was successful and an attribute was found. The boolean records + /// whether the resolved class member is a method. + pub found: Vec<(Attribute, AttributeBase1, bool)>, /// The attribute was not found. Callers can use fallback behavior, for /// example looking up a different attribute. pub not_found: Vec, @@ -380,12 +381,17 @@ impl LookupResult { } fn found(&mut self, attr: Attribute, on: AttributeBase1) { - self.found.push((attr, on)) + self.found.push((attr, on, false)) } - fn found_class_attribute(&mut self, class_attr: ClassAttribute, on: AttributeBase1) { + fn found_class_attribute( + &mut self, + class_attr: ClassAttribute, + on: AttributeBase1, + is_method: bool, + ) { self.found - .push((Attribute::class_attribute(class_attr), on)) + .push((Attribute::class_attribute(class_attr), on, is_method)) } fn not_found(&mut self, not_found: NotFoundOn) { @@ -403,7 +409,7 @@ impl LookupResult { fn decompose( self, ) -> ( - Vec<(Attribute, AttributeBase1)>, + Vec<(Attribute, AttributeBase1, bool)>, Vec, Vec, ) { @@ -640,7 +646,7 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { if class == self.stdlib.none_type().class_object() ) }); - for (attr, _) in found { + for (attr, _, _) in found { match self.resolve_get_access(attr_name, attr, range, errors, context) { Ok(ty) => types.push(ty), Err(err) => { @@ -837,7 +843,7 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { } let suppress_errors = ErrorCollector::new(errors.module().clone(), ErrorStyle::Never); let mut types = vec![self.heap.mk_any_implicit()]; - for (found_attr, _) in lookup_result.found { + for (found_attr, _, _) in lookup_result.found { if let Ok(ty) = self.resolve_get_access(attr_name, found_attr, range, &suppress_errors, None) { @@ -883,7 +889,7 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { } } }; - for (attr, _) in lookup_result.found { + for (attr, _, _) in lookup_result.found { attr_tys.push( self.resolve_get_access(attr_name, attr, range, errors, context) .unwrap_or_else(|e| { @@ -957,7 +963,7 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { let (setattr_found, setattr_not_found, setattr_error) = self .lookup_magic_dunder_attr(attr_base, &dunder::SETATTR) .decompose(); - for (setattr_attr, _) in setattr_found { + for (setattr_attr, _, _) in setattr_found { let result = self .resolve_get_access(attr_name, setattr_attr, range, errors, context) .map(|setattr_ty| { @@ -1006,7 +1012,7 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { let (delattr_found, delattr_not_found, delattr_error) = self .lookup_magic_dunder_attr(attr_base, &dunder::DELATTR) .decompose(); - for (delattr_attr, _) in delattr_found { + for (delattr_attr, _, _) in delattr_found { let result = self .resolve_get_access(attr_name, delattr_attr, range, errors, context) .map(|delattr_ty| { @@ -1066,6 +1072,15 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { }; let (lookup_found, lookup_not_found, lookup_error) = self.lookup_attr(attr_base.clone(), attr_name).decompose(); + if lookup_found.iter().any(|(_, _, is_method)| *is_method) { + self.error_with_context( + errors, + range, + ErrorKind::MethodAssign, + format!("Cannot assign to method `{attr_name}`"), + context, + ); + } for e in lookup_error { e.add_to(errors, range, attr_name, todo_ctx); should_narrow = false; @@ -1088,7 +1103,7 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { ); should_narrow = false; } - for (attr, found_on) in lookup_found { + for (attr, found_on, _) in lookup_found { match attr { // Attribute setting bypasses `__getattr__` lookup and checks `__setattr__` // If the attribute is not found, we fall back to `__setattr__` @@ -1174,6 +1189,11 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { } } + fn class_member_is_method(&self, class: &Class, name: &Name) -> bool { + self.get_class_member(class, name) + .is_some_and(|field| field.is_method()) + } + pub fn check_set_read_write_and_infer_narrow( &self, attr_ty: Type, @@ -1358,7 +1378,7 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { for error in lookup_error { error.add_to(errors, range, attr_name, todo_ctx); } - for (attr, _) in lookup_found { + for (attr, _, _) in lookup_found { match attr { // Attribute deletion bypasses `__getattr__` lookup and checks `__delattr__` // If the attribute is not found, we fall back to `__delattr__` @@ -1468,7 +1488,7 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { if (!got_attrs.is_empty()) && let Some(want) = self.get_protocol_attribute(protocol, got.clone(), attr_name) { - for (got_attr, _) in got_attrs.iter() { + for (got_attr, _, _) in got_attrs.iter() { // A `__getattr__` fallback on a class object is not evidence that the // missing member exists: `__getattr__` governs instance attribute // access, not attributes of the class object itself. Reject rather @@ -1656,10 +1676,10 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { /// Fold one attribute per base into a single attribute, or `None` when they cannot be combined. fn fold_attribute_candidates( &self, - candidates: &[Vec<(Attribute, AttributeBase1)>], + candidates: &[Vec<(Attribute, AttributeBase1, bool)>], combine: Combine, - ) -> Option<(Attribute, AttributeBase1)> { - let [(_, found_on)] = candidates.first()?.as_slice() else { + ) -> Option<(Attribute, AttributeBase1, bool)> { + let [(_, found_on, _)] = candidates.first()?.as_slice() else { return None; }; let found_on = found_on.clone(); @@ -1667,10 +1687,12 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { let mut types = Vec::with_capacity(candidates.len()); let mut read_only_reason = None; let mut is_class_attribute = false; + let mut is_method = false; for base_results in candidates { - let [(attribute, _)] = base_results.as_slice() else { + let [(attribute, _, candidate_is_method)] = base_results.as_slice() else { return None; }; + is_method |= candidate_is_method; let ty = match attribute { Attribute::Simple(ty) => ty, Attribute::ClassAttribute(ClassAttribute::ReadWrite(ty)) => { @@ -1702,7 +1724,7 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { } else { Attribute::simple(combined) }; - Some((attribute, found_on)) + Some((attribute, found_on, is_method)) } /// Look up an attribute on a single `AttributeBase1` using only class field declarations. @@ -1723,7 +1745,11 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { acc.found_type(Lit::Str(e.member.as_str().into()).to_implicit_type(), base) } AttributeBase1::LiteralString => match self.get_literal_string_attribute(attr_name) { - Some(attr) => acc.found_class_attribute(attr, base), + Some(attr) => acc.found_class_attribute( + attr, + base, + self.class_member_is_method(self.stdlib.str().class_object(), attr_name), + ), None => acc.not_found(NotFoundOn::ClassInstance( self.stdlib.str().class_object().dupe(), base, @@ -1732,7 +1758,7 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { AttributeBase1::ShapedArrayInstance(tensor) => { if attr_name.as_str() == "shape" { if let Some(attr) = self.get_shaped_array_attribute(tensor, attr_name) { - acc.found_class_attribute(attr, base); + acc.found_class_attribute(attr, base, false); return; } let shape = if tensor.tuple_carrier_shape_arg_index().is_some() { @@ -1748,7 +1774,11 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { // handles Self-type substitution via InstanceKind::ShapedArray. let metadata = self.get_metadata_for_class(tensor.base_class.class_object()); match self.get_shaped_array_attribute(tensor, attr_name) { - Some(attr) => acc.found_class_attribute(attr, base), + Some(attr) => acc.found_class_attribute( + attr, + base.clone(), + self.class_member_is_method(tensor.base_class.class_object(), attr_name), + ), None if metadata.has_base_any() => acc.found_type(Type::any_implicit(), base), None => acc.not_found(NotFoundOn::ClassInstance( tensor.base_class.class_object().dupe(), @@ -1759,16 +1789,16 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { AttributeBase1::ClassInstance(class) => { // Special handling for nn.ModuleDict with TypedDict type argument if let Some(attr) = self.try_nn_module_dict_attr(class, attr_name) { - acc.found_class_attribute(attr, base); + acc.found_class_attribute(attr, base, false); return; } // Normal class instance attribute lookup let metadata = self.get_metadata_for_class(class.class_object()); let attr_lookup_result = - self.get_enum_or_instance_attribute(class, metadata, attr_name); + self.get_enum_or_instance_attribute_with_method(class, metadata, attr_name); match attr_lookup_result { - Some(attr) => acc.found_class_attribute(attr, base), + Some((attr, is_method)) => acc.found_class_attribute(attr, base, is_method), None if metadata.has_base_any() => { acc.found_type(self.heap.mk_any_implicit(), base) } @@ -1785,10 +1815,10 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { } AttributeBase1::EnumLiteral(lit @ LitEnum { class, .. }) => { let metadata = self.get_metadata_for_class(class.class_object()); - let attr_lookup_result = - self.get_enum_literal_or_instance_attribute(lit, metadata, attr_name); + let attr_lookup_result = self + .get_enum_literal_or_instance_attribute_with_method(lit, metadata, attr_name); match attr_lookup_result { - Some(attr) => acc.found_class_attribute(attr, base), + Some((attr, is_method)) => acc.found_class_attribute(attr, base, is_method), None if metadata.has_base_any() => { acc.found_type(self.heap.mk_any_implicit(), base) } @@ -1798,10 +1828,11 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { } } AttributeBase1::SuperInstance(cls, obj) => { - match self.get_super_attribute(cls, obj, attr_name) { - Some(attr) => acc.found_class_attribute( + match self.get_super_attribute_with_method(cls, obj, attr_name) { + Some((attr, is_method)) => acc.found_class_attribute( attr.read_only_equivalent(ReadOnlyReason::Super), base, + is_method, ), None if let SuperObj::Instance(cls) = obj && self.extends_any(cls.class_object()) => @@ -1812,6 +1843,7 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { ReadOnlyReason::Super, ), base, + false, ) } None if let SuperObj::Class(cls) = obj @@ -1823,6 +1855,7 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { ReadOnlyReason::Super, ), base, + false, ) } None => { @@ -1852,7 +1885,14 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { // When looking up a magic dunder method as part of checking a class object // against a protocol, we prefer methods on the metaclass over methods on the // class object. See test::enums::test_iterate for why we need to do this. - acc.found_class_attribute(attr, base) + let metaclass = self + .get_metadata_for_class(class.class_object()) + .metaclass(self.stdlib); + acc.found_class_attribute( + attr, + base, + self.class_member_is_method(metaclass.class_object(), attr_name), + ) } else if let AttributeBase1::ClassObject(class) = &**protocol_base && self .get_class_member(class.class_object(), attr_name) @@ -1894,33 +1934,40 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { quantified.clone(), class, attr_name, - ), - _ => self.get_class_attribute(class, attr_name), + ) + .map(|attr| { + ( + attr, + self.class_member_is_method(class.class_object(), attr_name), + ) + }), + _ => self.get_class_attribute_with_method(class, attr_name), }; match attr { - Some( + Some(( no_access @ ClassAttribute::NoAccess( NoAccessReason::ClassUseOfInstanceAttribute(_), ), - ) => { + is_method, + )) => { // Instance-only attributes from `__slots__` produce slot descriptors. // The order of precedence is: data descriptors > slot descriptors > non-descriptor attributes // All attributes on `type` like `__name__` are considered data descriptors, despite not being annotated as such. let metadata = self.get_metadata_for_class(class.class_object()); let metaclass = metadata.metaclass(self.stdlib); let metaclass_attr = - self.get_metaclass_attribute(class, metaclass, attr_name); + self.get_metaclass_attribute_with_method(class, metaclass, attr_name); match metaclass_attr { - Some(meta_attr) + Some((meta_attr, meta_is_method)) if self.class_attribute_is_data_descriptor(&meta_attr) || metaclass.class_object().is_builtin("type") => { - acc.found_class_attribute(meta_attr, base) + acc.found_class_attribute(meta_attr, base, meta_is_method) } - _ => acc.found_class_attribute(no_access, base), + _ => acc.found_class_attribute(no_access, base, is_method), } } - Some(attr) => { + Some((attr, is_method)) => { // When the class defines a @property, class-level access converts it // to ReadWrite (the raw getter). Check the metaclass for a property // with the same name — it takes precedence per the descriptor protocol. @@ -1931,25 +1978,26 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { let metadata = self.get_metadata_for_class(class.class_object()); let metaclass = metadata.metaclass(self.stdlib); if !metaclass.class_object().is_builtin("type") { - let metaclass_attr = - self.get_metaclass_attribute(class, metaclass, attr_name); + let metaclass_attr = self.get_metaclass_attribute_with_method( + class, metaclass, attr_name, + ); match metaclass_attr { - Some(meta_attr) + Some((meta_attr, meta_is_method)) if self.class_attribute_is_data_descriptor(&meta_attr) || matches!( meta_attr, ClassAttribute::Property(_, _, _) ) => { - acc.found_class_attribute(meta_attr, base) + acc.found_class_attribute(meta_attr, base, meta_is_method) } - _ => acc.found_class_attribute(attr, base), + _ => acc.found_class_attribute(attr, base, is_method), } } else { - acc.found_class_attribute(attr, base) + acc.found_class_attribute(attr, base, is_method) } } else { - acc.found_class_attribute(attr, base) + acc.found_class_attribute(attr, base, is_method) } } None => { @@ -1967,13 +2015,15 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { acc, ); } else { - let instance_attr = self.get_metaclass_attribute( + let instance_attr = self.get_metaclass_attribute_with_method( class, metadata.metaclass(self.stdlib), attr_name, ); match instance_attr { - Some(attr) => acc.found_class_attribute(attr, base), + Some((attr, is_method)) => { + acc.found_class_attribute(attr, base, is_method) + } None if metadata.has_base_any() => { // We can't immediately fall back to Any in this case -- `type[Any]` is actually a special // AttributeBase which requires additional lookup on `type` itself before the Any fallback. @@ -1999,7 +2049,11 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { }, AttributeBase1::Quantified(q, bound) => { match self.get_bounded_quantified_attribute(q.clone(), bound, attr_name) { - Some(attr) => acc.found_class_attribute(attr, base), + Some(attr) => acc.found_class_attribute( + attr, + base.clone(), + self.class_member_is_method(bound.class_object(), attr_name), + ), None => { acc.not_found(NotFoundOn::ClassInstance(bound.class_object().dupe(), base)) } @@ -2040,8 +2094,8 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { acc.found_type(getter, base) } else { let class = self.stdlib.property(); - match self.get_instance_attribute(class, attr_name) { - Some(attr) => acc.found_class_attribute(attr, base), + match self.get_instance_attribute_with_method(class, attr_name) { + Some((attr, is_method)) => acc.found_class_attribute(attr, base, is_method), None => acc.not_found(NotFoundOn::ClassInstance( class.class_object().dupe(), base, @@ -2050,8 +2104,8 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { } } AttributeBase1::TypedDict(typed_dict) => { - match self.get_typed_dict_attribute(typed_dict, attr_name) { - Some(attr) => acc.found_class_attribute(attr, base), + match self.get_typed_dict_attribute_with_method(typed_dict, attr_name) { + Some((attr, is_method)) => acc.found_class_attribute(attr, base, is_method), None => acc.not_found(NotFoundOn::ClassInstance( typed_dict.class_object().dupe(), base, @@ -2059,7 +2113,11 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { } } AttributeBase1::SelfType(cls) => match self.get_self_attribute(cls, attr_name) { - Some(attr) => acc.found_class_attribute(attr, base), + Some(attr) => acc.found_class_attribute( + attr, + base.clone(), + self.class_member_is_method(cls.class_object(), attr_name), + ), None => { let metadata = self.get_metadata_for_class(cls.class_object()); if metadata.has_base_any() { @@ -2162,8 +2220,8 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { )); return; } - match self.get_metaclass_attribute(class, metaclass, dunder_name) { - Some(attr) => acc.found_class_attribute(attr, base), + match self.get_metaclass_attribute_with_method(class, metaclass, dunder_name) { + Some((attr, is_method)) => acc.found_class_attribute(attr, base, is_method), None => acc.not_found(NotFoundOn::ClassInstance( metaclass.class_object().clone(), base, @@ -2282,7 +2340,7 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { ) .decompose(); if getattribute_not_found.is_empty() && getattribute_internal_error.is_empty() { - for (attr, found_on) in getattribute_found { + for (attr, found_on, _) in getattribute_found { result.found( Attribute::getattr(not_found.clone(), attr, attr_name.clone()), found_on, @@ -2295,7 +2353,7 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { .lookup_magic_dunder_attr(not_found.attr_base1().to_attr_base(), &dunder::GETATTR) .decompose(); if getattr_not_found.is_empty() && getattr_internal_error.is_empty() { - for (attr, found_on) in getattr_found { + for (attr, found_on, _) in getattr_found { result.found( Attribute::getattr(not_found.clone(), attr, attr_name.clone()), found_on, @@ -3025,7 +3083,7 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { }; let (found, not_found, internal_errors) = lookup_result.decompose(); let mut results = Vec::new(); - for (attr, _) in found { + for (attr, _, _) in found { let found_ty = match self.resolve_get_access(attr_name, attr, range, errors, None) { Err(..) => fall_back_to_object(), Ok(ty) => ty, @@ -3283,7 +3341,7 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { let mut is_deprecated = false; let found_types: Vec<_> = found_attrs .into_iter() - .filter_map(|(attr, _)| { + .filter_map(|(attr, _, _)| { match &attr { Attribute::ClassAttribute(ClassAttribute::ReadWrite(ty)) | Attribute::ClassAttribute(ClassAttribute::ReadOnly(ty, _)) diff --git a/pyrefly/lib/alt/class/class_field.rs b/pyrefly/lib/alt/class/class_field.rs index 89d9a0ff61..0ab5673be7 100644 --- a/pyrefly/lib/alt/class/class_field.rs +++ b/pyrefly/lib/alt/class/class_field.rs @@ -1053,6 +1053,13 @@ impl ClassField { } } + pub(crate) fn is_method(&self) -> bool { + matches!( + &self.0, + ClassFieldInner::Method { .. } | ClassFieldInner::ProxyMethod { .. } + ) + } + pub fn is_final(&self) -> bool { match &self.0 { ClassFieldInner::Property { ty, .. } => ty.has_final_decoration(), @@ -4816,8 +4823,22 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { } pub fn get_instance_attribute(&self, cls: &ClassType, name: &Name) -> Option { + self.get_instance_attribute_with_method(cls, name) + .map(|(attr, _)| attr) + } + + pub fn get_instance_attribute_with_method( + &self, + cls: &ClassType, + name: &Name, + ) -> Option<(ClassAttribute, bool)> { self.get_class_member(cls.class_object(), name) - .map(|field| self.as_instance_attribute(name, field.as_ref(), &Instance::of_class(cls))) + .map(|field| { + let is_method = field.is_method(); + let attr = + self.as_instance_attribute(name, field.as_ref(), &Instance::of_class(cls)); + (attr, is_method) + }) } pub fn get_self_attribute(&self, cls: &ClassType, name: &Name) -> Option { @@ -4886,7 +4907,18 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { metaclass: &ClassType, name: &Name, ) -> Option { + self.get_metaclass_attribute_with_method(cls, metaclass, name) + .map(|(attr, _)| attr) + } + + pub fn get_metaclass_attribute_with_method( + &self, + cls: &ClassBase, + metaclass: &ClassType, + name: &Name, + ) -> Option<(ClassAttribute, bool)> { let attr = self.get_class_member(metaclass.class_object(), name)?; + let is_method = attr.is_method(); let attr = self.as_instance_attribute( name, attr.as_ref(), @@ -4897,11 +4929,12 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { .class_object() .has_toplevel_qname(ModuleName::builtins().as_str(), "type") { - return Some(ClassAttribute::read_write( - self.constructor_to_callable(cls.class_type()), + return Some(( + ClassAttribute::read_write(self.constructor_to_callable(cls.class_type())), + is_method, )); } - Some(attr) + Some((attr, is_method)) } /// Returns true if this attribute represents a data descriptor @@ -4960,6 +4993,15 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { td: &TypedDictInner, name: &Name, ) -> Option { + self.get_typed_dict_attribute_with_method(td, name) + .map(|(attr, _)| attr) + } + + pub fn get_typed_dict_attribute_with_method( + &self, + td: &TypedDictInner, + name: &Name, + ) -> Option<(ClassAttribute, bool)> { if let Some(meta) = self .get_metadata_for_class(td.class_object()) .typed_dict_metadata() @@ -4981,15 +5023,20 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { } }) .map(|member| { - self.as_instance_attribute( + let is_method = member.value.is_method(); + let attr = self.as_instance_attribute( name, member.value.as_ref(), &Instance::of_typed_dict(td), - ) + ); + (attr, is_method) }); } self.get_class_member(td.class_object(), name).map(|field| { - self.as_instance_attribute(name, field.as_ref(), &Instance::of_typed_dict(td)) + let is_method = field.is_method(); + let attr = + self.as_instance_attribute(name, field.as_ref(), &Instance::of_typed_dict(td)); + (attr, is_method) }) } @@ -5034,31 +5081,45 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { super_obj: &SuperObj, name: &Name, ) -> Option { + self.get_super_attribute_with_method(start_lookup_cls, super_obj, name) + .map(|(attr, _)| attr) + } + + pub fn get_super_attribute_with_method( + &self, + start_lookup_cls: &ClassType, + super_obj: &SuperObj, + name: &Name, + ) -> Option<(ClassAttribute, bool)> { match super_obj { SuperObj::Instance(obj) => self .get_super_class_member(obj.class_object(), Some(start_lookup_cls), name) .map(|member| { + let is_method = member.value.is_method(); if let Some(reason) = self.super_method_needs_impl_reason(&member) { - ClassAttribute::no_access(reason) + (ClassAttribute::no_access(reason), is_method) } else { - self.as_instance_attribute( + let attr = self.as_instance_attribute( name, member.value.as_ref(), &Instance::of_self_type(obj), - ) + ); + (attr, is_method) } }), SuperObj::Class(obj) => self .get_super_class_member(obj.class_object(), Some(start_lookup_cls), name) .map(|member| { + let is_method = member.value.is_method(); if let Some(reason) = self.super_method_needs_impl_reason(&member) { - ClassAttribute::no_access(reason) + (ClassAttribute::no_access(reason), is_method) } else { - self.as_class_attribute( + let attr = self.as_class_attribute( name, member.value.as_ref(), &ClassBase::SelfType(obj.clone()), - ) + ); + (attr, is_method) } }), } @@ -5098,8 +5159,21 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { /// Access is disallowed for instance-only attributes and for attributes whose /// type contains a class-scoped type parameter - e.g., `class A[T]: x: T`. pub fn get_class_attribute(&self, cls: &ClassBase, name: &Name) -> Option { + self.get_class_attribute_with_method(cls, name) + .map(|(attr, _)| attr) + } + + pub fn get_class_attribute_with_method( + &self, + cls: &ClassBase, + name: &Name, + ) -> Option<(ClassAttribute, bool)> { self.get_class_member(cls.class_object(), name) - .map(|field| self.as_class_attribute(name, field.as_ref(), cls)) + .map(|field| { + let is_method = field.is_method(); + let attr = self.as_class_attribute(name, field.as_ref(), cls); + (attr, is_method) + }) } pub fn get_bounded_quantified_class_attribute( diff --git a/pyrefly/lib/alt/class/enums.rs b/pyrefly/lib/alt/class/enums.rs index f220d34a1d..83e3ae0ac0 100644 --- a/pyrefly/lib/alt/class/enums.rs +++ b/pyrefly/lib/alt/class/enums.rs @@ -154,8 +154,19 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { metadata: &ClassMetadata, attr_name: &Name, ) -> Option { + self.get_enum_or_instance_attribute_with_method(class, metadata, attr_name) + .map(|(attr, _)| attr) + } + + pub fn get_enum_or_instance_attribute_with_method( + &self, + class: &ClassType, + metadata: &ClassMetadata, + attr_name: &Name, + ) -> Option<(ClassAttribute, bool)> { self.special_case_enum_attr_lookup(class, None, metadata, attr_name) - .or_else(|| self.get_instance_attribute(class, attr_name)) + .map(|attr| (attr, false)) + .or_else(|| self.get_instance_attribute_with_method(class, attr_name)) } /// Checks for a special-cased enum attribute on an enum literal, falling back to a regular instance attribute lookup. @@ -165,9 +176,20 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> { metadata: &ClassMetadata, attr_name: &Name, ) -> Option { + self.get_enum_literal_or_instance_attribute_with_method(lit, metadata, attr_name) + .map(|(attr, _)| attr) + } + + pub fn get_enum_literal_or_instance_attribute_with_method( + &self, + lit: &LitEnum, + metadata: &ClassMetadata, + attr_name: &Name, + ) -> Option<(ClassAttribute, bool)> { let class = &lit.class; self.special_case_enum_attr_lookup(class, Some(lit), metadata, attr_name) - .or_else(|| self.get_instance_attribute(class, attr_name)) + .map(|attr| (attr, false)) + .or_else(|| self.get_instance_attribute_with_method(class, attr_name)) } /// Special-case enum attribute lookups. Dispatches to the appropriate helper diff --git a/pyrefly/lib/test/attributes.rs b/pyrefly/lib/test/attributes.rs index 0e32b0c722..16f471c71e 100644 --- a/pyrefly/lib/test/attributes.rs +++ b/pyrefly/lib/test/attributes.rs @@ -262,8 +262,7 @@ Child.shared ); testcase!( - bug = "Example of how making methods read-write but not invariant is unsound", - test_method_assign, + test_method_assign_disabled_by_default, r#" from typing import Protocol class X(Protocol): @@ -280,6 +279,83 @@ y.foo() # result is "hi" "#, ); +testcase!( + test_method_assign, + TestEnv::new().enable_method_assign_error(), + r#" +from collections.abc import Callable + +class A: + def method(self) -> None: ... + + @staticmethod + def static_method() -> None: ... + + @classmethod + def class_method(cls) -> None: ... + + callback: Callable[[], None] = lambda: None + +class B(A): + pass + +def replacement(self: A) -> None: ... + +a = A() +A.method = replacement # E: Cannot assign to method `method` +a.method = lambda: None # E: Cannot assign to method `method` +A.static_method = lambda: None # E: Cannot assign to method `static_method` +a.static_method = lambda: None # E: Cannot assign to method `static_method` +A.class_method = lambda: None # E: Cannot assign to method `class_method` +a.class_method = lambda: None # E: Cannot assign to method `class_method` +B.method = replacement # E: Cannot assign to method `method` +B().method = lambda: None # E: Cannot assign to method `method` +A.callback = lambda: None +a.callback = lambda: None + "#, +); + +testcase!( + test_method_assign_lookup_precedence, + TestEnv::new().enable_method_assign_error(), + r#" +from collections.abc import Callable +from typing import TypedDict + +class Meta(type): + def f(cls) -> None: ... + +class C(metaclass=Meta): + pass + +C.f = C.f # E: Cannot assign to method `f` + +class D(TypedDict): + items: int + +d: D = {"items": 1} +d.items = d.items # E: Cannot assign to method `items` + +class Base: + def f(self) -> None: ... + +class Child(Base): + f: Callable[[], None] = lambda: None + + def test(self) -> None: + super().f = lambda: None # E: Cannot assign to method `f` # E: Cannot set field `f` + +class Parent: + f: Callable[[], None] = lambda: None + +class Derived(Parent): + def f(self) -> None: ... + + def test(self) -> None: + super().f = lambda: None # E: Cannot set field `f` + "#, +); + testcase!( test_attribute_union, r#" diff --git a/pyrefly/lib/test/util.rs b/pyrefly/lib/test/util.rs index 6cad8dd100..262117ff58 100644 --- a/pyrefly/lib/test/util.rs +++ b/pyrefly/lib/test/util.rs @@ -161,6 +161,7 @@ pub struct TestEnv { unknown_attribute_type_error: bool, implicit_abstract_class_error: bool, open_unpacking_error: bool, + method_assign_error: bool, missing_override_decorator_error: bool, missing_super_call_error: bool, not_required_key_access_error: bool, @@ -220,6 +221,7 @@ impl TestEnv { unknown_attribute_type_error: false, implicit_abstract_class_error: false, open_unpacking_error: false, + method_assign_error: false, missing_override_decorator_error: false, missing_super_call_error: false, not_required_key_access_error: false, @@ -402,6 +404,11 @@ impl TestEnv { self } + pub fn enable_method_assign_error(mut self) -> Self { + self.method_assign_error = true; + self + } + pub fn enable_missing_override_decorator_error(mut self) -> Self { self.missing_override_decorator_error = true; self @@ -675,6 +682,9 @@ impl TestEnv { if self.open_unpacking_error { errors.set_error_severity(ErrorKind::OpenUnpacking, Severity::Error); } + if self.method_assign_error { + errors.set_error_severity(ErrorKind::MethodAssign, Severity::Error); + } if self.missing_override_decorator_error { errors.set_error_severity(ErrorKind::MissingOverrideDecorator, Severity::Error); } diff --git a/scripts/error_presets.json b/scripts/error_presets.json index b7fe090f96..71cca9112e 100644 --- a/scripts/error_presets.json +++ b/scripts/error_presets.json @@ -78,6 +78,7 @@ "invalid-type-var-tuple": ["legacy", "default", "strict", "all"], "invalid-variance": ["legacy", "default", "strict", "all"], "invalid-yield": ["legacy", "default", "strict", "all"], + "method-assign": ["strict", "all"], "misplaced-ignore": ["legacy", "default", "strict", "all"], "missing-argument": ["legacy", "default", "strict", "all"], "missing-attribute": ["legacy", "default", "strict", "all"], diff --git a/test/args.md b/test/args.md index 244c1dcb95..a6f2630170 100644 --- a/test/args.md +++ b/test/args.md @@ -115,6 +115,15 @@ $ echo -e '[errors]\nimplicit-any = "ignore"' > $TMPDIR/pyrefly.toml && \ [0] ``` +## Strict mode rejects assigning to methods + +```scrut {output_stream: stdout} +$ echo -e 'class A:\n def f(self) -> None: ...\n\ndef replacement(self: A) -> None: ...\n\nA.f = replacement' > $TMPDIR/method_assign.py && \ +> $PYREFLY check $TMPDIR/method_assign.py --preset strict --output-format=min-text --summary=none +ERROR * Cannot assign to method `f` [method-assign] (glob) +[1] +``` + ## Scalar fields in the config file override `--preset` ```scrut {output_stream: stdout} diff --git a/website/docs/configuration.mdx b/website/docs/configuration.mdx index a346ed78d9..9c17b18be9 100644 --- a/website/docs/configuration.mdx +++ b/website/docs/configuration.mdx @@ -707,7 +707,7 @@ The default Pyrefly configuration. Equivalent to having no preset at all. Enables additional error codes on top of the default for stricter checking. - Sets [`strict-callable-subtyping`](#strict-callable-subtyping) `= true` -- Enables (as errors): [`direct-abstract-base-instantiation`](./error-kinds.mdx#direct-abstract-base-instantiation), [`implicit-any`](./error-kinds.mdx#implicit-any) (covers every implicit-`Any` sub-kind), [`missing-override-decorator`](./error-kinds.mdx#missing-override-decorator), [`open-unpacking`](./error-kinds.mdx#open-unpacking), [`potential-bad-keyword-argument`](./error-kinds.mdx#potential-bad-keyword-argument), [`unused-ignore`](./error-kinds.mdx#unused-ignore) +- Enables (as errors): [`direct-abstract-base-instantiation`](./error-kinds.mdx#direct-abstract-base-instantiation), [`implicit-any`](./error-kinds.mdx#implicit-any) (covers every implicit-`Any` sub-kind), [`method-assign`](./error-kinds.mdx#method-assign), [`missing-override-decorator`](./error-kinds.mdx#missing-override-decorator), [`open-unpacking`](./error-kinds.mdx#open-unpacking), [`potential-bad-keyword-argument`](./error-kinds.mdx#potential-bad-keyword-argument), [`unused-ignore`](./error-kinds.mdx#unused-ignore) #### Preset: `all` diff --git a/website/docs/error-kinds.mdx b/website/docs/error-kinds.mdx index 3cd9c5999d..d07491e47b 100644 --- a/website/docs/error-kinds.mdx +++ b/website/docs/error-kinds.mdx @@ -1249,6 +1249,23 @@ def bad_yield_from() -> Generator[int, None, None]: yield from 1 ``` +## method-assign + +Default severity: `ignore` + +Assigning to a method on a class object or instance is ambiguous because Python's +type system cannot distinguish the bound and unbound callable types that the +replacement will produce. This check is enabled by the `strict` preset. + +```python +class Service: + def run(self) -> None: ... + +def replacement(self: Service) -> None: ... + +Service.run = replacement # method-assign +``` + ## misplaced-ignore Default severity: `warn` diff --git a/website/docs/migrate/mypy/error-codes.mdx b/website/docs/migrate/mypy/error-codes.mdx index 5de834fd38..dedf8185e7 100644 --- a/website/docs/migrate/mypy/error-codes.mdx +++ b/website/docs/migrate/mypy/error-codes.mdx @@ -60,7 +60,7 @@ kind does not make every diagnostic identical to mypy's. | `return` | `bad-return` | Automatic | Broad | Mypy uses this for missing-return paths; Pyrefly groups incompatible and missing returns under `bad-return`. | | `return-value` | `bad-return` | Automatic | Direct-ish | | | `assignment` | `bad-assignment` | Automatic | Direct-ish | | -| `method-assign` | `bad-assignment` or `read-only` | Manual | Related | Depends on whether the failure is type incompatibility or attempted mutation. | +| `method-assign` | `method-assign` | Automatic | Direct | | | `type-arg` | `implicit-any-type-argument` | Automatic | Direct-ish | | | `type-var` | `bad-specialization` | Automatic | Partial | Pyrefly uses `invalid-type-var` for many TypeVar definition and usage errors. | | `union-attr` | `missing-attribute` | Automatic | Direct-ish | | diff --git a/website/docs/migrate/mypy/strict-mode.mdx b/website/docs/migrate/mypy/strict-mode.mdx index b87024fe5d..097eaf0407 100644 --- a/website/docs/migrate/mypy/strict-mode.mdx +++ b/website/docs/migrate/mypy/strict-mode.mdx @@ -53,6 +53,7 @@ The [`strict` preset](../../configuration.mdx#preset-strict) sets `strict-callable-subtyping = true` and enables [`implicit-any`](../../error-kinds.mdx#implicit-any) (which covers every implicit-`Any` sub-kind), +[`method-assign`](../../error-kinds.mdx#method-assign), [`missing-override-decorator`](../../error-kinds.mdx#missing-override-decorator), [`potential-bad-keyword-argument`](../../error-kinds.mdx#potential-bad-keyword-argument), [`unused-ignore`](../../error-kinds.mdx#unused-ignore), and @@ -66,6 +67,7 @@ implicit-`Any` sub-kind), | Redundant casts | `redundant-cast = "warn"` | Generated at warning severity | | Untyped or incompletely typed definitions | `implicit-any-parameter` + `unannotated-return` | Two Pyrefly policies stand in for several mypy flags | | Missing generic arguments | `implicit-any = "error"` | Broader than `implicit-any-type-argument` alone | +| Assigning to methods | `method-assign = "error"` | Direct mapping, and enabled by Pyrefly's strict preset | | Checking unannotated function bodies | Not expanded by the converter | Set `check-unannotated-defs = true` yourself | | Explicit `Any` | `explicit-any` | Only when `disallow_any_explicit` is set; mypy `strict` alone does not enable it | | Unused ignores | `unused-type-ignore` / `unused-ignore` | Not part of the mypy converter map; add by hand | diff --git a/website/docs/migrate/pyright/strict-mode.mdx b/website/docs/migrate/pyright/strict-mode.mdx index 8514b0dffe..6039ae583a 100644 --- a/website/docs/migrate/pyright/strict-mode.mdx +++ b/website/docs/migrate/pyright/strict-mode.mdx @@ -53,6 +53,7 @@ several linter-like checks. Pyrefly's - [`implicit-any`](../../error-kinds.mdx#implicit-any), which covers every implicit-`Any` sub-kind; +- [`method-assign`](../../error-kinds.mdx#method-assign); - [`missing-override-decorator`](../../error-kinds.mdx#missing-override-decorator); - [`potential-bad-keyword-argument`](../../error-kinds.mdx#potential-bad-keyword-argument); - [`unused-ignore`](../../error-kinds.mdx#unused-ignore);