Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.1k
bpo-43244: Remove symtable.h header file#24910
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
@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 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 thatmaybe someone somewhere might use it, we can continue to export symbols in libpython. But I would prefer to still rename functions (replace The PEP 384https://www.python.org/dev/peps/pep-0384/ explicitly excluded symtable.h from the limited C API (stable ABI)... But it seems like sadly The Python symtable module remains available and is unchanged. |
pablogsal commentedMar 17, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
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). |
See also PR#24911 "Remove PyAST_Validate() function". |
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 commentedMar 18, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I modified the PR to clarify that Py_SymtableString() was not usable with the stable ABI: |
Rename Include/symtable.h to to Include/internal/pycore_symtable.h,don't export symbols anymore (replace PyAPI_FUNC and PyAPI_DATA withextern) and rename functions:* PyST_GetScope() to _PyST_GetScope()* PySymtable_BuildObject() to _PySymtable_Build()* PySymtable_Free() to _PySymtable_Free()Remove PySymtable_Build(), Py_SymtableString() andPy_SymtableStringObject() functions.The Py_SymtableString() function was part the stable ABI by mistakebut it could not be used, since the symtable.h header file wasexcluded from the limited C API.The Python symtable module remains available and is unchanged.
@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 :-) |
I will review this today. Ping me if I have not done it by EOD |
| @@ -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: | |||
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.
Not sure if this should be here, as it was never documented
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.
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 :-)
| 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. |
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 would remove this, we are in the C-API changes section already.
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.
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.
| @@ -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) | |||
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 the rename?
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.
It's to prevent people to rely on it. It's to advertize: hey, it's a private function, don't use it!
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.
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) ...
Uh oh!
There was an error while loading.Please reload this page.
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:
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