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 crashes when moving type: ignore comments #3737

Closed
Zeckie opened this issue Jun 19, 2023 · 2 comments · Fixed by #3740
Closed

Black crashes when moving type: ignore comments #3737

Zeckie opened this issue Jun 19, 2023 · 2 comments · Fixed by #3740
Labels
C: crash Black is crashing F: comments The syntactic kind. Not in the language grammar, always on our minds. Best bugs.

Comments

@Zeckie
Copy link

Zeckie commented Jun 19, 2023

Describe the bug

Black crashes when run on some code that includes a multiline statement with #type: ignore comments on multiple lines.

To Reproduce

For example, take this code:

class Foo:
   def foo(self) -> None:
       if True:
           self.foooooo=# type:ignore[error-code]
               Baaaaaaar# type:ignore[error-code]
                   self.baz()
               )
           ) 

And run it with these arguments:

$ black file.py

The resulting error is:

cannot format black.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

Logfile:

--- src
+++ dst
@@ -108,8 +108,6 @@
        'Foo',  # str
    )  # /ClassDef
  type_ignores=
    TypeIgnore(
    )  # /TypeIgnore
-    TypeIgnore(
-    )  # /TypeIgnore
)  # /Module
\ No newline at end of file

Expected behavior
Run without errors, leaving #type: ignore comments with the code they are annotating.

Environment

  • Black's version: main, 23.3.0
  • OS and Python version: Windows 10, python 3.11.2

Additional context

The #type: ignore comments are for mypy.

@JelleZijlstra JelleZijlstra added F: comments The syntactic kind. Not in the language grammar, always on our minds. Best bugs. C: crash Black is crashing labels Jun 21, 2023
@Zeckie
Copy link
Author

Zeckie commented Jun 25, 2023

It is great to see this issue being addressed so quickly.

Some thoughts I had about this:

  • # type: ignores apply to lines, not statements or expressions. They can also be at the start of a file to turn off error checking for the whole file (PEP 484).
  • when used with mypy, they can include error codes to narrow down which error/s are to be ignored on that line. I don't think the use of # type:ignore with error codes is currently part of any standard
  • I think it should be fine for black to combine lines of a multiline statement, as long as the the ignores are also combined, for example:
      class Foo:
         def foo(self) -> None:
             if True:
                 self.foooooo= (  # type:ignore[error-code-a] # something
                     Baaaaaaar(  # type:ignore[error-code-b]
                         self.baz()
                     )
                 )
    could be converted to:
      class Foo:
         def foo(self) -> None:
             if True:
                 self.foooooo = Baaaaaaar(self.baz()) # type:ignore [error-code-a, error-code-b]  # something
  • Going the other way, splitting a line with a # type:ignore isn't safe, as black will not know how many errors are being ignored, or where on the line those errors are.

@rdrll
Copy link
Contributor

rdrll commented Jun 25, 2023

Thanks for the suggestions, I'd also like to add my two cents on these for consideration:

  • Regarding the use of # type: ignore to disable error checking for the entire file

    It seems Black recognizes type comments at the end of lines rather than at the beginning of a file. To prevent Black from formatting an entire file, one might use the # fmt: off to do it (see here). And for telling mypy to ignore a file, Black supports # mypy: ignore-errors (see here).

  • Regarding merging the # type: ignore's error code across multi-line and enabling Black to understand what error have been ignored

    This could indeed be an interesting topic for discussion, but I believe it would require a broader discussion on this, as this involves changing the way Black treats # type: ignore comments (almost) entirely and also require extensive modification on Black's core logic to parse the syntax tree to support this (currently Black don't functionality to modify or combine things inside the comments). And just like you mentioned, the use of error code is not a part of any standard, so I suspect that if it's worth to support this, considering the amount of efforts needed here. On the other hand, I think it's also not Black's focus to handle these magic comments perfectly (see here).

  • Current practice that Black handles type comment

    It might be helpful to provide an explanation on this for reference purposes.

    Currently Black use # type: ignore to ignore the format on a specific line, and when it comes to multi-line type comment handling, in fact, Black can actually parse # type: ignore in multi-line statement, it's just Black can't handle consecutive # type: ignore in multi-line statement (thanks to your finding).

    See this example (formatted by the current public release of Black):

    a = (  
        int(  # type: ignore
            int(  
                int(  # type: ignore
                    6
                )
            )
        )
    )

    would be converted to

    a = int(  # type: ignore
        int(int(6))  # type: ignore
    )

    without raising any error.

    Since Black already have partially handled this scenario, in order to maintain consistency in Black's results, it might not be the ideal time to redesign it before discussed separately.

  • I am still quite new to Black, so these are just my thoughts on the matter. It would be great if our maintainer on this issue ticket, could double check on these suggestions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: crash Black is crashing F: comments The syntactic kind. Not in the language grammar, always on our minds. Best bugs.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants