gh-105858: Improve AST node constructors #105880

JelleZijlstra · 2023-06-17T04:31:18Z

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()
<stdin>:1: DeprecationWarning: FunctionDef.__init__ missing 1 required positional argument: 'name'. This will become an error in Python 3.15.
<stdin>: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())
<stdin>: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() <stdin>:1: DeprecationWarning: FunctionDef.__init__ missing 1 required positional argument: 'name'. This will become an error in Python 3.15. <stdin>: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()) <stdin>: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.

Parser/asdl_c.py

bedevere-bot · 2024-02-18T14:51:05Z

🤖 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.

Misc/NEWS.d/next/Core and Builtins/2023-06-16-21-29-06.gh-issue-105858.Q7h0EV.rst

Parser/asdl_c.py

carljm · 2024-02-21T00:36:44Z

Parser/asdl_c.py

+        // 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.


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?

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.

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.

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.

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.

Misc/NEWS.d/next/Core and Builtins/2023-06-16-21-29-06.gh-issue-105858.Q7h0EV.rst

carljm

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

Doc/library/ast.rst

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

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() <stdin>:1: DeprecationWarning: FunctionDef.__init__ missing 1 required positional argument: 'name'. This will become an error in Python 3.15. <stdin>: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()) <stdin>: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>

JelleZijlstra · 2024-02-28T02:27:08Z

Clearly something is horribly wrong with the build, I'll take a look soon.

JelleZijlstra · 2024-02-28T02:42:09Z

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.

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.

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() <stdin>:1: DeprecationWarning: FunctionDef.__init__ missing 1 required positional argument: 'name'. This will become an error in Python 3.15. <stdin>: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()) <stdin>: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>

) 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.

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() <stdin>:1: DeprecationWarning: FunctionDef.__init__ missing 1 required positional argument: 'name'. This will become an error in Python 3.15. <stdin>: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()) <stdin>: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>

…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.

serhiy-storchaka · 2024-03-06T21:59:09Z

Parser/asdl_c.py

+                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;
+                }


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

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() <stdin>:1: DeprecationWarning: FunctionDef.__init__ missing 1 required positional argument: 'name'. This will become an error in Python 3.15. <stdin>: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()) <stdin>: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>

…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.

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() <stdin>:1: DeprecationWarning: FunctionDef.__init__ missing 1 required positional argument: 'name'. This will become an error in Python 3.15. <stdin>: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()) <stdin>: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>

…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.

bedevere-bot mentioned this pull request Jun 17, 2023

Improve the constructors of AST nodes #105858

Closed

JelleZijlstra added 7 commits June 21, 2023 18:42

Merge remote-tracking branch 'upstream/main' into astcons

d417cf3

Fix subclasses

c74740a

Fix most deprecationwarnings in test_ast

05d5569

Merge remote-tracking branch 'upstream/main' into astcons

f4b14ac

Fix renamed function

0efcdf3

Fix pickling; fix a refleak

f941696

Fix other refleak

4c42748

JelleZijlstra marked this pull request as ready for review October 24, 2023 19:55

JelleZijlstra requested a review from isidentical as a code owner October 24, 2023 19:55

bedevere-app bot added the awaiting core review label Oct 24, 2023

JelleZijlstra added 2 commits October 24, 2023 12:55

Merge branch 'main' into astcons

1a9b872

Merge branch 'main' into astcons

db16cc7

JelleZijlstra commented Feb 18, 2024

View reviewed changes

Parser/asdl_c.py Outdated Show resolved Hide resolved

JelleZijlstra added 2 commits February 18, 2024 06:39

Update Parser/asdl_c.py

6bf677b

fix merge

a2442b6

JelleZijlstra added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Feb 18, 2024

bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Feb 18, 2024

JelleZijlstra requested a review from carljm February 19, 2024 05:08

carljm reviewed Feb 21, 2024

View reviewed changes

Expand NEWS

5a1d37a

carljm approved these changes Feb 21, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Feb 21, 2024

JelleZijlstra added 2 commits February 25, 2024 14:40

Merge remote-tracking branch 'upstream/main' into astcons

c4627a1

docs

1f85438

AlexWaygood reviewed Feb 25, 2024

View reviewed changes

Doc/library/ast.rst Outdated Show resolved Hide resolved

Doc/library/ast.rst Outdated Show resolved Hide resolved

Doc/library/ast.rst Outdated Show resolved Hide resolved

Apply suggestions from code review

ff0cc67

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

This comment was marked as duplicate.

Sign in to view

JelleZijlstra mentioned this pull request Feb 28, 2024

gh-105858: Expose some union-related objects as internal APIs #116025

Merged

This comment was marked as duplicate.

Sign in to view

encukou pushed a commit that referenced this pull request Feb 28, 2024

gh-105858: Expose some union-related objects as internal APIs (GH-116025

d53560d

) 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.

serhiy-storchaka reviewed Mar 6, 2024

View reviewed changes

JelleZijlstra mentioned this pull request May 19, 2024

bpo-39981: Introduce default values for AST node classes #21417

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-105858: Improve AST node constructors #105880

gh-105858: Improve AST node constructors #105880

JelleZijlstra commented Jun 17, 2023 •

edited

bedevere-bot commented Feb 18, 2024

carljm Feb 21, 2024

JelleZijlstra Feb 21, 2024

carljm Feb 21, 2024

JelleZijlstra Feb 21, 2024

carljm Feb 21, 2024

carljm left a comment

This comment was marked as duplicate.

JelleZijlstra commented Feb 28, 2024

This comment was marked as duplicate.

JelleZijlstra commented Feb 28, 2024

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

serhiy-storchaka Mar 6, 2024

gh-105858: Improve AST node constructors #105880

gh-105858: Improve AST node constructors #105880

Conversation

JelleZijlstra commented Jun 17, 2023 • edited

bedevere-bot commented Feb 18, 2024

carljm Feb 21, 2024

Choose a reason for hiding this comment

JelleZijlstra Feb 21, 2024

Choose a reason for hiding this comment

carljm Feb 21, 2024

Choose a reason for hiding this comment

JelleZijlstra Feb 21, 2024

Choose a reason for hiding this comment

carljm Feb 21, 2024

Choose a reason for hiding this comment

carljm left a comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

JelleZijlstra commented Feb 28, 2024

This comment was marked as duplicate.

JelleZijlstra commented Feb 28, 2024

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

serhiy-storchaka Mar 6, 2024

Choose a reason for hiding this comment

JelleZijlstra commented Jun 17, 2023 •

edited