[ty] Add decorator check for implicit attribute assignments (#18587)

## Summary

Previously, the checks for implicit attribute assignments didn't
properly account for method decorators. This PR fixes that by:

- Adding a decorator check in `implicit_instance_attribute`. This allows
it to filter out methods with mismatching decorators when analyzing
attribute assignments.
- Adding attribute search for implicit class attributes: if an attribute
can't be found directly in the class body, the
`ClassLiteral::own_class_member` function will now search in
classmethods.
- Adding `staticmethod`: it has been added into `KnownClass` and
together with the new decorator check, it will no longer expose
attributes when the assignment target name is the same as the first
method name.

If accepted, it should fix https://github.com/astral-sh/ty/issues/205
and https://github.com/astral-sh/ty/issues/207.

## Test Plan

This is tested with existing mdtest suites and is able to get most of
the TODO marks for implicit assignments in classmethods and
staticmethods removed.

However, there's one specific test case I failed to figure out how to
correctly resolve:


b279508bdc/crates/ty_python_semantic/resources/mdtest/attributes.md (L754-L755)

I tried to add `instance_member().is_unbound()` check in this [else
branch](b279508bdc/crates/ty_python_semantic/src/types/infer.rs (L3299-L3301))
but it causes tests with class attributes defined in class body to fail.
While it's possible to implicitly add `ClassVar` to qualifiers to make
this assignment fail and keep everything else passing, it doesn't feel
like the right solution.

crates/ty_python_semantic/resources/mdtest/attributes.md

@@ -522,8 +522,8 @@ class C:
 # error: [unresolved-attribute]
 reveal_type(C.x)  # revealed: Unknown
 
-# TODO: this should raise `unresolved-attribute` as well, and the type should be `Unknown`
+# error: [unresolved-attribute]
-reveal_type(C().x)  # revealed: Unknown | Literal[1]
+reveal_type(C().x)  # revealed: Unknown
 
 # This also works if `staticmethod` is aliased:
 
@@ -537,8 +537,8 @@ class D:
 # error: [unresolved-attribute]
 reveal_type(D.x)  # revealed: Unknown
 
-# TODO: this should raise `unresolved-attribute` as well, and the type should be `Unknown`
+# error: [unresolved-attribute]
-reveal_type(D().x)  # revealed: Unknown | Literal[1]
+reveal_type(D().x)  # revealed: Unknown
 ```
 
 If `staticmethod` is something else, that should not influence the behavior:
@@ -571,8 +571,8 @@ class C:
 # error: [unresolved-attribute]
 reveal_type(C.x)  # revealed: Unknown
 
-# TODO: this should raise `unresolved-attribute` as well, and the type should be `Unknown`
+# error: [unresolved-attribute]
-reveal_type(C().x)  # revealed: Unknown | Literal[1]
+reveal_type(C().x)  # revealed: Unknown
 ```
 
 #### Attributes defined in statically-known-to-be-false branches
@@ -742,17 +742,9 @@ class C:
 # for a more realistic example, let's actually call the method
 C.class_method()
 
-# TODO: We currently plan to support this and show no error here.
+reveal_type(C.pure_class_variable)  # revealed: Unknown | Literal["value set in class method"]
-# mypy shows an error here, pyright does not.
-# error: [unresolved-attribute]
-reveal_type(C.pure_class_variable)  # revealed: Unknown
 
-# TODO: should be no error when descriptor protocol is supported
-# and the assignment is properly attributed to the class method.
-# error: [invalid-attribute-access] "Cannot assign to instance attribute `pure_class_variable` from the class object `<class 'C'>`"
 C.pure_class_variable = "overwritten on class"
-# TODO: should be no error
-# error: [unresolved-attribute] "Attribute `pure_class_variable` can only be accessed on instances, not on the class object `<class 'C'>` itself."
 reveal_type(C.pure_class_variable)  # revealed: Literal["overwritten on class"]
 
 c_instance = C()
@@ -2150,6 +2142,25 @@ class C:
 reveal_type(C().x)  # revealed: int
 ```
 
+### Attributes defined in methods with unknown decorators
+
+When an attribute is defined in a method that is decorated with an unknown decorator, we consider it
+to be accessible on both the class itself and instances of that class. This is consistent with the
+gradual guarantee, because the unknown decorator *could* be an alias for `builtins.classmethod`.
+
+```py
+# error: [unresolved-import]
+from unknown_library import unknown_decorator
+
+class C:
+    @unknown_decorator
+    def f(self):
+        self.x: int = 1
+
+reveal_type(C.x)  # revealed: int
+reveal_type(C().x)  # revealed: int
+```
+
 ## Enum classes
 
 Enums are not supported yet; attribute access on an enum class is inferred as `Todo`.

crates/ty_python_semantic/src/types.rs

@@ -2503,6 +2503,18 @@ impl<'db> Type<'db> {
         }
     }
 
+    #[salsa::tracked]
+    #[allow(unused_variables)]
+    // If we choose name `_unit`, the macro will generate code that uses `_unit`, causing clippy to fail.
+    fn lookup_dunder_new(self, db: &'db dyn Db, unit: ()) -> Option<PlaceAndQualifiers<'db>> {
+        self.find_name_in_mro_with_policy(
+            db,
+            "__new__",
+            MemberLookupPolicy::MRO_NO_OBJECT_FALLBACK
+                | MemberLookupPolicy::META_CLASS_NO_TYPE_FALLBACK,
+        )
+    }
+
     /// Look up an attribute in the MRO of the meta-type of `self`. This returns class-level attributes
     /// when called on an instance-like type, and metaclass attributes when called on a class-like type.
     ///
@@ -4662,12 +4674,7 @@ impl<'db> Type<'db> {
         // An alternative might be to not skip `object.__new__` but instead mark it such that it's
         // easy to check if that's the one we found?
         // Note that `__new__` is a static method, so we must inject the `cls` argument.
-        let new_method = self_type.find_name_in_mro_with_policy(
+        let new_method = self_type.lookup_dunder_new(db, ());
-            db,
-            "__new__",
-            MemberLookupPolicy::MRO_NO_OBJECT_FALLBACK
-                | MemberLookupPolicy::META_CLASS_NO_TYPE_FALLBACK,
-        );
         let new_call_outcome = new_method.and_then(|new_method| {
             match new_method.place.try_call_dunder_get(db, self_type) {
                 Place::Type(new_method, boundness) => {

crates/ty_python_semantic/src/types/class.rs

@@ -4,8 +4,10 @@ use std::sync::{LazyLock, Mutex};
 use super::TypeVarVariance;
 use super::{
     IntersectionBuilder, MemberLookupPolicy, Mro, MroError, MroIterator, SpecialFormType,
-    SubclassOfType, Truthiness, Type, TypeQualifiers, class_base::ClassBase, infer_expression_type,
+    SubclassOfType, Truthiness, Type, TypeQualifiers,
-    infer_unpack_types,
+    class_base::ClassBase,
+    function::{FunctionDecorators, FunctionType},
+    infer_expression_type, infer_unpack_types,
 };
 use crate::semantic_index::DeclarationWithConstraint;
 use crate::semantic_index::definition::{Definition, DefinitionState};
@@ -639,6 +641,29 @@ impl<'db> From<ClassType<'db>> for Type<'db> {
     }
 }
 
+/// A filter that describes which methods are considered when looking for implicit attribute assignments
+/// in [`ClassLiteral::implicit_attribute`].
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
+pub(super) enum MethodDecorator {
+    None,
+    ClassMethod,
+    StaticMethod,
+}
+
+impl MethodDecorator {
+    fn try_from_fn_type(db: &dyn Db, fn_type: FunctionType) -> Result<Self, ()> {
+        match (
+            fn_type.has_known_decorator(db, FunctionDecorators::CLASSMETHOD),
+            fn_type.has_known_decorator(db, FunctionDecorators::STATICMETHOD),
+        ) {
+            (true, true) => Err(()), // A method can't be static and class method at the same time.
+            (true, false) => Ok(Self::ClassMethod),
+            (false, true) => Ok(Self::StaticMethod),
+            (false, false) => Ok(Self::None),
+        }
+    }
+}
+
 /// Representation of a class definition statement in the AST: either a non-generic class, or a
 /// generic class that has not been specialized.
 ///
@@ -1284,8 +1309,10 @@ impl<'db> ClassLiteral<'db> {
             {
                 return Place::bound(synthesized_member).into();
             }
+            // The symbol was not found in the class scope. It might still be implicitly defined in `@classmethod`s.
+            return Self::implicit_attribute(db, body_scope, name, MethodDecorator::ClassMethod)
+                .into();
         }
-
         symbol
     }
 
@@ -1601,12 +1628,15 @@ impl<'db> ClassLiteral<'db> {
         }
     }
 
-    /// Tries to find declarations/bindings of an instance attribute named `name` that are only
+    /// Tries to find declarations/bindings of an attribute named `name` that are only
-    /// "implicitly" defined in a method of the class that corresponds to `class_body_scope`.
+    /// "implicitly" defined (`self.x = …`, `cls.x = …`) in a method of the class that
-    fn implicit_instance_attribute(
+    /// corresponds to `class_body_scope`. The `target_method_decorator` parameter is
+    /// used to skip methods that do not have the expected decorator.
+    fn implicit_attribute(
         db: &'db dyn Db,
         class_body_scope: ScopeId<'db>,
         name: &str,
+        target_method_decorator: MethodDecorator,
     ) -> Place<'db> {
         // If we do not see any declarations of an attribute, neither in the class body nor in
         // any method, we build a union of `Unknown` with the inferred types of all bindings of
@@ -1626,6 +1656,17 @@ impl<'db> ClassLiteral<'db> {
             attribute_assignments(db, class_body_scope, name)
         {
             let method_scope = method_scope_id.to_scope_id(db, file);
+            if let Some(method_def) = method_scope.node(db).as_function(&module) {
+                let method_name = method_def.name.as_str();
+                if let Place::Type(Type::FunctionLiteral(method_type), _) =
+                    class_symbol(db, class_body_scope, method_name).place
+                {
+                    let method_decorator = MethodDecorator::try_from_fn_type(db, method_type);
+                    if method_decorator != Ok(target_method_decorator) {
+                        continue;
+                    }
+                }
+            }
             let method_map = use_def_map(db, method_scope);
 
             // The attribute assignment inherits the reachability of the method which contains it
@@ -1901,7 +1942,7 @@ impl<'db> ClassLiteral<'db> {
                         // The attribute is declared and bound in the class body.
 
                         if let Some(implicit_ty) =
-                            Self::implicit_instance_attribute(db, body_scope, name)
+                            Self::implicit_attribute(db, body_scope, name, MethodDecorator::None)
                                 .ignore_possibly_unbound()
                         {
                             if declaredness == Boundness::Bound {
@@ -1935,9 +1976,13 @@ impl<'db> ClassLiteral<'db> {
                         if declaredness == Boundness::Bound {
                             declared.with_qualifiers(qualifiers)
                         } else {
-                            if let Some(implicit_ty) =
+                            if let Some(implicit_ty) = Self::implicit_attribute(
-                                Self::implicit_instance_attribute(db, body_scope, name)
+                                db,
-                                    .ignore_possibly_unbound()
+                                body_scope,
+                                name,
+                                MethodDecorator::None,
+                            )
+                            .ignore_possibly_unbound()
                             {
                                 Place::Type(
                                     UnionType::from_elements(db, [declared_ty, implicit_ty]),
@@ -1958,7 +2003,7 @@ impl<'db> ClassLiteral<'db> {
                     // The attribute is not *declared* in the class body. It could still be declared/bound
                     // in a method.
 
-                    Self::implicit_instance_attribute(db, body_scope, name).into()
+                    Self::implicit_attribute(db, body_scope, name, MethodDecorator::None).into()
                 }
                 Err((declared, _conflicting_declarations)) => {
                     // There are conflicting declarations for this attribute in the class body.
@@ -1969,7 +2014,7 @@ impl<'db> ClassLiteral<'db> {
             // This attribute is neither declared nor bound in the class body.
             // It could still be implicitly defined in a method.
 
-            Self::implicit_instance_attribute(db, body_scope, name).into()
+            Self::implicit_attribute(db, body_scope, name, MethodDecorator::None).into()
         }
     }
 

crates/ty_python_semantic/src/types/infer.rs

@@ -10918,4 +10918,98 @@ mod tests {
 
         Ok(())
     }
+
+    #[test]
+    fn dependency_implicit_class_member() -> anyhow::Result<()> {
+        fn x_rhs_expression(db: &TestDb) -> Expression<'_> {
+            let file_main = system_path_to_file(db, "/src/main.py").unwrap();
+            let ast = parsed_module(db, file_main).load(db);
+            // Get the third statement in `main.py` (x = …) and extract the expression
+            // node on the right-hand side:
+            let x_rhs_node = &ast.syntax().body[2].as_assign_stmt().unwrap().value;
+
+            let index = semantic_index(db, file_main);
+            index.expression(x_rhs_node.as_ref())
+        }
+
+        let mut db = setup_db();
+
+        db.write_dedented(
+            "/src/mod.py",
+            r#"
+            class C:
+                def __init__(self):
+                    self.instance_attr: str = "24"
+
+                @classmethod
+                def method(cls):
+                    cls.class_attr: int = 42
+            "#,
+        )?;
+        db.write_dedented(
+            "/src/main.py",
+            r#"
+            from mod import C
+            C.method()
+            x = C().class_attr
+            "#,
+        )?;
+
+        let file_main = system_path_to_file(&db, "/src/main.py").unwrap();
+        let attr_ty = global_symbol(&db, file_main, "x").place.expect_type();
+        assert_eq!(attr_ty.display(&db).to_string(), "Unknown | int");
+
+        // Change the type of `class_attr` to `str`; this should trigger the type of `x` to be re-inferred
+        db.write_dedented(
+            "/src/mod.py",
+            r#"
+            class C:
+                def __init__(self):
+                    self.instance_attr: str = "24"
+
+                @classmethod
+                def method(cls):
+                    cls.class_attr: str = "42"
+            "#,
+        )?;
+
+        let events = {
+            db.clear_salsa_events();
+            let attr_ty = global_symbol(&db, file_main, "x").place.expect_type();
+            assert_eq!(attr_ty.display(&db).to_string(), "Unknown | str");
+            db.take_salsa_events()
+        };
+        assert_function_query_was_run(&db, infer_expression_types, x_rhs_expression(&db), &events);
+
+        // Add a comment; this should not trigger the type of `x` to be re-inferred
+        db.write_dedented(
+            "/src/mod.py",
+            r#"
+            class C:
+                def __init__(self):
+                    self.instance_attr: str = "24"
+
+                @classmethod
+                def method(cls):
+                    # comment
+                    cls.class_attr: str = "42"
+            "#,
+        )?;
+
+        let events = {
+            db.clear_salsa_events();
+            let attr_ty = global_symbol(&db, file_main, "x").place.expect_type();
+            assert_eq!(attr_ty.display(&db).to_string(), "Unknown | str");
+            db.take_salsa_events()
+        };
+
+        assert_function_query_was_not_run(
+            &db,
+            infer_expression_types,
+            x_rhs_expression(&db),
+            &events,
+        );
+
+        Ok(())
+    }
 }