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

Black thinks that a file containing a syntax error is well formatted #2604

Closed
inglesp opened this issue Nov 12, 2021 · 5 comments
Closed

Black thinks that a file containing a syntax error is well formatted #2604

inglesp opened this issue Nov 12, 2021 · 5 comments
Labels
T: bug Something isn't working

Comments

@inglesp
Copy link

inglesp commented Nov 12, 2021

Describe the bug

Black thinks that a file containing a syntax error is well formatted.

To Reproduce

With a file containing just this code:

{
    f"{",
}

Black does not give an error:

$ black bad.py 
All done! ✨ 🍰 ✨
1 file left unchanged.

But there is a syntax error in the file:

$ python file.py 
  File "/home/inglesp/file.py", line 2
    f"{",
        ^
SyntaxError: f-string: expecting '}'

Expected behavior

I would expect Black to complain!

Environment

  • Black's version: 21.10b0
  • OS and Python version: Linux/Python 3.1.10

Additional context

Black does complain as expected with the following similar files:

{
    f"{"
}
{
f"{",
}

It also complains as expected if there is an extra newline at the end of the file, but that doesn't render in GHFM.

@inglesp inglesp added the T: bug Something isn't working label Nov 12, 2021
@isidentical
Copy link
Collaborator

On the cases where black reformatted the source code (in the 2 additional examples), it complains due to the underlying ast.parse() call (when used with --safe, which is implied by default). So if there is nothing to reformat, then it simply doesn't ever try to parse some of the extra stuff (e.g f-strings, which are ignored by the lib2to3) so it never notices this.

Not a black maintainer but I think this is the appropriate case since this is simply not black's job. If you use pre-commit, then you can use this action to ensure everything is parse-able (check-ast).

@inglesp
Copy link
Author

inglesp commented Nov 16, 2021

As an outsider to the project, this didn't make much sense:

So if there is nothing to reformat, then it simply doesn't ever try to parse some of the extra stuff (e.g f-strings, which are ignored by the lib2to3) so it never notices this.

But having dug in, I think I understand better.

Black's algorithm is:

  • Parse input string with lib2to3
  • Produce output string by reformatting according to Black's rules
  • In safe mode, if the input string and output string are different, check that they parse to the same AST

lib2to3 doesn't handle f-strings, and so it is the AST parsing that finds the SyntaxError.

In this case, my example is unchanged after reformatting, and so the input and output strings are the same, so Black doesn't need to check that they parse to the same AST, and Black thinks the file is correctly formatted.

Is that correct?

I don't know whether it's reasonable to expect Black to detect syntax errors -- but I do think it's surprising that it sometimes does and sometimes doesn't.

@felix-hilden
Copy link
Collaborator

felix-hilden commented Nov 16, 2021

That's fair, and it does seem a bit surprising. But we only promise to format correct Python.

@isidentical
Copy link
Collaborator

@JelleZijlstra I guess this can be closed after #2645

@JelleZijlstra
Copy link
Collaborator

Thanks, agree that #2645 addresses this. (For this specific case, we should catch this error in the future if we end up formatting the expression bits of f-strings.)

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
Projects
None yet
Development

No branches or pull requests

4 participants