Skip to content
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

Jump to bottom

bpo-43244: Remove PyAST_Validate() function #24911

Merged
merged 1 commit into from
Mar 18, 2021
Merged

bpo-43244: Remove PyAST_Validate() function #24911

merged 1 commit into from
Mar 18, 2021

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Mar 17, 2021 •
edited by bedevere-bot
Loading

Rename PyAST_Validate() function to _PyAST_Validate() and move it to
the internal C API. Don't export the function anymore: replace
PyAPI_FUNC() with extern. The function was excluded from the limited
C API (PEP 384).

The function was added in bpo-12575 by
the commit 832bfe2.

https://bugs.python.org/issue43244

@vstinner
Copy link
Member Author

See also PR #24910 "Remove symtable.h header file".

@vstinner vstinner mentioned this pull request Mar 17, 2021
Merged
pablogsal
pablogsal reviewed Mar 17, 2021
View reviewed changes
Doc/whatsnew/3.10.rst Outdated
@@ -1353,3 +1353,6 @@ Removed
Python already implicitly installs signal handlers: see
:c:member:`PyConfig.install_signal_handlers`.
(Contributed by Victor Stinner in :issue:`41713`.)

* Removed ``PyAST_Validate()`` function.
Copy link
Member

@pablogsal pablogsal Mar 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
* Removed ``PyAST_Validate()`` function.
* Removed the ``PyAST_Validate()`` function from the public API as the AST symbols are no longer exposed. There is no replacement for this function.

@vstinner
Copy link
Member Author

@pablogsal @lysnikolaou @gvanrossum @isidentical: Would you be ok to remove the PyAST_Validate() function from the public C API?

I failed to find any user of it. A GitHub code search for PyAST_Validate (in C code) only lists copies of the CPython code base (ast.h file): I checked the first 100 results (10 pages).

I don't think that the low-level AST C API must be used outside CPython. My plan is to move the whole Python-ast.h header file to the internal C API as well.

In Python, you can validate an AST using compile() which calls PyAST_obj2mod(), PyAST_Validate() and PyAST_CompileObject() internally.

If you have doubt and prefer to stil expose the function, we can export it but I would prefer to still rename it.

Note: PyAST_Validate() is excluded from the limited C API (PEP 384).

pablogsal
pablogsal reviewed Mar 17, 2021
View reviewed changes
Misc/NEWS.d/next/C API/2021-03-17-23-20-07.bpo-43244.diyn2C.rst Outdated
@@ -0,0 +1 @@
Remove ``PyAST_Validate()`` function. Patch by Victor Stinner.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Remove ``PyAST_Validate()`` function. Patch by Victor Stinner.
Remove the ``PyAST_Validate()`` function from the public API as the AST symbols are no longer exposed. There is no replacement for this function. Patch by Victor Stinner.

@pablogsal
Copy link
Member

pablogsal commented Mar 17, 2021 •
edited
Loading

@pablogsal @lysnikolaou @gvanrossum @isidentical: Would you be ok to remove the PyAST_Validate() function from the public C API?

Yes, the ast symbols are not exposed anymore so this function cannot be used without magical cast to void*:

In 3.8:

❯ nm ./python | grep _Py_While
00000000002384c0 t _Py_While

in 3.9+:

❯ nm ./python | grep _Py_While
(no output)

@vstinner
Copy link
Member Author

test_peg_generator fails. I'm working on a fix: PR #24912 defines also Py_BUILD_CORE macro in test_peg_generator.

@vstinner
Copy link
Member Author

vstinner commented Mar 18, 2021 •
edited
Loading

I searched PyAST_Validate() in the top 4000 PyPI packages: it is not used.

A single package contains PyAST_Validate(): graphene-federation 0.1.0. But its tarball contains a full virtual environment which contains a Python binary (!) which contains the "PyAST_Validate" string. It can be ignored ;-)

@vstinner
bpo-43244: Remove the PyAST_Validate() function
38ec882 
Remove the PyAST_Validate() function. It is no longer possible to
build a AST object (mod_ty type) with the public C API. The function
was already excluded from the limited C API (PEP 384).

Rename PyAST_Validate() function to _PyAST_Validate(), move it to the
internal C API, and don't export it anymore (replace PyAPI_FUNC with
extern).

The function was added in bpo-12575 by
the commit 832bfe2.
shihai1991
shihai1991 reviewed Mar 18, 2021
View reviewed changes
Doc/whatsnew/3.10.rst

* Remove the ``PyAST_Validate()`` function. It is no longer possible to build a
AST object (``mod_ty`` type) with the public C API. The function was already
excluded from the limited C API (:pep:`384`).
Copy link
Member

Choose a reason for hiding this comment

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

Hm, pep-384 doesn't mention the ast.h. Can we move the capi directly when this C API is not a limited C API?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just mentioned PEP 384 to explain what is the limited C API.

PyAST_Validate() definition is inside:

#ifndef Py_LIMITED_API
...
#endif /* !Py_LIMITED_API */

Copy link
Member Author

Choose a reason for hiding this comment

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

"Can we move the capi directly when this C API is not a limited C API?" No. I only removed the function since it cannot be used anymore since Python 3.10: " It is no longer possible to build a AST object (mod_ty type) with the public C API."

Copy link
Member

Choose a reason for hiding this comment

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

OK, Thanks. got it.

@vstinner vstinner merged commit eec8e61 into python:master Mar 18, 2021
@vstinner vstinner deleted the remove_ast_validate branch March 18, 2021 13:57
@vstinner
Copy link
Member Author

Pablo approved my PR. I failed to find usage of this function in GitHub code search and in the PyPI top 4000 packages. So I was confident enough to merge this PR.

Thanks for the review Pablo.

jab added a commit to jab/cpython that referenced this pull request Mar 20, 2021
@jab
Merge branch 'master' into justin39/aiter-c
97d06e5 
* master: (129 commits)
  bpo-43452: Micro-optimizations to PyType_Lookup (pythonGH-24804)
  bpo-43517: Fix false positive in detection of circular imports (python#24895)
  bpo-43494: Make some minor changes to lnotab notes (pythonGH-24861)
  Mention that code.co_lnotab is deprecated in what's new for 3.10. (python#24902)
  bpo-43244: Remove symtable.h header file (pythonGH-24910)
  bpo-43466: Add --with-openssl-rpath configure option (pythonGH-24820)
  Fix a typo in c-analyzer (pythonGH-24468)
  bpo-41561: Add workaround for Ubuntu's custom security level (pythonGH-24915)
  bpo-43521: Allow ast.unparse with empty sets and NaN (pythonGH-24897)
  bpo-43244: Remove the PyAST_Validate() function (pythonGH-24911)
  bpo-43541: Fix PyEval_EvalCodeEx() regression (pythonGH-24918)
  bpo-43244: Fix test_peg_generators on Windows (pythonGH-24913)
  bpo-39342: Expose X509_V_FLAG_ALLOW_PROXY_CERTS in ssl module (pythonGH-18011)
  bpo-43244: Fix test_peg_generator for PyAST_Validate() (pythonGH-24912)
  bpo-42128: Add 'missing :' syntax error message to match statements (pythonGH-24733)
  bpo-43244: Add pycore_ast.h header file (pythonGH-24908)
  bpo-43244: Rename pycore_ast.h to pycore_ast_state.h (pythonGH-24907)
  Remove unnecessary imports in the grammar parser (pythonGH-24904)
  bpo-35883: Py_DecodeLocale() escapes invalid Unicode characters (pythonGH-24843)
  Add PEP 626 to what's new in 3.10. (python#24892)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shihai1991 shihai1991 shihai1991 left review comments

@pablogsal pablogsal pablogsal approved these changes

@isidentical isidentical Awaiting requested review from isidentical

@lysnikolaou lysnikolaou Awaiting requested review from lysnikolaou

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vstinner @pablogsal @shihai1991 @the-knights-who-say-ni @bedevere-bot