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.

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

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

Merged
merged 23 commits into from
Oct 23, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
65a38e9
test: add test case for auto quoting
Glyphack Jun 9, 2024
c8df851
feat: quote annotations that contain quotes
Glyphack Jun 9, 2024
d97e2bd
test: update test for auto quoting
Glyphack Jun 10, 2024
8ca1f5c
refactor: cargo fmt
Glyphack Jun 10, 2024
29e0d4a
refactor: remove unused parameter
Glyphack Jun 10, 2024
1b58828
Update crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs
Glyphack Jun 11, 2024
14f51e3
test: test cases with same and opposite quotes
Glyphack Jun 11, 2024
e8c0f90
Only flip quotes if there is no Literal or Annotation
Glyphack Aug 23, 2024
c6ef3db
Try transformer
Glyphack Aug 24, 2024
75f4407
Remove unnecessary quotes in annotation expressions
Glyphack Aug 25, 2024
3a253f6
Update crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs
Glyphack Sep 11, 2024
bf363e2
Update crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs
Glyphack Sep 11, 2024
f27ae89
Apply comments
Glyphack Sep 11, 2024
2b58cbb
Fix clippy
Glyphack Sep 11, 2024
fbf0bb1
Add test cases for quoting annotations in Literal and Annotated
Glyphack Sep 12, 2024
5079a8c
Revert max iterations count
Glyphack Sep 16, 2024
f7e19b4
TEMP Remove problematic case from tests
Glyphack Oct 14, 2024
26081a6
Remove quote style
Glyphack Oct 14, 2024
5c4b4df
Remove import
Glyphack Oct 14, 2024
d4602a2
Add case for attributes in subscript
Glyphack Oct 14, 2024
114edd0
Merge branch 'main' into auto-quote-annotation-quotes
dhruvmanila Oct 23, 2024
ff973d4
Use semantic model to resolve imports; update tests
dhruvmanila Oct 23, 2024
2c10896
Rename `QuoteAnnotation` -> `QuoteAnnotator`
dhruvmanila Oct 23, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -97,3 +97,10 @@ def f():

def func(self) -> DataFrame | list[Series]:
pass

def f():
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Glyphack Glyphack Oct 14, 2024

Choose a reason for hiding this comment

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

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.

from django.contrib.auth.models import AbstractBaseUser

def foo(self, user: AbstractBaseUser["int"], view: "type[CondorBaseViewSet]"):
pass

Glyphack marked this conversation as resolved.
Show resolved Hide resolved
33 changes: 14 additions & 19 deletions crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ use ruff_python_codegen::{Generator, Stylist};
use ruff_python_semantic::{
analyze, Binding, BindingKind, Modules, NodeId, ResolvedReference, ScopeKind, SemanticModel,
};
use ruff_source_file::Locator;
use ruff_text_size::Ranged;

use crate::rules::flake8_type_checking::settings::Settings;
Expand Down Expand Up @@ -221,7 +220,6 @@ pub(crate) fn is_singledispatch_implementation(
pub(crate) fn quote_annotation(
Glyphack marked this conversation as resolved.
Show resolved Hide resolved
node_id: NodeId,
semantic: &SemanticModel,
locator: &Locator,
stylist: &Stylist,
generator: Generator,
) -> Result<Edit> {
Expand All @@ -233,54 +231,51 @@ pub(crate) fn quote_annotation(
// If we're quoting the value of a subscript, we need to quote the entire
// expression. For example, when quoting `DataFrame` in `DataFrame[int]`, we
// should generate `"DataFrame[int]"`.
return quote_annotation(parent_id, semantic, locator, stylist, generator);
return quote_annotation(parent_id, semantic, stylist, generator);
}
}
Some(Expr::Attribute(parent)) => {
if expr == parent.value.as_ref() {
// If we're quoting the value of an attribute, we need to quote the entire
// expression. For example, when quoting `DataFrame` in `pd.DataFrame`, we
// should generate `"pd.DataFrame"`.
return quote_annotation(parent_id, semantic, locator, stylist, generator);
return quote_annotation(parent_id, semantic, stylist, generator);
}
}
Some(Expr::Call(parent)) => {
if expr == parent.func.as_ref() {
// If we're quoting the function of a call, we need to quote the entire
// expression. For example, when quoting `DataFrame` in `DataFrame()`, we
// should generate `"DataFrame()"`.
return quote_annotation(parent_id, semantic, locator, stylist, generator);
return quote_annotation(parent_id, semantic, stylist, generator);
}
}
Some(Expr::BinOp(parent)) => {
if parent.op.is_bit_or() {
// If we're quoting the left or right side of a binary operation, we need to
// quote the entire expression. For example, when quoting `DataFrame` in
// `DataFrame | Series`, we should generate `"DataFrame | Series"`.
return quote_annotation(parent_id, semantic, locator, stylist, generator);
return quote_annotation(parent_id, semantic, stylist, generator);
}
}
_ => {}
}
}

// If the annotation already contains a quote, avoid attempting to re-quote it. For example:
// ```python
// from typing import Literal
//
// Set[Literal["Foo"]]
// ```
let text = locator.slice(expr);
if text.contains('\'') || text.contains('"') {
return Err(anyhow::anyhow!("Annotation already contains a quote"));
}

// Quote the entire expression.
let quote = stylist.quote();
let annotation = generator.expr(expr);

let annotation_new = if annotation.contains(stylist.quote().as_char()) {
annotation.replace(
stylist.quote().as_char(),
&stylist.quote().opposite().as_char().to_string(),
)
} else {
annotation
};
Glyphack marked this conversation as resolved.
Show resolved Hide resolved

Ok(Edit::range_replacement(
format!("{quote}{annotation}{quote}"),
format!("{quote}{annotation_new}{quote}"),
expr.range(),
))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,6 @@ fn quote_imports(checker: &Checker, node_id: NodeId, imports: &[ImportBinding])
Some(quote_annotation(
reference.expression_id()?,
checker.semantic(),
checker.locator(),
checker.stylist(),
checker.generator(),
))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -505,7 +505,6 @@ fn fix_imports(checker: &Checker, node_id: NodeId, imports: &[ImportBinding]) ->
Some(quote_annotation(
reference.expression_id()?,
checker.semantic(),
checker.locator(),
checker.stylist(),
checker.generator(),
))
Expand Down
Copy link
Member

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ quote.py:9:24: TCH002 [*] Move third-party import `pandas.DataFrame` into a type
13 16 |
14 17 |

quote.py:16:24: TCH002 Move third-party import `pandas.DataFrame` into a type-checking block
quote.py:16:24: TCH002 [*] Move third-party import `pandas.DataFrame` into a type-checking block
|
15 | def f():
16 | from pandas import DataFrame
Expand All @@ -65,6 +65,26 @@ quote.py:16:24: TCH002 Move third-party import `pandas.DataFrame` into a type-ch
|
= help: Move into type-checking block

ℹ Unsafe fix
1 |+from typing import TYPE_CHECKING
2 |+
3 |+if TYPE_CHECKING:
4 |+ from pandas import DataFrame
1 5 | def f():
2 6 | from pandas import DataFrame
3 7 |
--------------------------------------------------------------------------------
13 17 |
14 18 |
15 19 | def f():
16 |- from pandas import DataFrame
17 20 |
18 |- def baz() -> DataFrame["int"]:
21 |+ def baz() -> "DataFrame['int']":
19 22 | ...
20 23 |
21 24 |

quote.py:23:22: TCH002 [*] Move third-party import `pandas` into a type-checking block
|
22 | def f():
Expand Down Expand Up @@ -185,7 +205,7 @@ quote.py:45:24: TCH002 [*] Move third-party import `pandas.DataFrame` into a typ
49 52 |
50 53 |

quote.py:54:24: TCH002 Move third-party import `pandas.DataFrame` into a type-checking block
quote.py:54:24: TCH002 [*] Move third-party import `pandas.DataFrame` into a type-checking block
|
52 | from typing import Literal
53 |
Expand All @@ -196,6 +216,26 @@ quote.py:54:24: TCH002 Move third-party import `pandas.DataFrame` into a type-ch
|
= help: Move into type-checking block

ℹ Unsafe fix
1 |+from typing import TYPE_CHECKING
2 |+
3 |+if TYPE_CHECKING:
4 |+ from pandas import DataFrame
1 5 | def f():
2 6 | from pandas import DataFrame
3 7 |
--------------------------------------------------------------------------------
51 55 | def f():
52 56 | from typing import Literal
53 57 |
54 |- from pandas import DataFrame
55 58 |
56 |- def baz() -> DataFrame[Literal["int"]]:
59 |+ def baz() -> "DataFrame[Literal['int']]":
57 60 | ...
58 61 |
59 62 |

quote.py:71:24: TCH002 [*] Move third-party import `pandas.DataFrame` into a type-checking block
|
70 | def f():
Expand Down Expand Up @@ -369,6 +409,8 @@ quote.py:96:24: TCH002 [*] Move third-party import `pandas.DataFrame` into a typ
98 |- def func(self) -> DataFrame | list[Series]:
101 |+ def func(self) -> "DataFrame | list[Series]":
99 102 | pass
100 103 |
101 104 | def f():

quote.py:96:35: TCH002 [*] Move third-party import `pandas.Series` into a type-checking block
|
Expand Down Expand Up @@ -397,3 +439,34 @@ quote.py:96:35: TCH002 [*] Move third-party import `pandas.Series` into a type-c
98 |- def func(self) -> DataFrame | list[Series]:
101 |+ def func(self) -> "DataFrame | list[Series]":
99 102 | pass
100 103 |
101 104 | def f():

quote.py:102:44: TCH002 [*] Move third-party import `django.contrib.auth.models.AbstractBaseUser` into a type-checking block
|
101 | def f():
102 | from django.contrib.auth.models import AbstractBaseUser
| ^^^^^^^^^^^^^^^^ TCH002
103 |
104 | def foo(self, user: AbstractBaseUser["int"], view: "type[CondorBaseViewSet]"):
|
= help: Move into type-checking block

ℹ Unsafe fix
1 |+from typing import TYPE_CHECKING
2 |+
3 |+if TYPE_CHECKING:
4 |+ from django.contrib.auth.models import AbstractBaseUser
1 5 | def f():
2 6 | from pandas import DataFrame
3 7 |
--------------------------------------------------------------------------------
99 103 | pass
100 104 |
101 105 | def f():
102 |- from django.contrib.auth.models import AbstractBaseUser
103 106 |
104 |- def foo(self, user: AbstractBaseUser["int"], view: "type[CondorBaseViewSet]"):
107 |+ def foo(self, user: "AbstractBaseUser['int']", view: "type[CondorBaseViewSet]"):
105 108 | pass
106 109 |
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ pub fn test_snippet(contents: &str, settings: &LinterSettings) -> Vec<Message> {
}

thread_local! {
static MAX_ITERATIONS: std::cell::Cell<usize> = const { std::cell::Cell::new(10) };
static MAX_ITERATIONS: std::cell::Cell<usize> = const { std::cell::Cell::new(12) };
Glyphack marked this conversation as resolved.
Show resolved Hide resolved
}

pub fn set_max_iterations(max: usize) {
Expand Down
Loading