bpo-43244: Remove symtable.h header file #24910

vstinner · 2021-03-17T23:09:43Z

Move the symtable.h header file to the internal C API as
pycore_symtable.h. Don't export symbols anymore: replace PyAPI_FUNC()
and PyAPI_DATA() with extern. Rename functions:

PyST_GetScope() to _PyST_GetScope()
PySymtable_Build() to _PySymtable_Build()
PySymtable_BuildObject() to _PySymtable_BuildObject()
PySymtable_Free() to _PySymtable_Free()

Remove also Py_SymtableString() (was part of the stable ABI) and
Py_SymtableStringObject() functions.

The Python symtable module remains available and is unchanged.

https://bugs.python.org/issue43244

vstinner · 2021-03-17T23:35:01Z

@pablogsal @lysnikolaou @gvanrossum @isidentical: Are you ok to remove the symbol table C API from the public C API?

I failed to find any user of it. A search for PySymtable_Free in GitHub code search (C code) only lists copies of the CPython code base (symtablemodule.c and symtable.h files): I looked at the first 50 results (5 pages).

For me, the symbol table is an interdemediate state which is only used internally by the Python compiler, but it should not be exposed outside Python.

If you consider that maybe someone somewhere might use it, we can continue to export symbols in libpython. But I would prefer to still rename functions (replace Py with _Py) if we remove them from the public C API.

The PEP 384 https://www.python.org/dev/peps/pep-0384/ explicitly excluded symtable.h from the limited C API (stable ABI)... But it seems like sadly Py_SymtableString() landed into the stable ABI by mistake. IMO it's perfectly safe to remove it. It returns the struct symtable* type which is excluded from the limited C API.

The Python symtable module remains available and is unchanged.

pablogsal · 2021-03-17T23:37:34Z

For me, the symbol table is an interdemediate state which is only used internally by the Python compiler, but it should not be exposed outside Python.

Yeah, I will be +1 of removing it from the public C AP. Sharing low-level C structs publicly have proven to put us in corners that can be uncomfortable and this module (the python version and these functions from the C-API) is very niche so it should not affect anyone.

landed into the stable ABI by mistake.

As PEP 384 doesn't include it in the stable ABI I think is fine because we never publicly documented it (is also not in the docs AFAUK).

vstinner · 2021-03-17T23:38:49Z

See also PR #24911 "Remove PyAST_Validate() function".

vstinner · 2021-03-18T00:55:58Z

Rename functions:
PyST_GetScope() to _PyST_GetScope()
PySymtable_Build() to _PySymtable_Build()
PySymtable_BuildObject() to _PySymtable_BuildObject()
PySymtable_Free() to _PySymtable_Free()

I searched for these 4 functions in the top 4000 PyPI packages: they are not used.

Only graphene-federation-0.1.0.tar.gz contains them, but it's because the tarball contains a whole copy of a virtual environement which contains a "python" binary (!) which contains the symbols.

vstinner · 2021-03-18T02:00:11Z

I modified the PR to clarify that Py_SymtableString() was not usable with the stable ABI:

The Py_SymtableString() function was part the stable ABI by mistake
but it could not be used, because the symtable.h header file was
excluded from the limited C API.

Rename Include/symtable.h to to Include/internal/pycore_symtable.h, don't export symbols anymore (replace PyAPI_FUNC and PyAPI_DATA with extern) and rename functions: * PyST_GetScope() to _PyST_GetScope() * PySymtable_BuildObject() to _PySymtable_Build() * PySymtable_Free() to _PySymtable_Free() Remove PySymtable_Build(), Py_SymtableString() and Py_SymtableStringObject() functions. The Py_SymtableString() function was part the stable ABI by mistake but it could not be used, since the symtable.h header file was excluded from the limited C API. The Python symtable module remains available and is unchanged.

vstinner · 2021-03-18T15:19:56Z

@pablogsal: Would you mind to formally approve the PR and review the updated PR? I'm not sure if you clearly approved it or not :-)

pablogsal · 2021-03-18T15:21:45Z

I will review this today. Ping me if I have not done it by EOD

pablogsal · 2021-03-18T22:53:08Z

Doc/whatsnew/3.10.rst

@@ -1358,3 +1358,19 @@ Removed
  AST object (``mod_ty`` type) with the public C API. The function was already
  excluded from the limited C API (:pep:`384`).
  (Contributed by Victor Stinner in :issue:`43244`.)
+
+* Remove the ``symtable.h`` header file and the undocumented functions:


Not sure if this should be here, as it was never documented

Previously, I didn't document such changes, but there is always one project sleeping somewhere which relies on the modified/removed function, and then it's painful to understand why the project is broken. Even if a function is very well hidden and not documented, there is always a secret project relying on it :-)

pablogsal · 2021-03-18T22:53:39Z

Doc/whatsnew/3.10.rst

+  it could not be used, because the ``symtable.h`` header file was excluded
+  from the limited C API.
+
+  The Python :mod:`symtable` module remains available and is unchanged.


I would remove this, we are in the C-API changes section already.

It's to say that the function remains available, it's just that the low-level C API is no longer available. It's possible to use the symtable module in C.

pablogsal · 2021-03-18T22:54:52Z

Python/symtable.c

@@ -260,7 +260,7 @@ symtable_new(void)
 #define COMPILER_STACK_FRAME_SCALE 3

 struct symtable *
-PySymtable_BuildObject(mod_ty mod, PyObject *filename, PyFutureFeatures *future)
+_PySymtable_Build(mod_ty mod, PyObject *filename, PyFutureFeatures *future)


Why the rename?

It's to prevent people to rely on it. It's to advertize: hey, it's a private function, don't use it!

pablogsal

LGTM in general, I left some comments. Feel free to ignore them if you prefer

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

vstinner requested review from markshannon and a team as code owners March 17, 2021 23:09

the-knights-who-say-ni added the CLA signed label Mar 17, 2021

bedevere-bot added the awaiting core review label Mar 17, 2021

vstinner mentioned this pull request Mar 17, 2021

bpo-43244: Remove PyAST_Validate() function #24911

Merged

pablogsal reviewed Mar 18, 2021

View reviewed changes

pablogsal approved these changes Mar 18, 2021

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Mar 18, 2021

vstinner merged commit 28ad12f into python:master Mar 19, 2021

bedevere-bot removed the awaiting merge label Mar 19, 2021

vstinner deleted the pycore_symtable branch March 19, 2021 11:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-43244: Remove symtable.h header file #24910

bpo-43244: Remove symtable.h header file #24910

vstinner commented Mar 17, 2021 •

edited

Loading

vstinner commented Mar 17, 2021

pablogsal commented Mar 17, 2021 •

edited

Loading

vstinner commented Mar 17, 2021

vstinner commented Mar 18, 2021

vstinner commented Mar 18, 2021 •

edited

Loading

vstinner commented Mar 18, 2021

pablogsal commented Mar 18, 2021

pablogsal Mar 18, 2021

vstinner Mar 19, 2021

pablogsal Mar 18, 2021

vstinner Mar 19, 2021

pablogsal Mar 18, 2021

vstinner Mar 19, 2021

pablogsal left a comment

bpo-43244: Remove symtable.h header file #24910

bpo-43244: Remove symtable.h header file #24910

Conversation

vstinner commented Mar 17, 2021 • edited Loading

vstinner commented Mar 17, 2021

pablogsal commented Mar 17, 2021 • edited Loading

vstinner commented Mar 17, 2021

vstinner commented Mar 18, 2021

vstinner commented Mar 18, 2021 • edited Loading

vstinner commented Mar 18, 2021

pablogsal commented Mar 18, 2021

pablogsal Mar 18, 2021

Choose a reason for hiding this comment

vstinner Mar 19, 2021

Choose a reason for hiding this comment

pablogsal Mar 18, 2021

Choose a reason for hiding this comment

vstinner Mar 19, 2021

Choose a reason for hiding this comment

pablogsal Mar 18, 2021

Choose a reason for hiding this comment

vstinner Mar 19, 2021

Choose a reason for hiding this comment

pablogsal left a comment

Choose a reason for hiding this comment

vstinner commented Mar 17, 2021 •

edited

Loading

pablogsal commented Mar 17, 2021 •

edited

Loading

vstinner commented Mar 18, 2021 •

edited

Loading