Repository: python/cpython
Pull request: gh-122445: populate only modified fields in __static_attributes__
Fixes #122445.

<!--
Thanks for your contribution!
Please read this comment in its entirety. It's quite important.

# Pull Request title

It should be in the following format:

```
gh-NNNNN: Summary of the changes made
```

Where: gh-NNNNN refers to the GitHub issue number.

Most PRs will require an issue number. Trivial changes, like fixing a typo, do not need an issue.

# Backport Pull Request title

If this is a backport PR (PR made against branches other than `main`),
please ensure that the PR title is in the following format:

```
[X.Y] <title from the original PR> (GH-NNNN)
```

Where: [X.Y] is the branch name, e.g. [3.6].

GH-NNNN refers to the PR number from `main`.

-->


<!-- gh-issue-number: gh-122445 -->
* Issue: gh-122445
<!-- /gh-issue-number -->


Problem statement:
__static_attributes__ doesn't include method calls
# Feature or enhancement

### Proposal:

```python
class C:

  def f(self):
    self.x
    self.y[3]
    self.z()

print(C.__static_attributes__) # gives ('y', 'x'), but 'z' is absent
```
From 3.13 documentation, `__static_attributes__` is _A tuple containing names of attributes of this class which are accessed through self.X from any function in its body.`_. Is there a reason why subscriptions are included in the result, but not method calls ?

### Has this already been discussed elsewhere?

This is a minor feature, which does not need previous discussion elsewhere

### Links to previous discussion of this feature:

_No response_

<!-- gh-linked-prs -->
### Linked PRs
* gh-122446
* gh-122621
<!-- /gh-linked-prs -->

Hints from issue discussion:
cc @iritkatriel 
`__static_attributes__` is supposed to be a static approximation of the *instance* attributes, not the class's attributes.
So including methods is counter productive.

Maybe probably should only be counting stores, not loads when filling `__static_attributes__`?
> `__static_attributes__` is supposed to be a static approximation of the _instance_ attributes, not the class's attributes. So including methods is counter productive.
> 
> Maybe probably should only be counting stores, not loads when filling `__static_attributes__`?

I was thinking the same, but then we need to change the documentation as well to make this clear.

Commit message: enhance test

Patch to review:
diff --git a/Doc/reference/datamodel.rst b/Doc/reference/datamodel.rst
index 144c6f78ccd443..18df4d5af1ef62 100644
--- a/Doc/reference/datamodel.rst
+++ b/Doc/reference/datamodel.rst
@@ -1006,7 +1006,7 @@ Special attributes:
       a :ref:`generic class <generic-classes>`.
 
    :attr:`~class.__static_attributes__`
-      A tuple containing names of attributes of this class which are accessed
+      A tuple containing names of attributes of this class which are modified
       through ``self.X`` from any function in its body.
 
    :attr:`__firstlineno__`
diff --git a/Doc/whatsnew/3.13.rst b/Doc/whatsnew/3.13.rst
index fbf19d1c9598e1..298f1f5c10654b 100644
--- a/Doc/whatsnew/3.13.rst
+++ b/Doc/whatsnew/3.13.rst
@@ -247,7 +247,7 @@ Improved Error Messages
     TypeError: split() got an unexpected keyword argument 'max_split'. Did you mean 'maxsplit'?
 
 * Classes have a new :attr:`~class.__static_attributes__` attribute, populated by the compiler,
-  with a tuple of names of attributes of this class which are accessed
+  with a tuple of names of attributes of this class which are modified
   through ``self.X`` from any function in its body. (Contributed by Irit Katriel
   in :gh:`115775`.)
 
diff --git a/Lib/test/test_compile.py b/Lib/test/test_compile.py
index 9def47e101b496..4ebc605a3980f0 100644
--- a/Lib/test/test_compile.py
+++ b/Lib/test/test_compile.py
@@ -2089,12 +2089,15 @@ def f():
             self.assertEqual(end_col, 20)
 
 
-class TestExpectedAttributes(unittest.TestCase):
+class TestStaticAttributes(unittest.TestCase):
 
     def test_basic(self):
         class C:
             def f(self):
                 self.a = self.b = 42
+                # read fields are not included
+                self.f()
+                self.arr[3]
 
         self.assertIsInstance(C.__static_attributes__, tuple)
         self.assertEqual(sorted(C.__static_attributes__), ['a', 'b'])
diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-07-30-11-41-35.gh-issue-122445.Rq0bjS.rst b/Misc/NEWS.d/next/Core and Builtins/2024-07-30-11-41-35.gh-issue-122445.Rq0bjS.rst
new file mode 100644
index 00000000000000..f5aa07c6513ea9
--- /dev/null
+++ b/Misc/NEWS.d/next/Core and Builtins/2024-07-30-11-41-35.gh-issue-122445.Rq0bjS.rst	
@@ -0,0 +1 @@
+Add only fields which are modified via self.* to :attr:`~class.__static_attributes__`.
diff --git a/Python/compile.c b/Python/compile.c
index 02b5345cedd0a3..2d2ee1c6758942 100644
--- a/Python/compile.c
+++ b/Python/compile.c
@@ -563,8 +563,16 @@ compiler_unit_free(struct compiler_unit *u)
 }
 
 static int
-compiler_add_static_attribute_to_class(struct compiler *c, PyObject *attr)
+compiler_maybe_add_static_attribute_to_class(struct compiler *c, expr_ty e)
 {
+    assert(e->kind == Attribute_kind);
+    expr_ty attr_value = e->v.Attribute.value;
+    if (attr_value->kind != Name_kind ||
+        e->v.Attribute.ctx == Load ||
+        !_PyUnicode_EqualToASCIIString(attr_value->v.Name.id, "self"))
+    {
+        return SUCCESS;
+    }
     Py_ssize_t stack_size = PyList_GET_SIZE(c->c_stack);
     for (Py_ssize_t i = stack_size - 1; i >= 0; i--) {
         PyObject *capsule = PyList_GET_ITEM(c->c_stack, i);
@@ -573,7 +581,7 @@ compiler_add_static_attribute_to_class(struct compiler *c, PyObject *attr)
         assert(u);
         if (u->u_scope_type == COMPILER_SCOPE_CLASS) {
             assert(u->u_static_attributes);
-            RETURN_IF_ERROR(PySet_Add(u->u_static_attributes, attr));
+            RETURN_IF_ERROR(PySet_Add(u->u_static_attributes, e->v.Attribute.attr));
             break;
         }
     }
@@ -6065,11 +6073,7 @@ compiler_visit_expr(struct compiler *c, expr_ty e)
             ADDOP(c, loc, NOP);
             return SUCCESS;
         }
-        if (e->v.Attribute.value->kind == Name_kind &&
-            _PyUnicode_EqualToASCIIString(e->v.Attribute.value->v.Name.id, "self"))
-        {
-            RETURN_IF_ERROR(compiler_add_static_attribute_to_class(c, e->v.Attribute.attr));
-        }
+        RETURN_IF_ERROR(compiler_maybe_add_static_attribute_to_class(c, e));
         VISIT(c, expr, e->v.Attribute.value);
         loc = LOC(e);
         loc = update_start_location_to_match_attr(c, loc, e);


Code review comments:
diff_hunk: @@ -563,8 +563,16 @@ compiler_unit_free(struct compiler_unit *u)
 }
 
 static int
-compiler_add_static_attribute_to_class(struct compiler *c, PyObject *attr)
+compiler_maybe_add_static_attribute_to_class(struct compiler *c, expr_ty e)
 {
+    assert(e->kind == Attribute_kind);
+    expr_ty attr_value = e->v.Attribute.value;
+    if (attr_value->kind != Name_kind ||
+        e->v.Attribute.ctx == Load ||
path: Python/compile.c
text: @user1:
```suggestion
        e->v.Attribute.ctx != Store ||
```

I think it best to stick to stores, and ignore deletions. I doubt it will make any difference in practice and it is easier to document.

diff_hunk: @@ -1006,7 +1006,7 @@ Special attributes:
       a :ref:`generic class <generic-classes>`.
 
    :attr:`~class.__static_attributes__`
-      A tuple containing names of attributes of this class which are accessed
+      A tuple containing names of attributes of this class which are modified
path: Doc/reference/datamodel.rst
text: @user1:
Maybe "assigned to" rather than "modified through".

@author:
It could be del'ed as well.

@author:
Ah I just saw the other comment. Ok.