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

Let black run on files even if ast.parse with type_comments=True fails #3593

Closed
tushar-deepsource opened this issue Mar 3, 2023 · 2 comments · Fixed by #3594
Closed

Let black run on files even if ast.parse with type_comments=True fails #3593

tushar-deepsource opened this issue Mar 3, 2023 · 2 comments · Fixed by #3594
Labels
T: enhancement New feature or request

Comments

@tushar-deepsource
Copy link

Is your feature request related to a problem? Please describe.

Right now, if a file parsing because of invalid type comments (even if the code runs just fine), black won't format the file.

For example:

import typing

def foo(
    # type: typing.List
    bar,
):
    print(bar)

foo(10)

While the code still runs:

$ python3 asd.py            
10

Black no longer formats it:

$ black asd.py        
error: cannot format asd.py: cannot use --safe with this file; failed to parse source file AST: invalid syntax (<unknown>, line 4)
This could be caused by running Black with an older Python version that does not support new syntax used in your source file.

Oh no! 💥 💔 💥
1 file failed to reformat.

This is because ast.parse on that code fails if you pass type_comments=True:

>>> import ast
>>> ast.parse(open('asd.py').read(), type_comments=True)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/opt/homebrew/Cellar/[email protected]/3.10.9/Frameworks/Python.framework/Versions/3.10/lib/python3.10/ast.py", line 50, in parse
    return compile(source, filename, mode, flags,
  File "<unknown>", line 4
    # type: typing.List
            ^^^^^^^^^^^
SyntaxError: invalid syntax

(Black does work if you do black asd.py --fast, as it no longer does the AST based check.)

Describe the solution you'd like

If ast.parse(..., type_comments=True) fails for the before/after check, maybe doing it with type_comments=False is fine, just for that file?

Describe alternatives you've considered

None so far.

Additional context

I don't think this can be considered an upstream bug, as this is indeed invalid syntax for a type checker's perspective. But black shouldn't care if the types are correct, in any way shape or form.

@tushar-deepsource tushar-deepsource added the T: enhancement New feature or request label Mar 3, 2023
@tushar-deepsource
Copy link
Author

If the change is approved, I can work on the PR as well.

@JelleZijlstra
Copy link
Collaborator

Makes sense to me. We should try ast.parse(type_comments=False) only as a fallback, so performance won't be impacted for the case where type_comments=True parses correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants