Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.4k
gh-105858: Improve AST node constructors#105880
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.Uh oh!
There was an error while loading.Please reload this page.
bedevere-bot commentedFeb 18, 2024
🤖 New build scheduled with the buildbot fleet by@JelleZijlstra for commita2442b6 🤖 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 OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| // 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. |
There was a problem hiding this comment.
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'tobject.__reduce__ (which IIRC uses__new__ to safely create an empty instance, then updates its__dict__) work fine for AST objects as well?
There was a problem hiding this comment.
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 donode = ast.FunctionDef(); node.__dict__.update(...), and the first line raises warnings because we don't set the requiredname field.
There was a problem hiding this comment.
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 whyobject.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). Soobject.reduce() is not equivalent tonode = ast.FunctionDef(); node.__dict__.update(...) -- it's more likenode = 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.
There was a problem hiding this comment.
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 argumentsI don't entirely understand what_reduce_ex is doing there, but it picksast.AST as the base, which fails.
There was a problem hiding this comment.
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.
Misc/NEWS.d/next/Core and Builtins/2023-06-16-21-29-06.gh-issue-105858.Q7h0EV.rstShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
This comment was marked as duplicate.
This comment was marked as duplicate.
Reproduces locally with |
We now use these in the AST parsing code afterpythongh-105880. A few comparable types (e.g.,NoneType) are already exposed as internal APIs.
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.
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.
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 aftergh-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 afterpythongh-105880. A few comparable types (e.g.,NoneType) are already exposed as internal APIs.
| 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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There are leaks ofname 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 afterpythongh-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 afterpythongh-105880. A few comparable types (e.g.,NoneType) are already exposed as internal APIs.
Python 3.13 added the warning which will become an error in Python 3.15(seepython/cpython#105880)
Uh oh!
There was an error while loading.Please reload this page.
Demonstration: