gh-105858: Improve AST node constructors by JelleZijlstra · Pull Request #105880 · python/cpython (original) (raw)

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.

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

Already on GitHub?Sign in to your account

Conversation50 Commits18 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Demonstration:

ast.FunctionDef.annotations {'name': <class 'str'>, 'args': <class 'ast.arguments'>, 'body': list[ast.stmt], 'decorator_list': list[ast.expr], 'returns': ast.expr | None, 'type_comment': str | None, 'type_params': list[ast.type_param]} ast.FunctionDef() :1: DeprecationWarning: FunctionDef.init missing 1 required positional argument: 'name'. This will become an error in Python 3.15. :1: DeprecationWarning: FunctionDef.init missing 1 required positional argument: 'args'. This will become an error in Python 3.15. <ast.FunctionDef object at 0x101959460> node = ast.FunctionDef(name="foo", args=ast.arguments()) node.decorator_list [] ast.FunctionDef(whatever="you want", name="x", args=ast.arguments()) :1: DeprecationWarning: FunctionDef.init got an unexpected keyword argument 'whatever'. Support for arbitrary keyword arguments is deprecated and will be removed in Python 3.15. <ast.FunctionDef object at 0x1019581f0>

Issue: Improve the constructors of AST nodes #105858

Demonstration:

ast.FunctionDef.annotations {'name': <class 'str'>, 'args': <class 'ast.arguments'>, 'body': list[ast.stmt], 'decorator_list': list[ast.expr], 'returns': ast.expr | None, 'type_comment': str | None, 'type_params': list[ast.type_param]} ast.FunctionDef() :1: DeprecationWarning: FunctionDef.init missing 1 required positional argument: 'name'. This will become an error in Python 3.15. :1: DeprecationWarning: FunctionDef.init missing 1 required positional argument: 'args'. This will become an error in Python 3.15. <ast.FunctionDef object at 0x101959460> node = ast.FunctionDef(name="foo", args=ast.arguments()) node.decorator_list [] ast.FunctionDef(whatever="you want", name="x", args=ast.arguments()) :1: DeprecationWarning: FunctionDef.init got an unexpected keyword argument 'whatever'. Support for arbitrary keyword arguments is deprecated and will be removed in Python 3.15. <ast.FunctionDef object at 0x1019581f0>

Known problems:

Subclasses of AST nodes don't work properly, because we don't look up annotations on the right class.
Unpickling throws DeprecationWarnings, probably because of how we construct the unpickled object.

Need to think more about how to handle those cases.

🤖 New build scheduled with the buildbot fleet by @JelleZijlstra for commit a2442b6 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

Comment on lines +1053 to +1056

// Serialize the fields as positional args if possible, because if we
// serialize them as a dict, during unpickling they are set only after
// the object is constructed, which will now trigger a DeprecationWarning
// if the AST type has required fields.

Choose a reason for hiding this comment

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

Why do AST types even need their own custom reduce function? Shouldn't object.__reduce__ (which IIRC uses __new__ to safely create an empty instance, then updates its __dict__) work fine for AST objects as well?

Choose a reason for hiding this comment

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

The __new__ call will trigger DeprecationWarnings. If we don't have our own reducer, unpickling will essentially do node = ast.FunctionDef(); node.__dict__.update(...), and the first line raises warnings because we don't set the required name field.

Choose a reason for hiding this comment

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

Why would the __new__ call raise deprecation warnings? Aren't the deprecation warnings in __init__, not __new__? That's why object.reduce() uses __new__, so that it can bypass whatever is happening in custom __init__ methods (otherwise lots of custom classes would fail to unpickle by default). So object.reduce() is not equivalent to node = ast.FunctionDef(); node.__dict__.update(...) -- it's more like node = ast.FunctionDef.__new__(ast.FunctionDef); node.__dict__.update(...), which is a critical difference. So it seems to me like using the default reducer should work fine here, and not require all this extra work. But I could definitely be missing something; there's probably some reason these nodes had a custom reduce() in the first place.

Choose a reason for hiding this comment

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

I see what you mean. I tried commenting out the __reduce__ definition, and lots of tests fail with:

ERROR: test_pickling (test.test_ast.AST_Tests.test_pickling) (ast=<ast.Module object at 0x102323da0>, protocol=1)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jelle/py/cpython/Lib/test/test_ast.py", line 977, in test_pickling
    ast2 = pickle.loads(pickle.dumps(ast, protocol))
                        ~~~~~~~~~~~~^^^^^^^^^^^^^^^
  File "/Users/jelle/py/cpython/Lib/copyreg.py", line 71, in _reduce_ex
    state = base(self)
            ~~~~^^^^^^
TypeError: AST constructor takes at most 0 positional arguments

I don't entirely understand what _reduce_ex is doing there, but it picks ast.AST as the base, which fails.

Choose a reason for hiding this comment

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

Interesting. Well, I might have just nerd-sniped myself into digging into this more later to understand what's happening, but I don't think it should block the PR; your added code to use positional args works fine.

Choose a reason for hiding this comment

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

If you are satisfied with the Discourse thread outcome, the code here LGTM! Thanks for this improvement.

Co-authored-by: Alex Waygood Alex.Waygood@Gmail.com

This comment was marked as duplicate.

Reproduces locally with ./python.exe -m test -u cpu test_peg_generator -v. Now looking into ways to let the extension build by the PEG generator access this type, or find some workaround.

JelleZijlstra added a commit to JelleZijlstra/cpython that referenced this pull request

Feb 28, 2024

We now use these in the AST parsing code after pythongh-105880. A few comparable types (e.g., NoneType) are already exposed as internal APIs.

This comment was marked as duplicate.

gvanrossum pushed a commit to gvanrossum/cpython that referenced this pull request

Feb 28, 2024

Demonstration:

ast.FunctionDef.annotations {'name': <class 'str'>, 'args': <class 'ast.arguments'>, 'body': list[ast.stmt], 'decorator_list': list[ast.expr], 'returns': ast.expr | None, 'type_comment': str | None, 'type_params': list[ast.type_param]} ast.FunctionDef() :1: DeprecationWarning: FunctionDef.init missing 1 required positional argument: 'name'. This will become an error in Python 3.15. :1: DeprecationWarning: FunctionDef.init missing 1 required positional argument: 'args'. This will become an error in Python 3.15. <ast.FunctionDef object at 0x101959460> node = ast.FunctionDef(name="foo", args=ast.arguments()) node.decorator_list [] ast.FunctionDef(whatever="you want", name="x", args=ast.arguments()) :1: DeprecationWarning: FunctionDef.init got an unexpected keyword argument 'whatever'. Support for arbitrary keyword arguments is deprecated and will be removed in Python 3.15. <ast.FunctionDef object at 0x1019581f0>

encukou pushed a commit that referenced this pull request

Feb 28, 2024

)

We now use these in the AST parsing code after gh-105880. A few comparable types (e.g., NoneType) are already exposed as internal APIs.

woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request

Mar 4, 2024

Demonstration:

ast.FunctionDef.annotations {'name': <class 'str'>, 'args': <class 'ast.arguments'>, 'body': list[ast.stmt], 'decorator_list': list[ast.expr], 'returns': ast.expr | None, 'type_comment': str | None, 'type_params': list[ast.type_param]} ast.FunctionDef() :1: DeprecationWarning: FunctionDef.init missing 1 required positional argument: 'name'. This will become an error in Python 3.15. :1: DeprecationWarning: FunctionDef.init missing 1 required positional argument: 'args'. This will become an error in Python 3.15. <ast.FunctionDef object at 0x101959460> node = ast.FunctionDef(name="foo", args=ast.arguments()) node.decorator_list [] ast.FunctionDef(whatever="you want", name="x", args=ast.arguments()) :1: DeprecationWarning: FunctionDef.init got an unexpected keyword argument 'whatever'. Support for arbitrary keyword arguments is deprecated and will be removed in Python 3.15. <ast.FunctionDef object at 0x1019581f0>

woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request

Mar 4, 2024

…ythonGH-116025)

We now use these in the AST parsing code after pythongh-105880. A few comparable types (e.g., NoneType) are already exposed as internal APIs.

Comment on lines +1081 to +1092

if (!value) {
if (PyErr_Occurred()) {
goto cleanup;
}
break;
}
if (PyList_Append(positional_args, value) < 0) {
goto cleanup;
}
if (PyDict_DelItem(remaining_dict, name) < 0) {
goto cleanup;
}

Choose a reason for hiding this comment

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

There are leaks of name here. They will be fixed in #116438.

adorilson pushed a commit to adorilson/cpython that referenced this pull request

Mar 25, 2024

Demonstration:

ast.FunctionDef.annotations {'name': <class 'str'>, 'args': <class 'ast.arguments'>, 'body': list[ast.stmt], 'decorator_list': list[ast.expr], 'returns': ast.expr | None, 'type_comment': str | None, 'type_params': list[ast.type_param]} ast.FunctionDef() :1: DeprecationWarning: FunctionDef.init missing 1 required positional argument: 'name'. This will become an error in Python 3.15. :1: DeprecationWarning: FunctionDef.init missing 1 required positional argument: 'args'. This will become an error in Python 3.15. <ast.FunctionDef object at 0x101959460> node = ast.FunctionDef(name="foo", args=ast.arguments()) node.decorator_list [] ast.FunctionDef(whatever="you want", name="x", args=ast.arguments()) :1: DeprecationWarning: FunctionDef.init got an unexpected keyword argument 'whatever'. Support for arbitrary keyword arguments is deprecated and will be removed in Python 3.15. <ast.FunctionDef object at 0x1019581f0>

adorilson pushed a commit to adorilson/cpython that referenced this pull request

Mar 25, 2024

…ythonGH-116025)

We now use these in the AST parsing code after pythongh-105880. A few comparable types (e.g., NoneType) are already exposed as internal APIs.

diegorusso pushed a commit to diegorusso/cpython that referenced this pull request

Apr 17, 2024

Demonstration:

ast.FunctionDef.annotations {'name': <class 'str'>, 'args': <class 'ast.arguments'>, 'body': list[ast.stmt], 'decorator_list': list[ast.expr], 'returns': ast.expr | None, 'type_comment': str | None, 'type_params': list[ast.type_param]} ast.FunctionDef() :1: DeprecationWarning: FunctionDef.init missing 1 required positional argument: 'name'. This will become an error in Python 3.15. :1: DeprecationWarning: FunctionDef.init missing 1 required positional argument: 'args'. This will become an error in Python 3.15. <ast.FunctionDef object at 0x101959460> node = ast.FunctionDef(name="foo", args=ast.arguments()) node.decorator_list [] ast.FunctionDef(whatever="you want", name="x", args=ast.arguments()) :1: DeprecationWarning: FunctionDef.init got an unexpected keyword argument 'whatever'. Support for arbitrary keyword arguments is deprecated and will be removed in Python 3.15. <ast.FunctionDef object at 0x1019581f0>

diegorusso pushed a commit to diegorusso/cpython that referenced this pull request

Apr 17, 2024

…ythonGH-116025)

We now use these in the AST parsing code after pythongh-105880. A few comparable types (e.g., NoneType) are already exposed as internal APIs.

cedk added a commit to cedk/genshi that referenced this pull request

May 11, 2025

cedk mentioned this pull request

May 11, 2025