Skip to content

gh-129858: Special syntax error for elif block after else#129902

Merged
pablogsal merged 12 commits into
python:mainfrom
swfarnsworth:elif-error-message
Apr 25, 2025
Merged

gh-129858: Special syntax error for elif block after else#129902
pablogsal merged 12 commits into
python:mainfrom
swfarnsworth:elif-error-message

Conversation

@swfarnsworth

@swfarnsworth swfarnsworth commented Feb 9, 2025

Copy link
Copy Markdown
Contributor

Previously, having an elif block after an else block would raise a standard syntax error. This PR implements a special syntax error saying "elif not allowed after else".

I compiled cpython with this version of the parser and confirmed that it doesn't conflict with other special syntax or indentation errors.

Previously, having an elif block after an else block would raise a standard syntax error.
@swfarnsworth

Copy link
Copy Markdown
Contributor Author

@pablogsal I see that you've been tagged for review. Please see the associated issue for discussion about whether an even more sophisticated error message is possible.

Comment thread Grammar/python.gram Outdated
@picnixz

picnixz commented Feb 9, 2025

Copy link
Copy Markdown
Member

I compiled cpython with this version of the parser and confirmed that it doesn't conflict with other special syntax or indentation errors.

Please add a test. There should be tests in some test_syntax.py file or test_parser.py files (I don't remember where we put them). Just Ctrl+F some error message to find the file.

@swfarnsworth

Copy link
Copy Markdown
Contributor Author

@picnixz No problem--will do.

@swfarnsworth

Copy link
Copy Markdown
Contributor Author

I've implemented a test as requested, though I'll hold off on committing or pushing it until we decide on the exact wording of the error message.

@ghost

ghost commented Feb 11, 2025

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed

@swfarnsworth

Copy link
Copy Markdown
Contributor Author

Those last two commits should appear as being from this GitHub account. Let me see if I can delete them and try again.

@picnixz

picnixz commented Feb 11, 2025

Copy link
Copy Markdown
Member

In general, we avoid force-pushing, but you can do it in this case and amend the commit author and put the correct email address. (Not sure if it could help in this though)

@swfarnsworth

Copy link
Copy Markdown
Contributor Author

The force push seems to have fixed the problem without any issues.

The usernames for my work and personal email differ by one letter, which is occasionally inconvenient.

@picnixz

picnixz commented Feb 12, 2025

Copy link
Copy Markdown
Member
  • For the detection issue, you should just merge main into that branch with the Update branch button!
  • A NEWS entry and a What's New entry could be added in order to indicate that we have an improved message now. I don't have a good wording here though so maybe @hugovk may have some suggestion?

@swfarnsworth

Copy link
Copy Markdown
Contributor Author

We can draw inspiration from here, I suppose?

@swfarnsworth

Copy link
Copy Markdown
Contributor Author

What about this? I'm not sure if the blocks need something more realistic than pass.

elif statements that follow an else block now have a specific error message.

>>> if word.startswith('a'):
...     pass
... else:
...     pass
... elif word.startswith('b'):
...     pass
File "<stdin>", line 5
  elif word.startswith('b'):
  ^^^^
SyntaxError: 'elif' block follows an 'else' block

@picnixz

picnixz commented Feb 12, 2025

Copy link
Copy Markdown
Member

We can draw inspiration from here, I suppose?

Yes that's what I had in mind.

@picnixz

picnixz commented Feb 12, 2025

Copy link
Copy Markdown
Member

Another possibility is:

>>> if who == "me":
...     print("This is me!")
... else:
...     print("This is not me!")
... elif who is None:
...     print("Who is it?")
File "<stdin>", line 5
  elif who is None:
  ^^^^
SyntaxError: 'elif' block follows an 'else' block

@swfarnsworth

Copy link
Copy Markdown
Contributor Author

I like that, but I think I'll do "It's me!" and "It's not me!" in honor of Elphaba,

@picnixz

picnixz commented Feb 12, 2025

Copy link
Copy Markdown
Member

I like that, but I think I'll do "It's me!" and "It's not me!" in honor of Elphaba,

Let's go for this one :)

@swfarnsworth

Copy link
Copy Markdown
Contributor Author

We also get a subtle homage to Britney :)

Comment thread Misc/NEWS.d/next/Core_and_Builtins/2025-02-12-01-36-13.gh-issue-129858.M-f7Gb.rst Outdated

@hugovk hugovk 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.

I've updated the PR from main which will fix the unrelated "computed changes" error.

Comment thread Misc/NEWS.d/next/Core_and_Builtins/2025-02-12-01-36-13.gh-issue-129858.M-f7Gb.rst Outdated
Comment thread Doc/whatsnew/3.14.rst Outdated
swfarnsworth and others added 3 commits February 14, 2025 10:48
…e-129858.M-f7Gb.rst

Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>

@picnixz picnixz 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.

Some markup nits and LGTM

Comment thread Doc/whatsnew/3.14.rst Outdated
^^^^^^^
ValueError: too many values to unpack (expected 3, got 4)

* ``elif`` statements that follow an ``else`` block now have a specific error message.

@picnixz picnixz Feb 15, 2025

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.

* :keyword:`elif` statements that follow an :keyword:`else` block
  now have a specific error message.

@@ -0,0 +1 @@
``elif`` statements that follow an ``else`` block now have a specific error message.

@picnixz picnixz Feb 15, 2025

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.

:keyword:`elif` statements that follow an :keyword:`else` block now have a specific error message.

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.

You can also put the keyword syntax here.

Comment thread Doc/whatsnew/3.14.rst
@swfarnsworth

Copy link
Copy Markdown
Contributor Author

@picnixz done!
Is this going to get squash merged?

@picnixz

picnixz commented Feb 15, 2025

Copy link
Copy Markdown
Member

Is this going to get squash merged?

Yes, that's why contributors don't need to force-push unless they know what they're doing and/or it's a draft PR. Force-pushing rewrites the history and makes reviews harder if we want to compare with a previous commit (and that's the main reason why we don't want force pushes).

I personally do force-push when I have no PR open or if it's a draft or if I need to amend a commit due to git issues (like you had).

@swfarnsworth

Copy link
Copy Markdown
Contributor Author

I see that there are now merge conflicts with parser.c. I figure this means someone else has made changes to the grammar. Should I undo my changes to parser.c, pull the changes to the grammar, and rebuild parser.c?

@picnixz

picnixz commented Feb 28, 2025

Copy link
Copy Markdown
Member

I see that there are now merge conflicts with parser.c. I figure this means someone else has made changes to the grammar.

You can just regenerate the parser files indeed (I think they are automatically generated right?)

@swfarnsworth

Copy link
Copy Markdown
Contributor Author

@picnixz yeah, there's a make command to do it.

@picnixz

picnixz commented Feb 28, 2025

Copy link
Copy Markdown
Member

Just merge the grammar from main, and regenerate the Parser/parser.c file then. When there are conflicts in auto-generated files, just regenerating them is fine.

@python-cla-bot

python-cla-bot Bot commented Apr 18, 2025

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

CLA signed

@hugovk

hugovk commented Apr 18, 2025

Copy link
Copy Markdown
Member

Please can you fix the merge conflict?

Note the 3.14 feature freeze is in just under three weeks: https://peps.python.org/pep-0745/

@swfarnsworth

Copy link
Copy Markdown
Contributor Author

Apologies for the delay @hugovk @picnixz. I was having issues with the dev container I was using to develop this and didn't see the message from picnixz that soon followed my most recent message.

@swfarnsworth swfarnsworth requested review from hugovk and picnixz April 19, 2025 14:51
@hugovk

hugovk commented Apr 19, 2025

Copy link
Copy Markdown
Member

Please check the CI failures.

@swfarnsworth

Copy link
Copy Markdown
Contributor Author

@hugovk @picnixz I think everything is fixed.

@pablogsal pablogsal enabled auto-merge (squash) April 25, 2025 01:25
@pablogsal pablogsal merged commit 99b71ef into python:main Apr 25, 2025
@pablogsal

Copy link
Copy Markdown
Member

LGTM!

Fantastic job @swfarnsworth! Thanks for your contribution :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants