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

AST safety check fails to catch incorrect f-string change #4268

Closed
kwk opened this issue Mar 8, 2024 · 6 comments · Fixed by #4270
Closed

AST safety check fails to catch incorrect f-string change #4268

kwk opened this issue Mar 8, 2024 · 6 comments · Fixed by #4270
Labels
T: bug Something isn't working T: style What do we want Blackened code to look like?

Comments

@kwk
Copy link

kwk commented Mar 8, 2024

Describe the style change

I wonder if f-strings should be formatted at all.

Examples in the current Black style

print(f"{"|".join(['a','b','c'])}")

Desired style

print(f"{"|".join(['a','b','c'])}")

Note

There's no change to the input here.

Additional context

Black formats the "|" to become " | " and that affects the output which cannot be desired.

Black formats the snippet like this currently:

print(f"{" | ".join(['a','b','c'])}")
@kwk kwk added the T: style What do we want Blackened code to look like? label Mar 8, 2024
@JelleZijlstra JelleZijlstra added the T: bug Something isn't working label Mar 8, 2024
@JelleZijlstra
Copy link
Collaborator

Confirmed this bug:

% python3.12 -m black -c '''print(f"{"|".join(['a','b','c'])}")'''
print(f"{" | ".join([a,b,c])}")

This is related to our lack of nested f-string support (#3746). I suppose our parser misparses this as two strings with an | operator in between, then we reformat it to add spaces around the operator, and the AST safety check doesn't catch it because it ignores leading/trailing whitespace for docstring reasons.

Other than adding actual support for this syntax, which is tracked in #3746, we should catch this in the AST safety check by being less lenient about the whitespace changes we allow in strings.

@JelleZijlstra JelleZijlstra changed the title must leave f-string untouched AST safety check fails to catch incorrect f-string change Mar 8, 2024
@kwk
Copy link
Author

kwk commented Mar 8, 2024

Thank you for confirming this issue. I work around it like so:

# fmt: off
print(f"{"|".join(['a','b','c'])}")
# fmt: on

JelleZijlstra added a commit to JelleZijlstra/black that referenced this issue Mar 9, 2024
Fixes psf#4268

Previously we would allow whitespace changes in all strings, now
only in docstrings.
JelleZijlstra added a commit that referenced this issue Mar 10, 2024
Fixes #4268

Previously we would allow whitespace changes in all strings, now
only in docstrings.

Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>
sumezulike pushed a commit to sumezulike/black that referenced this issue Mar 10, 2024
Fixes psf#4268

Previously we would allow whitespace changes in all strings, now
only in docstrings.

Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>
kwk added a commit to kwk/llvm-snapshots that referenced this issue Mar 11, 2024
@kwk
Copy link
Author

kwk commented Mar 15, 2024

Thank you for addressing this. Can the fix be expected in 24.2.1 or what's the next version that will include the fix?

@JelleZijlstra
Copy link
Collaborator

There will be a new release (24.3.0) soon.

@JelleZijlstra
Copy link
Collaborator

I just released 24.3.0 which includes this fix.

@kwk
Copy link
Author

kwk commented Mar 19, 2024

I just released 24.3.0 which includes this fix.

I still have to wrap the code to not be formatted like this:

# fmt: off
table += f"|{c}|{" | ".join(cols)}|\n"
# fmt: on

Warning

error: cannot format snapshot_manager/snapshot_manager/build_status.py: INTERNAL ERROR: Black produced code that is not equivalent to the source. Please report a bug on https://github.com/psf/black/issues. This diff might be helpful: /tmp/blk_aym4688c.log

$ cat /tmp/blk_aym4688c.log
--- src
+++ dst
@@ -6797,11 +6797,11 @@
                                 value=
                                 Constant(
                                     kind=
                                     None,  # NoneType
                                     value=
-                                    '|',  # str
+                                    ' | ',  # str
                                 )  # /Constant
                             )  # /Attribute
                             keywords=
                         )  # /Call
                     )  # /FormattedValue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: bug Something isn't working T: style What do we want Blackened code to look like?
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants