quote annotations that already contain quotes

[Glyphack]

Summary

• feat: quote annotations that contain quotes
• test: add test case for auto quoting annotation with a quote

In the new logic if there's an annotation containing same quotes as the current quoting style the fix will replace the current quotes with the opposite of what quoting style is going to be used for quoting the whole annotation.
So this will depend on the stylist.quote() value to decide what character to replace.

If the annotation contain quotes of opposite style, then quoting it will not cause syntax errors.

Another option is escape the current quotes, but this might lead to conflict with other ruff rule Q0004.

Fixes: #9137

Test Plan

[codspeed-hq]

CodSpeed Performance Report

Merging #11811 will not alter performance

_{Comparing Glyphack:auto-quote-annotation-quotes (5079a8c) with main (3b57faf)}

Summary

✅ 32 untouched benchmarks

[dhruvmanila]

I'm sorry I didn't notice this earlier but looking at the documentation of quote_annotation, I don't know if this is the intended solution. I'm referring to the last two points in the examples which mentions the following conversion:

1. Series["pd.Timestamp"] -> "Series[pd.Timestamp]"
2. Series[Literal["pd.Timestamp"]] -> "Series[Literal['pd.Timestamp']]"

In (1), the inner quote isn't required while in (2) it is required. So, all in all, it's a little bit more complicated than just replacing the quote chars 😅

[dhruvmanila]

I discussed this with @AlexWaygood and I think this requires a different solution. Basically, per https://typing.readthedocs.io/en/latest/spec/annotations.html#type-and-annotation-expressions and what Alex suggests in general is that:

Broadly speaking, any sub-element in the typing grammar that is marked as expression cannot have the quotes removed, but all other sub-elements (whether they're annotation expressions or type expressions, etc.), you should be able to remove the quotes from if a broader type/annotation expression that the sub-element is part of is quoted

So,

• We shouldn't removes quotes from all parameters in Literal and all except the first parameter in Annotated
• While, we can remove quotes from other types which is why in (1) the quotes around pd.Timestamp is removed

[Glyphack]

No worries, I also expected my initial implementation to be wrong. Just wanted to open the PR and see the difference in test cases. Let me go over this again but it will be in a few days I'll notify when it's ready.

[dhruvmanila]

Thank you for understanding and take your time.

[Glyphack]

@dhruvmanila I tried to make some adjustment to make it work but I can use your help.

So now we can remove the inner quotes if there's any all the type except for when the inner quote is around values inside a Literal and specific parameters of Annotated.

To find out if the quotes are around those structures we need to take the expression and traverse down the AST and see if the quotes are at these specific places or not.
I'm thinking to have a function that does something similar to https://github.com/Glyphack/ruff/blob/14f51e3d9e96829deab91dd6e24cda5d0ec2fe36/crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs#L229 and checks all children of that annotation expression to make sure the quotes inside are not belonging to those two types and we can safely remove them.

I'm thinking how can I limit the check here since traversing and checking all the node types and visiting the children might be a lot of code. For example do you think if we move this logic somewhere else it could help? Or maybe I'm missing something here.

An alternative way could be only checking if the annotation contains the work Literal or Annotated and then check based on characters in the string to determine if the quotes are there or not. I'm not entirely sure if this would work or not. Sounds very hacky but less code than checking node types of children.

What do you think?

[Glyphack]

I tried your way and I could implement something that solves the initial problem and the Litral and Annotated cases. What do you think about this?
Is there any edge cases that I'm not seeing in this scenario?

I am not checking all ast node types right now but will complete that if this is not missing any critical parts.

[dhruvmanila]

Perfect, I'll look at this today. Thanks for working on this!

[dhruvmanila]

I tried your way and I could implement something that solves the initial problem and the Litral and Annotated cases. What do you think about this?

I think the visitor pattern seems to be working well.

I am not checking all ast node types right now but will complete that if this is not missing any critical parts.

Can you give me an example of what do you mean here?

[Glyphack]

Can you give me an example of what do you mean here?

I thought there are more expression enum values that I should handle here(not send everything to catch all), after checking them now I did not see any of them that require special attention like the subscript and tuple so I think it's fine.

I applied the comments but this is not ready for review yet please wait until I add more test cases.

[dhruvmanila]

I applied the comments but this is not ready for review yet please wait until I add more test cases.

No worries, feel free to ping me if you're stuck or when it's ready to review.

[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]

What's the reason for changing this? Can you specify which test case required this?

[Glyphack]

This test case fails after the new changes:

rules::flake8_type_checking::tests::quote::rule_typingonlythirdpartyimport_path_new_quote_py_expects

I initially did this before trying the visitor approach. I'm not sure if now it can be removed or not. I will look into it to make sure we are not flipping the quotes multiple times. Please let me know if you have a suggestion for figuring out if this is indication of a bug or it's okay to increase the number.

``` Failed to converge after 10 iterations. This likely indicates a bug in the implementation of the fix. Last diagnostics: quote.py:24:24: TCH002 [*] Move third-party import `pandas.DataFrame` into a type-checking block | 23 | def f(): 24 | from pandas import DataFrame | ^^^^^^^^^ TCH002 25 | 26 | def baz() -> DataFrame[int]: | = help: Move into type-checking block

ℹ Unsafe fix
2 2 |
3 3 | if TYPE_CHECKING:
4 4 | from pandas import DataFrame
5 |+ from pandas import DataFrame
5 6 | import pandas as pd
6 7 | import pandas as pd
7 8 | import pandas as pd

21 22 |
22 23 |
23 24 | def f():
24 |- from pandas import DataFrame
25 25 |
26 |- def baz() -> DataFrame[int]:
26 |+ def baz() -> "DataFrame[int]":
27 27 | ...
28 28 |
29 29 |

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
2 2 |
3 3 | if TYPE_CHECKING:
4 4 | from pandas import DataFrame
5 |+ from pandas import DataFrame
5 6 | import pandas as pd
6 7 | import pandas as pd
7 8 | 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 |

</details>

[dhruvmanila]

I just ran it on your branch with the original value and it works. I think it can be reverted back.

[Glyphack]

I changed it back to original config in this commit but CI fails:
5079a8c

https://github.com/astral-sh/ruff/actions/runs/10886671657/job/30207264819?pr=11811#step:8:3383

[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.