bpo-45759: Better error messages for non-matching 'elif'/'else' statements

[thatbirdguythatuknownot]

#89920

https://bugs.python.org/issue45759

[pablogsal]

Please, next time don't close the old PR as the history of comments and reviews is now lost, which makes more difficult to check things.

[pablogsal]

This is more acceptable, but it will trigger with any compound statement:

with A():
    elif 3:
        ...
SyntaxError: 'elif' must match an if-statement here

Also, a slight minor problem is that at this level we are catching an incorrect keyword in a specific bad context, but there could be lot more. For instance, if you would write:

[x for x in range(3) if 34 elif 12]

[thatbirdguythatuknownot]

Is there anything I should do about those?

[pablogsal]

No for now, let's discuss first if we are ok with this. We may need to adapt the error message

[pablogsal]

No for now, let's discuss first if we are ok with this.

I mean, let's wait to see what @lysnikolaou and @isidentical think

[pablogsal]

Maybe also @cfbolz if he has some time

[thatbirdguythatuknownot]

@pablogsal should we keep waiting for the other reviewers, or should this pull request be closed?

[pablogsal]

@pablogsal should we keep waiting for the other reviewers, or should this pull request be closed?

We should wait for other reviewers.

[isidentical]

I am currently unable to comment on this issue, so feel free to skip me.

[thatbirdguythatuknownot]

@pablogsal It's been 7 days and I plan on closing this pull request because of inactivity. Should we keep waiting for @lysnikolaou?

[pablogsal]

I am still not super convinced, but this version of the patch is more concise so it I am more favourable to consider it, although it matches some potentially incorrect cases (although we could fix that with a better error). What do you think @lysnikolaou @isidentical ?

[thatbirdguythatuknownot]

Fixed a typo I found in Lib/test/test_syntax.py along with the new test cases.

[cfbolz]

(I'm commenting as an error-interested bystander)

I like this approach! It fits also with what @pablogsal and I discussed on twitter earlier: https://twitter.com/cfbolz/status/1458442881346248708

I wonder whether it would make sense to be quite conservative in adding such rules by making them relatively specific. Eg I think as the patch stands it would also trigger the new error message in code like this:

else = f.read()

And it can be argued that it's more likely that the name was misspelled here. So maybe we can make it more specific by changing the rules to:

a='elif' named_expression ':'
and
a='else' ':'

Or, more strongly, maybe even requiring a block after the ':'s. That way we can later make these rules more general by removing some requirements but don't end up in the situation where a very general rule gives somewhat nonsensical messages in a situation that we didn't think about.

Relatedly, I think it makes sense to add some more tests that really explore the boundaries of the new behavior so we get an idea when they apply.

Does that make any sense, @pablogsal et al?

[pablogsal]

Thanks @cfbolz for the message!

In general it makes sense but I would recommend to extend the rule until just the :. The reason is that if you parse more, it can trigger other stuff in the block or move the furthest parsed position. In the C parser we never use the furthest parser position of the second phase, but this is still important to have in mind as could have other effects.

My main worry is precisely the intersection with other errors and the possible false positives and this is what we should carefully examinate to not shoot ourselves in the foot later :)

[cfbolz]

Yes, I think to mitigate that we should be quite diligent in collecting varied test cases (this goes a bit beyond the scope of the pull request though)

[thatbirdguythatuknownot]

In general it makes sense but I would recommend to extend the rule until just the :. The reason is that if you parse more, it can trigger other stuff in the block or move the furthest parsed position. In the C parser we never use the furthest parser position of the second phase, but this is still important to have in mind as could have other effects.

@pablogsal Should I extend the rule now?
Edit: I am gonna extend the rule while there's no reply yet.

[thatbirdguythatuknownot]

Because of inactivity, I'll have to close this commit.

[arhadthedev]

@thatbirdguythatuknownot, I think closing is premature. Here is an activity graph on pablogsal's profile:

The last two weeks he is busy with bug fixes, so this PR just got postponed for a while. Actually, you can even come to his page and click through these squares of the graph to see per day activity, GitHub logs everything.

[thatbirdguythatuknownot]

@arhadthedev So should I reopen it?

[arhadthedev]

@thatbirdguythatuknownot Yeah.

[thatbirdguythatuknownot]

Parser/parser.c is conflicting though, I don't know how to fix it.

[kumaraditya303]

Since there is a new pull request and this one has conflicts, can this be closed ?

[thatbirdguythatuknownot]

Since there is a new pull request and this one has conflicts, can this be closed ?

It was advised to keep pull requests opened to not lose history of reviews and comments. It can be closed once a decision is made.

[MaxwellDupre]

I get a compile error:
make
gcc -c -Wsign-compare -g -Og -Wall -std=c99 -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Werror=implicit-function-declaration -fvisibility=hidden -I./Include/internal -I. -I./Include -DPy_BUILD_CORE -o Parser/parser.o Parser/parser.c
Parser/parser.c: In function ‘invalid_expression_rule’:
Parser/parser.c:18323:94: error: ‘Token’ has no member named ‘level’
18323 | _res = _PyPegen_check_legacy_stmt ( p , a ) ? NULL : p -> tokens [p -> mark - 1] -> level == 0 ? NULL : RAISE_SYNTAX_ERROR_KNOWN_RANGE ( a , b , "invalid syntax. Perhaps you forgot a comma?" );
| ^~
make: *** [Makefile:1960: Parser/parser.o] Error 1

I notice that this has two conditional expressions: I suggest this is split into two for better readability.

[thatbirdguythatuknownot]

I get a compile error: make gcc -c -Wsign-compare -g -Og -Wall -std=c99 -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Werror=implicit-function-declaration -fvisibility=hidden -I./Include/internal -I. -I./Include -DPy_BUILD_CORE -o Parser/parser.o Parser/parser.c Parser/parser.c: In function ‘invalid_expression_rule’: Parser/parser.c:18323:94: error: ‘Token’ has no member named ‘level’ 18323 | _res = _PyPegen_check_legacy_stmt ( p , a ) ? NULL : p -> tokens [p -> mark - 1] -> level == 0 ? NULL : RAISE_SYNTAX_ERROR_KNOWN_RANGE ( a , b , "invalid syntax. Perhaps you forgot a comma?" ); | ^~ make: *** [Makefile:1960: Parser/parser.o] Error 1

I notice that this has two conditional expressions: I suggest this is split into two for better readability.

@MaxwellDupre that isn't part of the PR.

[lysnikolaou]

@MaxwellDupre Thanks for reviving this!

@thatbirdguythatuknownot Would you like to continue working on this PR? First step would be to rebase onto current main and fix any conflicts. If you feel you don't have time for this, I can always do the rebase for you as well. Whatever you want.

I'm generally in favor of this, so we can continue the discussion after a rebase has been done and we can play around with it some more.

[thatbirdguythatuknownot]

@thatbirdguythatuknownot Would you like to continue working on this PR? First step would be to rebase onto current main and fix any conflicts. If you feel you don't have time for this, I can always do the rebase for you as well. Whatever you want.

Sorry, I don't know how to rebase (following the guides I found on the internet; I don't know how to resolve the conflicts). I just started using git a month ago and I haven't explored much of it yet.

[lysnikolaou]

I've pushed a couple of commits to a state where it's mergeable to main. Let's continue the discussion on whether it's good to go as-is.

[lysnikolaou]

Let's go ahead and merge this!