◐ Shell
reader mode source ↗
Skip to content

bpo-43244: Remove PyAST_Validate() function#24911

Merged
vstinner merged 1 commit into
python:masterfrom
vstinner:remove_ast_validate
Mar 18, 2021
Merged

bpo-43244: Remove PyAST_Validate() function#24911
vstinner merged 1 commit into
python:masterfrom
vstinner:remove_ast_validate

Conversation

@vstinner

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

Copy link
Copy Markdown
Member

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

vstinner commented Mar 17, 2021

Copy link
Copy Markdown
Member Author

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

@vstinner vstinner mentioned this pull request Mar 17, 2021
Merged
@vstinner

vstinner commented Mar 17, 2021

Copy link
Copy Markdown
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 commented Mar 17, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

@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

vstinner commented Mar 18, 2021

Copy link
Copy Markdown
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

vstinner commented Mar 18, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

vstinner commented Mar 18, 2021

Copy link
Copy Markdown
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