Skip to content

Simplify BracketTracker initialization#2

Open
Faraax66 wants to merge 1 commit into
mainfrom
test-pr-2
Open

Simplify BracketTracker initialization#2
Faraax66 wants to merge 1 commit into
mainfrom
test-pr-2

Conversation

@Faraax66

Copy link
Copy Markdown
Owner

Removing the early default value for bracket_depth since it's always set before use in practice. Should reduce redundant assignment.

Removed early initialization in BracketTracker to simplify logic.
@Faraax66

Copy link
Copy Markdown
Owner Author

⚠️ Heads-up: this area has a recent history of crashes caused by bracket_depth not being initialized before use. PR psf#5161 fixed an AttributeError: 'Leaf' object has no attribute 'bracket_depth' crash on valid code, and PR psf#5215 was a follow-up that explicitly ensured Leaf.bracket_depth is always initialized. The assumption that bracket_depth is "always set before use in practice" is what led to those bugs — please make sure to audit all code paths carefully, including edge cases like # fmt: skip after opening brackets, before removing this default initialization.

@msftcangoblowm

Copy link
Copy Markdown

I'm the author of black#5214

I agree with your main point.

You mentioned # fmt: skip, but which tags make up these long lines does not contribute to the crash.

This particular issue is triggered by:

  1. 1st long line (triggers black.linegen.transform_line)
  2. 2nd long line black struggles whether or not to enclose it with visible paranthesis. One inline comment ... crash Two inline comments ... different issue for another day

Can exchange # fmt: skip for say: # isort:skip and the crash would still occur.

Lets keep it simple and just wait for PR black#5215 to be merged.

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.

2 participants