[flake8-type-checking] Support auto-quoting when annotations contain quotes

[Glyphack]

Summary

This PR updates the fix generation logic for auto-quoting an annotation to generate an edit even when there's a quote character present.

The logic uses the visitor pattern, maintaining it's state on where it is and generating the string value one node at a time. This can be considered as a specialized form of Generator. The state required to maintain is whether we're currently inside a typing.Literal or typing.Annotated because the string value in those types should not be un-quoted i.e., Generic[Literal["int"]] should become "Generic[Literal['int']], the quotes inside the Literal should be preserved.

Fixes: #9137

Test Plan

Add various test cases to validate this change, validate the snapshots. There are no ecosystem changes to go through.

[codspeed-hq]

CodSpeed Performance Report

Merging #11811 will not alter performance

_{Comparing Glyphack:auto-quote-annotation-quotes (2c10896) with main (2f88f84)}

Summary

✅ 32 untouched benchmarks

[MichaReiser]

Putting back in your queue according to Dhruv's comment

[Glyphack]

Thanks, I was away for a while I want to work on this tomorrow.

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[dhruvmanila]

Thanks for working on this. Overall, the approach looks good. I'll give it a detailed review once you think it's ready. I'd also recommend adding test cases with mixed quote style i.e., scenarios where we might not be able to provide a fix.

[dhruvmanila]

Should we only look for Name node here? What about attribute nodes like pd.DataFrame or typing.Optional[int] which is more relevant in this context as it's a subscript expression.

[Glyphack]

Correct. Is it enough if we rely on an attribute with value of "typing" and attr of "Optional" or "Literal", etc.?
Or should we fully resolved and check the qualified name in this case that the symbol is resolved to that class or not?

[Glyphack]

I added another case for attributes.

[dhruvmanila]

I think I know why the test is failing within the current MAX_ITERATIONS value. All of the different test cases are within the same function here so when some of them tries to add the if TYPE_CHECKING block along with the TYPE_CHECKING import, the edits collide and thus it won't be applied which will then require another iteration. We should split the test cases, grouping them based on similarity, in similar way as done in the top half of the file.

[Glyphack]

You are right. I think I can fix that easily. There's just another problem I encountered. Aside from the new tests I've written the old ones also have this problem that each time the type annotation is changed a new import is added and the duplicate imports I think are removed in the end. So now the file fails again.

---- rules::flake8_type_checking::tests::quote::rule_typingonlythirdpartyimport_path_new_quote_py_expects stdout ----
thread 'rules::flake8_type_checking::tests::quote::rule_typingonlythirdpartyimport_path_new_quote_py_expects' panicked at crates/ruff_linter/src/test.rs:167:17:
Failed to converge after 10 iterations. This likely indicates a bug in the implementation of the fix. Last diagnostics:
quote.py:17:24: TCH002 [*] Move third-party import `pandas.DataFrame` into a type-checking block
   |
16 | def f():
17 |     from pandas import DataFrame
   |                        ^^^^^^^^^ TCH002
18 |
19 |     def baz() -> DataFrame:
   |
   = help: Move into type-checking block

ℹ Unsafe fix
3  3  | if TYPE_CHECKING:
4  4  |     from pandas import DataFrame
5  5  |     from pandas import DataFrame
   6  |+    from pandas import DataFrame
6  7  |     import pandas as pd
7  8  |     import pandas as pd
8  9  |     import pandas as pd
--------------------------------------------------------------------------------
14 15 |
15 16 |
16 17 | def f():
17    |-    from pandas import DataFrame
18 18 |
19    |-    def baz() -> DataFrame:
   19 |+    def baz() -> "DataFrame":
20 20 |         ...
21 21 |
22 22 |

note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

It can be reproduced by just duplicating one of the functions multiple times in master:

def f():
    from pandas import DataFrame

    def baz() -> DataFrame[int]:
        ...

def f():
    from pandas import DataFrame

    def baz() -> DataFrame[int]:
        ...

def f():
    from pandas import DataFrame

    def baz() -> DataFrame[int]:
        ...

My guess is that two rules are conflicting here. One is moving the import to type checking block but since now that import is in type checking block another function later in the quote.py is using that import and it is resolved when the symbol is looked up here

I'm not sure if I'm right because the import should be resolved to the import inside the function itself. It's my guess. I will investigate this later after resolving other issues.
For now I'm going to create a new file that will avoid this issue. Moving my new test cases to a file works fine.

[dhruvmanila]

Thanks for writing an extensive test suite. I think the changes are looking pretty great and close to being merged. I would suggest to separate the test cases in groups where the first group raises the diagnostics and the second group doesn't. The first group would need to be split into individual isolated functions like existing test cases. The second group can be in a single function because that won't raise any diagnostics.

Also, apologies for the late review, I was on vacation for the past 2 weeks which is why the review was delayed.

[dhruvmanila]

I've updated the code to use the semantic model to resolve the expression into its qualified name and we use that to match against the typing import. This way we make sure that any import aliases are correctly resolved.

[Glyphack]

I really appreciate your help and the reviews it thought me a lot.

[dhruvmanila]

Previously, the state would only append so it would be [..., AnnotatedFirst, AnnotatedRest] and later it would only pop the AnnotatedRest state. I've updated this to replace AnnotatedFirst with AnnotatedRest instead.

[dhruvmanila]

There was a branch for Expr::BoolOp although I don't think that's required. Can you confirm this? @Glyphack

[Glyphack]

You are correct. I was not very familiar with syntax in type annotation but I don't see anything with a boolean operation.

[dhruvmanila]

I've split the files because otherwise we run into the iteration limit in tests which is set to 10. We'd need to increase it substantially (~20) to fit all the test cases in a single file. The reason the limit needs to be increased seems unrelated to what's being changed in this PR and my guess for why it's happening is because the import edits are colliding.

[dhruvmanila]

Thanks for contributing this change and for your patience throughout the review cycle! I think this is ready to land, I'll go through the snapshots once and move ahead.

[dhruvmanila]

This is a large snapshot diff because one of the test case was removed which had DataFrame["int"] as an annotation. Previously, we would highlight this but won't be auto-fixable but now we do. Once that happens, it hits the iteration limit in tests and so I moved it to a different file which means the line numbers for all subsequent test cases have been updated. I verified that this snapshot update is correct by keeping the test case and updating the limit which gives reduced diff in the snapshot.