gh-119395: Fix leaky mangling after generic classes #119399
Conversation
JelleZijlstra
commented
May 22, 2024
•
- Issue: PEP 695: Name mangling leaks to surrounding scope #119395
| class Outer: | ||
| __before = "before" | ||
| class Inner[T]: | ||
| __x = "inner" |
There was a problem hiding this comment.
Is it a good idea to test __before = "class"; __after = "class" names in the class scope here? Or do we have them tested?
There was a problem hiding this comment.
You mean in the inner class? That wouldn't really be relevant here since there is nothing else interesting going on in the inner class body.
Python/compile.c
Outdated
| @@ -2624,6 +2624,7 @@ compiler_class(struct compiler *c, stmt_ty s) | |||
|
|
|||
| asdl_type_param_seq *type_params = s->v.ClassDef.type_params; | |||
| int is_generic = asdl_seq_LEN(type_params) > 0; | |||
| PyObject *old_u_private = Py_XNewRef(c->u->u_private); | |||
| if (is_generic) { | |||
| Py_XSETREF(c->u->u_private, Py_NewRef(s->v.ClassDef.name)); | |||
There was a problem hiding this comment.
The save-restore pattern is not needed or used in compiler_class_body, because there c->u->u_private is set after compiler_enter_scope, so it is set only on the inner compiler unit, not the outer one.
Setting it on the outer compiler unit works here, because compiler_enter_scope copies it to inner scopes. But is it required to set it on the outer unit? I don't see anything happening between here and the compiler_enter_scope call that seems to require mangling?
If we move this line down to after compiler_enter_scope, does that work and fix the bug, without requiring save/restore? If so, that seems simpler and more consistent with the intended/existing pattern for handling u_private.
Python/symtable.c
Outdated
| @@ -1673,13 +1672,15 @@ symtable_visit_stmt(struct symtable *st, stmt_ty s) | |||
| VISIT_QUIT(st, 0); | |||
| if (s->v.ClassDef.decorator_list) | |||
| VISIT_SEQ(st, expr, s->v.ClassDef.decorator_list); | |||
| PyObject *old_st_private = st->st_private; | |||
There was a problem hiding this comment.
I don't care too much either way, but I think it would be equivalent and simpler to move the existing pair of lines
tmp = st->st_private;
st->st_private = s->v.ClassDef.name;
up to right here. We want to unconditionally set it to the same thing either way.
If it's a problem to have st_private set during lines 1686 to 1691, then that would imply that your current version is problematic as well.
|
#119464 supersedes this PR, but keeping this one open for now since we may want to apply this change in 3.12. |