Skip to content

gh-119395: Fix leaky mangling after generic classes #119399

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
JelleZijlstra wants to merge 4 commits into from
Closed

gh-119395: Fix leaky mangling after generic classes #119399

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6bb58cb
Fix leaky mangling
JelleZijlstra May 22, 2024
3a4e387
blurb
JelleZijlstra May 22, 2024
718e28e
Merge remote-tracking branch 'upstream/main' into pep695-leakymangle
JelleZijlstra May 23, 2024
ac26abc
simplify
JelleZijlstra May 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Lib/importlib/_bootstrap_external.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,7 @@ def _write_atomic(path, data, mode=0o666):
# Python 3.13a5 3569 (Specialize CONTAINS_OP)
# Python 3.13a6 3570 (Add __firstlineno__ class attribute)
# Python 3.14a1 3600 (Add LOAD_COMMON_CONSTANT)
# Python 3.14a1 3601 (Fix leaky name mangling in generic classes)

# Python 3.15 will start with 3700

Expand All @@ -489,7 +490,7 @@ def _write_atomic(path, data, mode=0o666):
# Whenever MAGIC_NUMBER is changed, the ranges in the magic_values array
# in PC/launcher.c must also be updated.

MAGIC_NUMBER = (3571).to_bytes(2, 'little') + b'\r\n'
MAGIC_NUMBER = (3601).to_bytes(2, 'little') + b'\r\n'

_RAW_MAGIC_NUMBER = int.from_bytes(MAGIC_NUMBER, 'little') # For import.c

Expand Down
33 changes: 33 additions & 0 deletions Lib/test/test_type_params.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -823,6 +823,39 @@ def meth[__U](self, arg: __T, arg2: __U):

self.assertEqual(Foo.Alias.__value__, (T, V))

def test_no_leaky_mangling_in_module(self):
ns = run_code("""
__before = "before"
class X[T]: pass
__after = "after"
""")
self.assertEqual(ns["__before"], "before")
self.assertEqual(ns["__after"], "after")

def test_no_leaky_mangling_in_function(self):
ns = run_code("""
def f():
class X[T]: pass
_X_foo = 2
__foo = 1
assert locals()['__foo'] == 1
return __foo
""")
self.assertEqual(ns["f"](), 1)

def test_no_leaky_mangling_in_class(self):
ns = run_code("""
class Outer:
__before = "before"
class Inner[T]:
__x = "inner"

@sobolevn sobolevn May 22, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a good idea to test __before = "class"; __after = "class" names in the class scope here? Or do we have them tested?

@JelleZijlstra JelleZijlstra May 22, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

__after = "after"
""")
Outer = ns["Outer"]
self.assertEqual(Outer._Outer__before, "before")
self.assertEqual(Outer.Inner._Inner__x, "inner")
self.assertEqual(Outer._Outer__after, "after")


class TypeParamsComplexCallsTest(unittest.TestCase):
def test_defaults(self):
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Core and Builtins/2024-05-22-05-51-23.gh-issue-119399.4zmHsV.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix bug where any names appearing after a generic class were mangled as if
they were part of the generic class.
2 changes: 2 additions & 0 deletions Python/compile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));

@carljm carljm May 22, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

PyObject *type_params_name = PyUnicode_FromFormat("<generic parameters of %U>",
Expand Down Expand Up @@ -2701,6 +2702,7 @@ compiler_class(struct compiler *c, stmt_ty s)
s->v.ClassDef.bases,
s->v.ClassDef.keywords));
}
Py_XSETREF(c->u->u_private, old_u_private);

/* 6. apply decorators */
RETURN_IF_ERROR(compiler_apply_decorators(c, decos));
Expand Down
4 changes: 3 additions & 1 deletion Python/symtable.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1508,7 +1508,6 @@ symtable_enter_type_param_block(struct symtable *st, identifier name,
lineno, col_offset, end_lineno, end_col_offset)) {
return 0;
}
st->st_private = name;
// This is used for setting the generic base
_Py_DECLARE_STR(generic_base, ".generic_base");
if (!symtable_add_def(st, &_Py_STR(generic_base), DEF_LOCAL,
Expand Down Expand Up @@ -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;

@carljm carljm May 22, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@JelleZijlstra JelleZijlstra May 23, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would introduce the #119311 bug to non-generic classes too. However, you're right that this can be simplified. I'm about to open an alternative PR that fixes both this and #119311.

if (asdl_seq_LEN(s->v.ClassDef.type_params) > 0) {
if (!symtable_enter_type_param_block(st, s->v.ClassDef.name,
(void *)s->v.ClassDef.type_params,
false, false, s->kind,
LOCATION(s))) {
VISIT_QUIT(st, 0);
}
st->st_private = s->v.ClassDef.name;
VISIT_SEQ(st, type_param, s->v.ClassDef.type_params);
}
VISIT_SEQ(st, expr, s->v.ClassDef.bases);
Expand Down Expand Up @@ -1709,6 +1710,7 @@ symtable_visit_stmt(struct symtable *st, stmt_ty s)
if (!symtable_exit_block(st))
VISIT_QUIT(st, 0);
}
st->st_private = old_st_private;
break;
}
case TypeAlias_kind: {
Expand Down