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.

Sign up for GitHub

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

Jump to bottom

Fix a magical comment caused internal error #3740

Merged
merged 6 commits into from Jun 27, 2023
Merged

Fix a magical comment caused internal error #3740

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
20ffd47
bug fixing
rdrll Jun 21, 2023
b85d90e
Update CHANGES.md
rdrll Jun 21, 2023
b8e98eb
Lint & Change log fixing
rdrll Jun 21, 2023
ecb270f
updates
rdrll Jun 22, 2023
5035ce6
functionality change for is_type_comment
rdrll Jun 24, 2023
1c68d4b
Merge branch 'psf:main' into fix_consecutive_ignore_internal_error
rdrll Jun 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGES.md
View file
Open in desktop
Expand Up @@ -12,6 +12,8 @@

- Fix a bug where an illegal trailing comma was added to return type annotations using
PEP 604 unions (#3735)
- Fix a bug where multi-line open parenthesis magic comment like `type: ignore` were not
correctly parsed (#3740)

### Preview style

Expand Down
10 changes: 8 additions & 2 deletions src/black/linegen.py
View file
Open in desktop
Expand Up @@ -1399,8 +1399,14 @@ def maybe_make_parens_invisible_in_atom(
if is_lpar_token(first) and is_rpar_token(last):
middle = node.children[1]
# make parentheses invisible
first.value = ""
last.value = ""
if (
not node.children[1]
rdrll marked this conversation as resolved.
Show resolved Hide resolved
.prefix.strip()
.replace(" ", "")
.startswith("#type:ignore")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use is_type_comment() from lines.py?

Copy link
Contributor Author

@rdrll rdrll Jun 22, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

):
first.value = ""
last.value = ""
maybe_make_parens_invisible_in_atom(
middle,
parent=parent,
Expand Down
29 changes: 29 additions & 0 deletions tests/data/miscellaneous/consecutive_ignore.diff
View file
Open in desktop
@@ -0,0 +1,29 @@
--- [Deterministic header]
+++ [Deterministic header]
rdrll marked this conversation as resolved.
Show resolved Hide resolved
@@ -1,21 +1,15 @@
# This is a regression test. Issue #3737

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

-b = (
- int(
- 6
- )
-)
+b = int(6)

-print( "111") # type:ignore
-print( "111" ) # type:ignore
-print( "111" ) # type:ignore
+print("111") # type:ignore
+print("111") # type:ignore
+print("111") # type:ignore
21 changes: 21 additions & 0 deletions tests/data/miscellaneous/consecutive_ignore.py
View file
Open in desktop
@@ -0,0 +1,21 @@
# This is a regression test. Issue #3737

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

b = (
int(
6
)
)

print( "111") # type:ignore
print( "111" ) # type:ignore
print( "111" ) # type:ignore
21 changes: 21 additions & 0 deletions tests/test_black.py
View file
Open in desktop
Expand Up @@ -280,6 +280,27 @@ def test_pep_695_version_detection(self) -> None:
versions = black.detect_target_versions(root)
self.assertIn(black.TargetVersion.PY312, versions)

def test_consecutive_ignore(self) -> None:
# https://github.com/psf/black/issues/3737

source, _ = read_data("miscellaneous", "consecutive_ignore.py")
expected, _ = read_data("miscellaneous", "consecutive_ignore.diff")
tmp_file = Path(black.dump_to_file(source))
diff_header = re.compile(
rf"{re.escape(str(tmp_file))}\t\d\d\d\d-\d\d-\d\d "
r"\d\d:\d\d:\d\d\.\d\d\d\d\d\d\+\d\d:\d\d"
)
try:
result = BlackRunner().invoke(black.main, ["--diff", str(tmp_file)])
self.assertEqual(result.exit_code, 0)
finally:
os.unlink(tmp_file)

actual = result.output
actual = diff_header.sub(DETERMINISTIC_HEADER, actual)
self.assertEqual(actual, expected)
print(result.output)

def test_expression_ff(self) -> None:
source, expected = read_data("simple_cases", "expression.py")
tmp_file = Path(black.dump_to_file(source))
Expand Down