Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix 3678 "problem parsing with statements" #4538

Merged

Conversation

MeGaGiGaGon
Copy link
Contributor

Description

This fixes #3678. The fix is done by adding two new functions to node.py, is_tuple_containing_star and is_generator, which are then added to the checks in maybe_make_parens_invisible_in_atom inside linegen.py, which is used by the function remove_with_parens that Jelle suggested modifying in a comment on #3678

Both functions are based on the existing is_tuple_containing_walrus. That makes me fairly confident in is_tuple_containing_star, since it does the same thing as is_tuple_containing_walrus, just with syms.namedexpr_test swapped for syms.star_expr. I'm less confident in is_generator since for a in b being called syms.old_comp_for seems weird and I don't totally understand syms/_python_symbols, but it seems to work.

Side note, _python_symbols is really hard to work with since node.type prints as an integer and the _python_symbol variable <--> integer relationship is done via setattr, so I ended up having to do [print(x, eval(f"syms.{x}") for x in dir(syms) if x[0:2] != "__"] to figure out what type generators get.

As usual, both names subject to bikeshedding. I chose is_tuple_containing_star because it compares with syms.star_expr, but is_tuple_containing_unpacking could also work. I chose is_generator since the issue is with a generator, though that could be something else since it doesn't match the syms.old_comp_for comparison.

The tests contain both a top level case and one where the issue is nested, since the nested case caused issues for me while making the fix.

Checklist - did you ...

  • Add an entry in CHANGES.md if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@JelleZijlstra JelleZijlstra merged commit 3b00112 into psf:main Dec 24, 2024
46 checks passed
@MeGaGiGaGon MeGaGiGaGon deleted the fix-3678-with-statement-parsing-problems branch December 24, 2024 20:29
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.

problem parsing with statements
2 participants