bpo-40334: Support Python 3.6 in the PEG generator #19786
Conversation
|
Thanks @serhiy-storchaka for helping with this :) Two things first:
|
|
The problem was in the subtle difference in the comma interpretation between (a := b,)and a = b, |
|
Changes:
|
|
The grammar parser is generated from metagrammar. I didn't read the whole diff -- do you also fix the Python generator not to generate walruses? How important is it to be able to bootstrap with Python < 3.8? I rather like the way the generator generates walruses... |
|
Would it be possible to avoid the long indent blocks to substitute the walrus? They will scale with the number of alternatives in a rule and although is generated code, I find that a bit unpleasant as it makes the generated code much harder to read and reason about IMHO :( |
|
I think it is important to not depend on the latest version of Python. This adds obstacles for new contributors. In the past all scripts used for building Python was conservative enough. I think that we should support at least the oldest Python version for which regular bugfixes are accepted (currently it is 3.6). In addition, this can help to catch some bugs (as using token data from a wrong Python version). This PR allows to use 3.6, but it fails on 3.7, -- therefore there are some bugs in the generator. The generated code with walruses is not so easy to read. It is subjective, but it uses some tricks (as in |
Oh, interesting! What is the failure in 3.7? Something to do with the
I still would like to not have one indentation level per alternative in this PR. I am refering to this pattern in the generated code: |
|
Honestly this is horrible. Please withdraw this. New contributors won't need to regenerate the parser unless they're hacking on the grammar, in which case I think requiring 3.8 is not so bad. |
Yes, it is. You can try yourself with this PR.
It is perhaps an example with the largest nesting level. We can get rid from three levels. And after using PEP 8 style for generated code it would look better: if rulename:
opt = self.memoflag()
if self.expect(":")
alts = self.alts()
if alts and self.expect('NEWLINE') and self.expect('INDENT'):
more_alts = self.more_alts()
if more_alts and self.expect('DEDENT'):
return Rule(rulename[0], rulename[1], Rhs(alts.alts + more_alts.alts), memo=opt)The current code is not an example of beauty either: if (
(rulename := self.rulename())
and
(opt := self.memoflag(),)
and
(literal := self.expect(":"))
and
(alts := self.alts())
and
(newline := self.expect('NEWLINE'))
and
(indent := self.expect('INDENT'))
and
(more_alts := self.more_alts())
and
(dedent := self.expect('DEDENT'))
):
return Rule ( rulename [ 0 ] , rulename [ 1 ] , Rhs ( alts . alts + more_alts . alts ) , memo = opt )Further we can merge common code and generate more optimal and compact code for the function (https://github.com/python/cpython/pull/19786/files#diff-97f24c073d6407aacd17575fa57da559L169-L225). 21 lines instead of 56! mark = self.mark()
rulename = self.rulename()
if rulename:
opt = self.memoflag()
if self.expect(":")
mark2 = self.mark()
alts = self.alts()
if alts and self.expect('NEWLINE') and self.expect('INDENT'):
more_alts = self.more_alts()
if more_alts and self.expect('DEDENT'):
return Rule(rulename[0], rulename[1], Rhs(alts.alts + more_alts.alts), memo=opt)
self.reset(mark2)
if self.expect('NEWLINE') and self.expect('INDENT'):
more_alts = self.more_alts()
if more_alts and self.expect('DEDENT'):
return Rule(rulename[0], rulename[1], more_alts, memo=opt)
self.reset(mark2)
alts = self.alts()
if alts and self.expect('NEWLINE'):
return Rule(rulename[0], rulename[1], alts, memo=opt)
self.reset(mark)But we need to use nesting for this. It is not possible to write efficient and readable code using only one level of nesting. New contributors need to regenerate some code when they fix docstring in a function implemented with Argument Clinic. And it is easier to run The purposes of this PR:
If you think it's not worth it, I'll withdraw this PR. |
|
IIUC there was a buildbot using 3.6? Let's not worry about new contributors until we see complaints. |
|
I do not know what relation buildbots using 3.6 have to this issue. Are they failing with the PEG generator? I am not a new contributor, but it touched me when I was not able to build 3.9. |
|
I'm not sure myself, I was speculating about the buildbots (IIRC there was a buildbot that had a failure due to having 3.6 installed, but maybe it was a different issue). Anyway, I still really dislike this solution. Surely you can manage to install 3.8 on your machine. I'll close it now to spare us the pain of extended discussion. |
https://bugs.python.org/issue40334