Fix a magical comment caused internal error

[rdrll]

Description

Fix #3737

Analysis of this issue:

Black has no problem parsing multiline consecutive magical comments like type: ignore, it works fine with these examples:

print(   "111") # type:ignore
print(   "111"                         ) # type:ignore
print(   "111"       ) # type:ignore

But Black encountered issue when parsing multiline open-parenthesis consecutive magical comments, code like this will cause Black to produce an INTERNAL ERROR: Black produced code that is not equivalent to the source. problem:

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

Solution

This PR resolves the issue by adding an additional check, before it try to remove outer parentheses.

Additional regression test has been added in tests/test_black.py, and the input & desired output has been placed under tests/data/miscellaneous folder.

With this patch, Black will produce the following output for the example code above:

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

Note that the innermost layer remains formatted, despite the presence of a type: ignore for that line, the reason is because I find when it comes to the innermost layer, it seem to be a different kind of node (the type of Node would be 322 instead of 267 for outer layers).

So in order to change this behaviour, some nuanced modifications on a larger scale seems to be necessary while making sure not breaking any current things, meanwhile, it also makes sense to 'always format the innermost layer' based on examples here and in the original issue ticket. So I'm not entirely clear whether it's a good idea to do so. But for now, it correctly understands this kind of magic comment usage and no longer raises the internal error.

Checklist - did you ...

• Add an entry in CHANGES.md if necessary?
• Add / update tests if necessary?
• Add new / update outdated documentation?

Since it's a bug fixing and not making much enhancement, so I don't think anything is need to be added to documentation.

Please let me know if any changes are needed!

[github-actions]

diff-shades reports zero changes comparing this PR (1c68d4b) to main (c732a1f).

---

What is this? | Workflow run | diff-shades documentation

[JelleZijlstra]

Thank you!

[JelleZijlstra]

Can you use is_type_comment() from lines.py?

[rdrll]

Umm I don't think I can directly replace code here with the is_type_comment() function, as the function expect a leaf as input and check on its value field, while here the comment is as a node's prefix (node.children[1] is still a node), I tried to find some existing function to convert this prefix string into a leaf, but I haven't found one yet... Any further guidance would be really helpful!

[JelleZijlstra]

That's true, the interface doesn't apply. However, there is also a difference in behavior: the other function specifically looks for # type: ignore, but this one also allows # type:ignore. Mypy allows both so I think your solution is better, but we should be consistent (see also #2501 which asks us to format type-ignore comments).

I think we should:

• Add a new function, say is_type_ignore_comment_string, that takes a str and returns whether it's a type ignore comment. It should ignore whitespace.
• Split is_type_comment() into is_type_comment() and is_type_ignore_comment()
• Use is_type_ignore_comment_string() both here and from is_type_ignore_comment().

[JelleZijlstra]

We also discussed this in #3339. I think for now let's not worry about type:ignore without the space and handle only # type: ignore with a space. We can figure out how to deal with the spacing issue in the other PR.

[rdrll]

Gotcha! Thanks for the guidance on design! I'll then keep it consistent with the rest part of Black to only handle the with-space case.

Just to clarify my understanding - when you suggest splitting is_type_comment() into is_type_comment() and is_type_ignore_comment(), does that mean we will be using the original is_type_comment() to handle non-ignore type of comments and use the new is_type_ignore_comment() to handle ignore-type of comments? (so all current usage of is_type_comment() for ignore-type comments will be using the new one instead)

Btw, I noticed that in #3339, it's been mentioned that the type comment is a deprecated feature, I tried to search for information on this deprecation but couldn't quite find where Python states about this deprecation. I'm wondering if you know if there's a particular PEP doc or other places that states it's a deprecated feature? I think it would also be beneficial to have this discussion documented here for future reference.

[JelleZijlstra]

Currently is_type_comment() takes a suffix argument, which is always passed as either "" or " ignore". Calls that use the former should stay as is_type_coment(), and calls that use the latter use is_type_ignore_comment().

What's deprecated-ish is using type comments for type annotations like x: [] # type: List[str]. This was a compatibility feature introduced in PEP 484 for compatibility with Python 2 and Python 3.5, but it's not needed in modern versions of Python. Therefore, we're thinking of deprecating mypy support for it: python/mypy#12947. However, # type: ignore has no such replacement and it's here to stay.

[rdrll]

I have updated the code. I also removed the optional string parameter suffix in is_type_coment(), This parameter was only used to pass in "ignore", and since the handling of ignore comments is now done by the new function is_type_ignore_comment(), there is no need to keep this parameter. Also, considering that is_type_comment() is responsible for handling only general types comments and there are plans to deprecate its support in the future, I added a note to its docstring stating that it might be deprecated in the future.