◐ Shell
reader mode source ↗
Skip to content

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

Merged
vstinner merged 1 commit into
python:masterfrom
vstinner:pycore_symtable
Mar 19, 2021
Merged

bpo-43244: Remove symtable.h header file#24910
vstinner merged 1 commit into
python:masterfrom
vstinner:pycore_symtable

Conversation

@vstinner

@vstinner vstinner commented Mar 17, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

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 vstinner requested review from a team and markshannon as code owners March 17, 2021 23:09
@vstinner

vstinner commented Mar 17, 2021

Copy link
Copy Markdown
Member Author

@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

pablogsal commented Mar 17, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

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 vstinner mentioned this pull request Mar 17, 2021
Merged
@vstinner

vstinner commented Mar 17, 2021

Copy link
Copy Markdown
Member Author

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

@vstinner

vstinner commented Mar 18, 2021

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

vstinner commented Mar 18, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

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.

@vstinner
bpo-43244: Remove symtable.h header file …
c536a50 
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

vstinner commented Mar 18, 2021

Copy link
Copy Markdown
Member Author

@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

pablogsal commented Mar 18, 2021

Copy link
Copy Markdown
Member

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

pablogsal
pablogsal approved these changes Mar 18, 2021
View reviewed changes

@pablogsal pablogsal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

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

@vstinner vstinner merged commit 28ad12f into python:master Mar 19, 2021
@vstinner vstinner deleted the pycore_symtable branch March 19, 2021 11:41
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

@pablogsal pablogsal pablogsal approved these changes

@markshannon markshannon

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vstinner @pablogsal @the-knights-who-say-ni @bedevere-bot