bpo-42123: Run the parser two times and only enable invalid rules on the second run

[lysnikolaou]

Here's the easiest way I could think of to do this. Are we okay with something
as simple as this or would we like to investigate further whether we could
actually use the memo cache wherever possible?

The first parser run is only responsible for detecting whether
there is a SyntaxError or not. If there isn't, the AST gets returned.
Otherwise, the parser is run a second time with all the invalid_*
rules enabled so that all the customized error messages get produced.

Regarding bpo-41659, we can now implement a very simple fix:

--- a/Grammar/python.gram
+++ b/Grammar/python.gram
@@ -460,6 +460,7 @@ await_primary[expr_ty] (memo):
     | AWAIT a=primary { CHECK_VERSION(5, "Await expressions are", _Py_Await(a, EXTRA)) }
     | primary
 primary[expr_ty]:
+    | primary b='{' { RAISE_SYNTAX_ERROR_KNOWN_LOCATION(b, "invalid syntax") }
     | a=primary '.' b=NAME { _Py_Attribute(a, b->v.Name.id, Load, EXTRA) }
     | a=primary b=genexp { _Py_Call(a, CHECK(_PyPegen_singleton_seq(p, b)), NULL, EXTRA) }
     | a=primary '(' b=[arguments] ')' {

With this patch, I'm now getting:

➜  cpygen git:(parser-second-run) ✗ ./python
Python 3.10.0a0 (heads/parser-second-run-dirty:72fe7254b9, Sep  5 2020, 16:40:28) 
[GCC 9.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> a{
  File "<stdin>", line 1
    a{
     ^
SyntaxError: invalid syntax
>>> a.b.c{
  File "<stdin>", line 1
    a.b.c{
         ^
SyntaxError: invalid syntax

Thoughts?

https://bugs.python.org/issue42123

[lysnikolaou]

Two more things:

1. I actually wanted to open the PR on the we-like-parsers repo, but I messed that up. I don't think there's a problem with it being here, so I'm keeping this open.
2. Could someone actually benchmark this, since the benchmarks on my personal PC are not at all reliable?

[pablogsal]

Are we okay with something
as simple as this or would we like to investigate further whether we could
actually use the memo cache wherever possible?

I think one of the requirements for Guido's approach to work is that the memo cache is preserved between restarts of the parser.

2. Could someone actually benchmark this, since the benchmarks on my personal PC are not at all reliable?

I can benchmark this tomorrow as I am currently repairing my main PC (watercooling leak 😱 ) and is not operational.

[lysnikolaou]

Oooh, that's correct. Ok, let me have another look at this then.

[lysnikolaou]

@pablogsal Do you have the bandwidth for another review on this? If you feel it's okay, I'm going to add an issue and a NEWS blurb.

[pablogsal]

@pablogsal Do you have the bandwidth for another review on this? If you feel it's okay, I'm going to add an issue and a NEWS blurb.

Will check this if I can today or tomorrow

[pablogsal]

After some benchmarking, it results that the parser with this PR is up to 10% (in xxl benchmark) and ~3% faster in parsing the stdlib. Let's merge it :)

[lysnikolaou]

@ambv Do we backport this to 3.9? It's very low-risk and gives a sizable performance boost.

[terryjreedy]

"Do we backport this to 3.9?" We almost never backport performance patches. Low risk is not no risk. With the yearly releases, there is less than a year to wait. With the early and repeated alphas, bleeding edge fans can get this soon enough.

[gvanrossum]

OTOH, for the PEG parser we've been backporting everything relentlessly, otherwise when we fix a bug in 3.10 the backport is a pain in the neck.

[lysnikolaou]

Ok, since there's no consensus, let's not backport this.

[terryjreedy]

I intentionally added 'almost' to my previous comment to suggest that carefully considered exceptions are possible. I forgot about the 3.9 escape option to run the old parser. Guido's comment, when merge conflicts really happen, is real to me from my experience with IDLE. We (I) 'relentlessly' backport everything, but have not always. Manual backports are an extra risk. So is not backporting a fix because it is too much trouble.

I believe PP was once tested on a large number of other python codebases other than the stdlib. Is that done routinely, and in particular for this patch (before and after)? That would make a difference to me.

[lysnikolaou]

I think @pablogsal benchmarked this against something like the 10 most downloaded PyPI packages, although I'm not sure.

[pablogsal]

I think @pablogsal benchmarked this against something like the 10 most downloaded PyPI packages, although I'm not sure.

Yup, I did the benchmark again the 15 most downloaded packages.

[lysnikolaou]

FWIW I think it's worth it to backport, especially now that #23006 was merged into 3.9.

[pablogsal]

FWIW I think it's worth it to backport, especially now that #23006 was merged into 3.9.

I would be supportive of the backport for the reasons previously mentioned

[gvanrossum]

Agreed.