astral-sh / ruff

An extremely fast Python linter and code formatter, written in Rust.
https://docs.astral.sh/ruff
MIT License
32.59k stars 1.09k forks source link

TCH auto-quoting is invalid for strings containing quotation marks #13934

Open dscorbett opened 2 weeks ago

dscorbett commented 2 weeks ago

The automated fixes for TCH001, TCH002, and TCH003 do not correctly handle string literals containing quotation marks. In the following example, running Ruff introduces an invalid type annotation, which mypy reports.

$ ruff --version
ruff 0.7.1

$ cat tch.py
from collections.abc import Sequence
from typing import Literal
def f(x: Sequence[Literal["'"]]) -> None:
    pass

$ mypy tch.py
Success: no issues found in 1 source file

$ ruff check --isolated --config 'lint.flake8-type-checking.quote-annotations = true' --select TCH tch.py --unsafe-fixes --fix
Found 1 error (1 fixed, 0 remaining).

$ cat tch.py
from typing import Literal, TYPE_CHECKING

if TYPE_CHECKING:
    from collections.abc import Sequence
def f(x: "Sequence[Literal[''']]") -> None:
    pass

$ mypy tch.py
tch.py:5: error: Invalid type comment or annotation  [valid-type]
Found 1 error in 1 file (checked 1 source file)
MichaReiser commented 2 weeks ago

As with other string related fixes, we could consider disabling the fix if the string contains nested quotes. Ideally we would be somewhat clever about it to only disable it when running the fix creates invalid syntax because the use of Literal["Test"] is common

dhruvmanila commented 1 week ago

Ideally we would be somewhat clever about it to only disable it when running the fix creates invalid syntax because the use of Literal["Test"] is common

I'm not sure what do you mean here because we do fix this correctly by flipping the quotes but we don't consider nested quotes inside another existing string literal (https://play.ruff.rs/fc94efc6-2ccf-4b20-b053-f810a7d06c2f). I think we should avoid generating the fix for this case.

cc @Glyphack if you're interested

MichaReiser commented 1 week ago

I'm not sure what do you mean here because we do fix this correctly by flipping the quotes but we don't consider nested quotes inside another existing string literal (play.ruff.rs/fc94efc6-2ccf-4b20-b053-f810a7d06c2f).

That's what I mean. We can't just naively test if the annotation contains any quotes. Instead, we have to search for nested quotes.

Glyphack commented 1 week ago

Thanks I like to spend some time on this. I think the simplest fix would be to check if the StringLiteral contains any quotes inside and don't apply the fix or check if the inner quotes will be a problem and then don't apply the fix.

dscorbett commented 1 week ago

Other characters to beware of are backslashes and newlines.