Repository navigation
make built-in tokenizer available via Python C API #47603
Description
| BPO | 3353 |
|---|---|
| Nosy | @amauryfa, @meadori, @berkerpeksag, @serhiy-storchaka, @asottile, @DimitrisJim, @pablogsal |
| Dependencies | |
| Files |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = None
created_at = <Date 2008-07-14.11:32:15.414>
labels = ['interpreter-core', 'type-feature', '3.7']
title = 'make built-in tokenizer available via Python C API'
updated_at = <Date 2021-01-27.21:14:20.006>
user = 'https://bugs.python.org/effbot'bugs.python.org fields:
activity = <Date 2021-01-27.21:14:20.006>
actor = 'pablogsal'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Interpreter Core']
creation = <Date 2008-07-14.11:32:15.414>
creator = 'effbot'
dependencies = ['25643']
files = ['10961', '35730', '38992', '38999']
hgrepos = ['260']
issue_num = 3353
keywords = ['patch']
message_count = 34.0
messages = ['69650', '70101', '70102', '70181', '70227', '70305', '143717', '221293', '240882', '240927', '240967', '245939', '289535', '289537', '289584', '289585', '289587', '289590', '289591', '385736', '385756', '385788', '385790', '385791', '385792', '385793', '385794', '385795', '385796', '385797', '385798', '385799', '385808', '385811']
nosy_count = 12.0
nosy_names = ['effbot', 'amaury.forgeotdarc', 'djmitche', 'kirkshorts', 'meador.inge', 'berker.peksag', 'serhiy.storchaka', 'superluser', 'Andrew.C', 'Anthony Sottile', 'Jim Fasarakis-Hilliard', 'pablogsal']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue3353'
versions = ['Python 3.7']
Activity
effbot commentedon Jul 14, 2008
CPython provides a Python-level API to the parser, but not to the
tokenizer itself. Somewhat annoyingly, it does provide a nice C API,
but that's not properly exposed for external modules.
To fix this, the tokenizer.h file should be moved from the Parser
directory to the Include directory, and the (semi-public) functions that
already available must be flagged with PyAPI_FUNC, as shown below.
The PyAPI_FUNC fix should be non-intrusive enough to go into 2.6 and
3.0; moving stuff around is perhaps better left for a later release
(which could also include a Python binding).
Index: tokenizer.h
===================================================================
amauryfa commentedon Jul 21, 2008
IMO the "struct tok_state" should not be part of the API, it contains
too many implementation details. Or maybe as an opaque structure.
effbot commentedon Jul 21, 2008
There are a few things in the struct that needs to be public, but that's
nothing that cannot be handled by documentation. No need to complicate
the API just in case.
kirkshorts commentedon Jul 23, 2008
Sorry for the terribly dumb question about this.
Are you meaning that, at this stage, all that is required is:
Just I have read this now 10 times or so and keep thinking more must be
involved :-) [certainly given my embarrassing start to the Python dev
community re:asynchronous thread exceptions :-| ]
I have attached a patch that does this. Though at this time it is
lacking any documentation that will state what parts of "struct
tok_state" are private and public. I will need to trawl the code some
more to do that.
I have executed:
And all proceed well.
effbot commentedon Jul 24, 2008
That's should be all that's needed to expose the existing API, as is.
If you want to verify the build, you can grab the pytoken.c and setup.py
files from this directory, and try building the module.
http://svn.effbot.org/public/stuff/sandbox/pytoken/
Make sure you remove the local copy of "tokenizer.h" that's present in
that directory before you build. If that module builds, all's well.
kirkshorts commentedon Jul 26, 2008
Did that and it builds fine.
So my test procedure was:
Build platform: Ubuntu , gcc 4.2.3
All works fine.
thanks for the extra test files.
diff makes it an empty file not removed why is that? (and is it correct?)
meadori commentedon Sep 8, 2011
It would be nice if this same C API was used to implement the 'tokenize' module. Issues like bpo-2180 will potentially require bug fixes in two places :-/
AndrewC commentedon Jun 22, 2014
The previously posted patch has become outdated due to signature changes staring with revision 89f4293 on Nov 12, 2009. Attached is an updated patch.
Can it also be confirmed what are the outstanding items for this patch to be applied? Based on the previous logs it's not clear if it's waiting for documentation on the struct tok_state or if there is another change requested. Thanks.
djmitche commentedon Apr 14, 2015
From my read of this bug, there are two distinct tasks mentioned:
#1 is largely complete in Andrew's latest patch, but that will likely need:
#2 is, I think, a separate project. There may be good reasons *not* to do this which I'm not aware of, and barring such reasons the rewrite will be difficult and could potentially change behavior like bpo-2180. So I would suggest filing a new issue for #2 when #1 is complete. And I'll work on #1.
djmitche commentedon Apr 14, 2015
Here's an updated patch for #1:
Existing Patch:
New:
I'd like particular attention to the documentation for the tokenizer -- I'm not entirely confident that I have documented the functions correctly! In particular, I'm not sure how PyTokenizer_FromString handles encodings.
There's a further iteration possible here, but it's beyond my understanding of the tokenizer and of possible uses of the API. That would be to expose some of the tokenizer state fields and document them, either as part of the limited ABI or even the stable API. In particular, there are about a half-dozen struct fields used by the parser, and those would be good candidates for addition to the public API.
If that's desirable, I'd prefer to merge a revision of my patch first, and keep the issue open for subsequent improvement.
djmitche commentedon Apr 14, 2015
New:
djmitche commentedon Jun 29, 2015
This seems to have stalled out after the PyCon sprints. Any chance the final patch can be reviewed?
DimitrisJim commentedon Mar 13, 2017
Could you submit a PR for this?
I haven't seen any objections to this change, a PR will expose this to more people and a clear decision on whether this change is warranted can be finally made (I hope).
13 remaining items
pablogsal commentedon Jan 27, 2021
For reimplementing Lib/tokenize.py we don't need to publicly expose anything in the C-API. We can have a private _tokenize module with uses whatever you need and then you use that _tokenize module in the tokenize.py file to reimplement the exact Python API that the module exposes.
Publicly exposing the headers or APIs opens new boxes of potential problems: ABI stability, changes in the signatures, changes in the structs. Our experience so far with other parts is that almost always is painful to add optimization to internal functions that are partially exposed, so I am still not convinced offering public C-APIs for the builtin tokenizer.
asottile commentedon Jan 27, 2021
private api sounds fine too -- I thought it was necessary to implement the module (as it needs external linkage) but if it isn't then even better
pablogsal commentedon Jan 27, 2021
We can make it builtin the same way we do for the _ast module, or we can have a new module under Modules (exposing the symbols in the dynamic table) **but** making them private (and not documented), which explicitly goes against what this issue proposes.
asottile commentedon Jan 27, 2021
Either works for me, would you be able to point me to the starting bits as to how
_astbecomes builtin?pablogsal commentedon Jan 27, 2021
https://github.com/python/cpython/blob/master/Python/Python-ast.c#L10075-L10079
and
cpython/PC/config.c
Line 84 in 6329893
But before that I have some questions. For example: How do you plan to implement the readline() interface that tokenize.py uses in the c-module without modifying tokenize.c?
asottile commentedon Jan 27, 2021
I haven't looked into or thought about that yet, it might not be possible
It might also make sense to build new tokenize.py apis avoiding the
readline()api -- I always found it painful to work withpablogsal commentedon Jan 27, 2021
Then we would need to maintain the old Python APIs + the new ones using the module? What you are proposing seems more than just speeding up tokenize.py re-using the existing c code
pablogsal commentedon Jan 27, 2021
I have built a draft of how the changes required to make what you describe, in case you want to finish them:
https://github.com/pablogsal/cpython/tree/tokenizer_mod
pablogsal commentedon Jan 27, 2021
Problems that you are going to find:
❯ python -c "1_"
File "<string>", line 1
1_
^
SyntaxError: invalid decimal literal
❯ python -m tokenize <<< "1_"
1,0-1,1: NUMBER '1'
1,1-1,2: NAME '_'
1,2-1,3: NEWLINE '\n'
2,0-2,0: ENDMARKER ''
The encoding cannot be immediately specified. You need to thread it in many places.
The readline() function can now return whatever or be whatever, that needs to be handled (better) in the c tokenizer to not crash.
str/bytes in the c tokenizer.
The c tokenizer does not get the full line in some cases or is tricky to get the full line.
lysnikolaou commentedon Jun 19, 2023
Since 3.12 the Python
tokenizemodule uses the C tokenizer internally. Is this enough to close this issue?pablogsal commentedon Jun 19, 2023
Yup