gh-116021: Deprecate support for instantiating abstract AST nodes#137865
gh-116021: Deprecate support for instantiating abstract AST nodes#137865brianschubert wants to merge 15 commits intopython:mainfrom
Conversation
Lib/test/test_sys.py
Outdated
| check(_ast.AST(), size('P')) | ||
| with self.assertWarns(DeprecationWarning): | ||
| check(_ast.AST(), size('P')) |
There was a problem hiding this comment.
I think this should probably be rewritten, but I'm not sure I understand the goal of this test well enough to do that. Suggestions welcome!
There was a problem hiding this comment.
It's to check that objects in Python/ have the correct types. I don't know why it's actually in test_sys though. I think we should split this test and move each check to the module it belongs (ast.AST's size should be checked in test_ast).
For now, leave it here. A separate issue should be opened.
There was a problem hiding this comment.
I think it's here because it's arguably testing sys.getsizeof. Maybe it could be changed to test a concrete AST class instead?
There was a problem hiding this comment.
check(_ast.Module(), size('3P')) might work then? (not sure if I'm counting the object size right)
There was a problem hiding this comment.
@JelleZijlstra @picnixz any further thoughts re: above? Happy to update this is someone can confirm a good substitute
There was a problem hiding this comment.
Yes, what you suggest should work (haven't verified the object size is right).
There was a problem hiding this comment.
Looking again 3P seems right, updated!
picnixz
left a comment
There was a problem hiding this comment.
Would it be easier to have a module function checking if a node is abstract? say _ast.is_abstract(node) (in the C module, not the Python module). That way we don't need an additional method on AST objects (which could be shadowed).
Lib/test/test_sys.py
Outdated
| check(_ast.AST(), size('P')) | ||
| with self.assertWarns(DeprecationWarning): | ||
| check(_ast.AST(), size('P')) |
There was a problem hiding this comment.
It's to check that objects in Python/ have the correct types. I don't know why it's actually in test_sys though. I think we should split this test and move each check to the module it belongs (ast.AST's size should be checked in test_ast).
For now, leave it here. A separate issue should be opened.
Parser/asdl_c.py
Outdated
| return result; | ||
| } | ||
|
|
||
| /* Helper for checking if a node class is abstract in the tests. */ |
There was a problem hiding this comment.
I think we should use clinic here, but this would create a new file as we don't use it at all. Maybe we can do it in a follow-up PR though.
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
3.14 + 5 = 3.19
Lib/test/test_sys.py
Outdated
| check(_ast.AST(), size('P')) | ||
| with self.assertWarns(DeprecationWarning): | ||
| check(_ast.AST(), size('P')) |
There was a problem hiding this comment.
I think it's here because it's arguably testing sys.getsizeof. Maybe it could be changed to test a concrete AST class instead?
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Demonstration:
This tracks which AST nodes are abstract using an internal set that gets built during node type initialization. I tried a few different approaches, and this seemed the simplest. A nice thing about this approach is that the check for abstract-ness depends on the exact node type, so this avoids most inheritance subtleties that could interfere with existing user-defined subclasses.
📚 Documentation preview 📚: https://cpython-previews--137865.org.readthedocs.build/