Skip to content

[flake8-type-checking] Skip quoting annotation if it becomes invalid syntax (TCH001) #14285

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

Merged
merged 4 commits into from
Nov 15, 2024

Conversation

Glyphack
Copy link
Contributor

@Glyphack Glyphack commented Nov 11, 2024

Fix: #13934

Summary

Current implementation has a bug when the current annotation contains a string with single and double quotes.

TL;DR: I think these cases happen less than other use cases of Literal. So instead of fixing them we skip the fix in those cases.

One of the problematic cases:

from typing import Literal
from third_party import Type

def error(self, type1: Type[Literal["'"]]):
    pass

The outcome is:

- def error(self, type1: Type[Literal["'"]]):
+ def error(self, type1: "Type[Literal[''']]"):

While it should be:

"Type[Literal['\'']"

The solution in this case is that we check if there’s any quotes same as the quote style we want to use for this Literal parameter then escape that same quote used in the string.

Also this case is not uncommon to have: https://grep.app/search?current=2&q=Literal["'

But this can get more complicated for example in case of:

- def error(self, type1: Type[Literal["\'"]]):
+ def error(self, type1: "Type[Literal[''']]"):

Here we escaped the inner quote but in the generated annotation it gets removed. Then we flip the quote style of the Literal paramter and the formatting is wrong.

In this case the solution is more complicated.

  1. When generating the string of the source code preserve the backslash.
  2. After we have the annotation check if there isn’t any escaped quote of the same type we want to use for the Literal parameter. In this case check if we have any without \ before them. This can get more complicated since there can be multiple backslashes so checking for only \’ won’t be enough.

Another problem is when the string contains \n. In case of Type[Literal["\n"]] we generate 'Type[Literal["\n"]]' and both pyright and mypy reject this annotation.
https://pyright-play.net/?code=GYJw9gtgBALgngBwJYDsDmUkQWEMoAySMApiAIYA2AUAMaXkDOjUAKoiQNqsC6AXFAB0w6tQAmJYLBKMYAfQCOAVzCk5tMChjlUjOQCNytANaMGjABYAKRiUrAANLA4BGAQHJ2CLkVIVKnABEADoogTw87gCUfNRQ8VAITIyiElKksooqahpaOih6hiZmTNa29k7w3m5sHJy%2BZFRBoeE8MXEJScxAA

Test Plan

I added test cases for the original code in the reported issue and two more cases for backslash and new line.

@Glyphack Glyphack marked this pull request as draft November 11, 2024 18:49
Copy link
Contributor

github-actions bot commented Nov 11, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@Glyphack
Copy link
Contributor Author

Glyphack commented Nov 12, 2024

Hi @dhruvmanila I spent some time on finding the issue and the fix and wrote it down below. I'm not sure if it's okay to skip fixing these tricky cases
but would be happy to find a better one with your guidance.
Let me know what you think.

@dhruvmanila
Copy link
Member

Hey, thanks for looking into this and opening this PR.

I think the solution for now would be to avoid providing a fix if the annotation contains the preferred quote (the one used in to surround the annotation in the fix) or if it contains any escape characters like a newline character. Although Pyright supports them, the initial implementation for string annotations in red knot will reject them (#14151). There's also this discussion about the different string kinds here.

I think that's what this PR is doing right now based on looking at the snapshot changes, right?

@Glyphack
Copy link
Contributor Author

Yes @dhruvmanila this will skip the fix for those scenarios. sorry the point was not clear in the description.

@Glyphack Glyphack force-pushed the linter-tch-autofix-quotations branch from 9b49d98 to c3428cf Compare November 14, 2024 17:08
@Glyphack Glyphack marked this pull request as ready for review November 14, 2024 17:16
Copy link
Member

@dhruvmanila dhruvmanila left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this.

@dhruvmanila dhruvmanila added the bug Something isn't working label Nov 15, 2024
@dhruvmanila dhruvmanila changed the title Skip auto quoting when result is invalid [flake8-type-checking] Skip quoting annotation if it becomes invalid syntax (TCH001) Nov 15, 2024
@dhruvmanila
Copy link
Member

(I updated the PR to get it into the next release.)

@dhruvmanila dhruvmanila enabled auto-merge (squash) November 15, 2024 11:09
@dhruvmanila dhruvmanila merged commit 6591775 into astral-sh:main Nov 15, 2024
19 checks passed
@Glyphack Glyphack deleted the linter-tch-autofix-quotations branch November 15, 2024 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TCH auto-quoting is invalid for strings containing quotation marks
2 participants