Skip to content

Commit

Permalink
Avoid using typing-imported symbols for runtime edits
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh committed May 25, 2023
1 parent 28a5e60 commit c271db8
Show file tree
Hide file tree
Showing 14 changed files with 219 additions and 60 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
from typing import TYPE_CHECKING

if TYPE_CHECKING:
from sys import exit as bar


def main():
exit(0)
10 changes: 10 additions & 0 deletions crates/ruff/resources/test/fixtures/pyupgrade/UP006_1.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
from __future__ import annotations

import typing

if typing.TYPE_CHECKING:
from collections import defaultdict


def f(x: typing.DefaultDict[str, str]) -> None:
...
8 changes: 8 additions & 0 deletions crates/ruff/resources/test/fixtures/pyupgrade/UP006_2.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import typing

if typing.TYPE_CHECKING:
from collections import defaultdict


def f(x: typing.DefaultDict[str, str]) -> None:
...
8 changes: 8 additions & 0 deletions crates/ruff/resources/test/fixtures/pyupgrade/UP006_3.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import typing

if typing.TYPE_CHECKING:
from collections import defaultdict


def f(x: "typing.DefaultDict[str, str]") -> None:
...
51 changes: 35 additions & 16 deletions crates/ruff/src/importer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,16 @@ impl<'a> Importer<'a> {
at: TextSize,
semantic_model: &SemanticModel,
) -> Result<(Edit, String)> {
self.get_symbol(module, member, at, semantic_model)?
.map_or_else(
|| self.import_symbol(module, member, at, semantic_model),
Ok,
)
match self.get_symbol(module, member, at, semantic_model) {
None => self.import_symbol(module, member, at, semantic_model),
Some(Resolution::Success(edit, binding)) => Ok((edit, binding)),
Some(Resolution::LateBinding) => {
bail!("Unable to use existing symbol due to late binding")
}
Some(Resolution::IncompatibleContext) => {
bail!("Unable to use existing symbol due to incompatible context")
}
}
}

/// Return an [`Edit`] to reference an existing symbol, if it's present in the given [`SemanticModel`].
Expand All @@ -84,21 +89,25 @@ impl<'a> Importer<'a> {
member: &str,
at: TextSize,
semantic_model: &SemanticModel,
) -> Result<Option<(Edit, String)>> {
) -> Option<Resolution> {
// If the symbol is already available in the current scope, use it.
let Some((source, binding)) = semantic_model.resolve_qualified_import_name(module, member) else {
return Ok(None);
};
let imported_name = semantic_model.resolve_qualified_import_name(module, member)?;

// The exception: the symbol source (i.e., the import statement) comes after the current
// location. For example, we could be generating an edit within a function, and the import
// If the symbol source (i.e., the import statement) comes after the current location,
// abort. For example, we could be generating an edit within a function, and the import
// could be defined in the module scope, but after the function definition. In this case,
// it's unclear whether we can use the symbol (the function could be called between the
// import and the current location, and thus the symbol would not be available). It's also
// unclear whether should add an import statement at the top of the file, since it could
// be shadowed between the import and the current location.
if source.start() > at {
bail!("Unable to use existing symbol `{binding}` due to late-import");
if imported_name.range().start() > at {
return Some(Resolution::LateBinding);
}

// If the symbol source (i.e., the import statement) is in a typing-only context, but we're
// in a runtime context, abort.
if imported_name.context().is_typing() && semantic_model.execution_context().is_runtime() {
return Some(Resolution::IncompatibleContext);
}

// We also add a no-op edit to force conflicts with any other fixes that might try to
Expand All @@ -118,10 +127,10 @@ impl<'a> Importer<'a> {
// By adding this no-op edit, we force the `unused-imports` fix to conflict with the
// `sys-exit-alias` fix, and thus will avoid applying both fixes in the same pass.
let import_edit = Edit::range_replacement(
self.locator.slice(source.range()).to_string(),
source.range(),
self.locator.slice(imported_name.range()).to_string(),
imported_name.range(),
);
Ok(Some((import_edit, binding)))
Some(Resolution::Success(import_edit, imported_name.into_name()))
}

/// Generate an [`Edit`] to reference the given symbol. Returns the [`Edit`] necessary to make
Expand Down Expand Up @@ -220,3 +229,13 @@ impl<'a> Importer<'a> {
Ok(Edit::range_replacement(state.to_string(), stmt.range()))
}
}

enum Resolution {
/// The symbol is available for use.
Success(Edit, String),
/// The symbol is imported, but the import came after the current location.
LateBinding,
/// The symbol is imported, but in an incompatible context (e.g., in typing-only context, while
/// we're in a runtime context).
IncompatibleContext,
}
1 change: 1 addition & 0 deletions crates/ruff/src/rules/pylint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ mod tests {
#[test_case(Rule::SysExitAlias, Path::new("sys_exit_alias_7.py"); "PLR1722_7")]
#[test_case(Rule::SysExitAlias, Path::new("sys_exit_alias_8.py"); "PLR1722_8")]
#[test_case(Rule::SysExitAlias, Path::new("sys_exit_alias_9.py"); "PLR1722_9")]
#[test_case(Rule::SysExitAlias, Path::new("sys_exit_alias_10.py"); "PLR1722_10")]
#[test_case(Rule::ContinueInFinally, Path::new("continue_in_finally.py"); "PLE0116")]
#[test_case(Rule::GlobalStatement, Path::new("global_statement.py"); "PLW0603")]
#[test_case(Rule::GlobalVariableNotAssigned, Path::new("global_variable_not_assigned.py"); "PLW0602")]
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
source: crates/ruff/src/rules/pylint/mod.rs
---
sys_exit_alias_10.py:8:5: PLR1722 Use `sys.exit()` instead of `exit`
|
8 | def main():
9 | exit(0)
| ^^^^ PLR1722
|
= help: Replace `exit` with `sys.exit()`


5 changes: 4 additions & 1 deletion crates/ruff/src/rules/pyupgrade/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@ mod tests {
#[test_case(Rule::TypeOfPrimitive, Path::new("UP003.py"); "UP003")]
#[test_case(Rule::UselessObjectInheritance, Path::new("UP004.py"); "UP004")]
#[test_case(Rule::DeprecatedUnittestAlias, Path::new("UP005.py"); "UP005")]
#[test_case(Rule::NonPEP585Annotation, Path::new("UP006.py"); "UP006")]
#[test_case(Rule::NonPEP585Annotation, Path::new("UP006_0.py"); "UP006_0")]
#[test_case(Rule::NonPEP585Annotation, Path::new("UP006_1.py"); "UP006_1")]
#[test_case(Rule::NonPEP585Annotation, Path::new("UP006_2.py"); "UP006_2")]
#[test_case(Rule::NonPEP585Annotation, Path::new("UP006_3.py"); "UP006_3")]
#[test_case(Rule::NonPEP604Annotation, Path::new("UP007.py"); "UP007")]
#[test_case(Rule::SuperCallWithParameters, Path::new("UP008.py"); "UP008")]
#[test_case(Rule::UTF8EncodingDeclaration, Path::new("UP009_0.py"); "UP009_0")]
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
---
source: crates/ruff/src/rules/pyupgrade/mod.rs
---
UP006.py:4:10: UP006 [*] Use `list` instead of `typing.List` for type annotation
UP006_0.py:4:10: UP006 [*] Use `list` instead of `typing.List` for type annotation
|
4 | def f(x: typing.List[str]) -> None:
| ^^^^^^^^^^^ UP006
Expand All @@ -19,7 +19,7 @@ UP006.py:4:10: UP006 [*] Use `list` instead of `typing.List` for type annotation
6 6 |
7 7 |

UP006.py:11:10: UP006 [*] Use `list` instead of `List` for type annotation
UP006_0.py:11:10: UP006 [*] Use `list` instead of `List` for type annotation
|
11 | def f(x: List[str]) -> None:
| ^^^^ UP006
Expand All @@ -37,7 +37,7 @@ UP006.py:11:10: UP006 [*] Use `list` instead of `List` for type annotation
13 13 |
14 14 |

UP006.py:18:10: UP006 [*] Use `list` instead of `t.List` for type annotation
UP006_0.py:18:10: UP006 [*] Use `list` instead of `t.List` for type annotation
|
18 | def f(x: t.List[str]) -> None:
| ^^^^^^ UP006
Expand All @@ -55,7 +55,7 @@ UP006.py:18:10: UP006 [*] Use `list` instead of `t.List` for type annotation
20 20 |
21 21 |

UP006.py:25:10: UP006 [*] Use `list` instead of `IList` for type annotation
UP006_0.py:25:10: UP006 [*] Use `list` instead of `IList` for type annotation
|
25 | def f(x: IList[str]) -> None:
| ^^^^^ UP006
Expand All @@ -73,7 +73,7 @@ UP006.py:25:10: UP006 [*] Use `list` instead of `IList` for type annotation
27 27 |
28 28 |

UP006.py:29:11: UP006 [*] Use `list` instead of `List` for type annotation
UP006_0.py:29:11: UP006 [*] Use `list` instead of `List` for type annotation
|
29 | def f(x: "List[str]") -> None:
| ^^^^ UP006
Expand All @@ -91,7 +91,7 @@ UP006.py:29:11: UP006 [*] Use `list` instead of `List` for type annotation
31 31 |
32 32 |

UP006.py:33:12: UP006 [*] Use `list` instead of `List` for type annotation
UP006_0.py:33:12: UP006 [*] Use `list` instead of `List` for type annotation
|
33 | def f(x: r"List[str]") -> None:
| ^^^^ UP006
Expand All @@ -109,7 +109,7 @@ UP006.py:33:12: UP006 [*] Use `list` instead of `List` for type annotation
35 35 |
36 36 |

UP006.py:37:11: UP006 [*] Use `list` instead of `List` for type annotation
UP006_0.py:37:11: UP006 [*] Use `list` instead of `List` for type annotation
|
37 | def f(x: "List[str]") -> None:
| ^^^^ UP006
Expand All @@ -127,7 +127,7 @@ UP006.py:37:11: UP006 [*] Use `list` instead of `List` for type annotation
39 39 |
40 40 |

UP006.py:41:13: UP006 [*] Use `list` instead of `List` for type annotation
UP006_0.py:41:13: UP006 [*] Use `list` instead of `List` for type annotation
|
41 | def f(x: """List[str]""") -> None:
| ^^^^ UP006
Expand All @@ -145,15 +145,15 @@ UP006.py:41:13: UP006 [*] Use `list` instead of `List` for type annotation
43 43 |
44 44 |

UP006.py:45:10: UP006 Use `list` instead of `List` for type annotation
UP006_0.py:45:10: UP006 Use `list` instead of `List` for type annotation
|
45 | def f(x: "Li" "st[str]") -> None:
| ^^^^^^^^^^^^^^ UP006
46 | ...
|
= help: Replace with `list`

UP006.py:49:11: UP006 [*] Use `list` instead of `List` for type annotation
UP006_0.py:49:11: UP006 [*] Use `list` instead of `List` for type annotation
|
49 | def f(x: "List['List[str]']") -> None:
| ^^^^ UP006
Expand All @@ -171,7 +171,7 @@ UP006.py:49:11: UP006 [*] Use `list` instead of `List` for type annotation
51 51 |
52 52 |

UP006.py:49:17: UP006 [*] Use `list` instead of `List` for type annotation
UP006_0.py:49:17: UP006 [*] Use `list` instead of `List` for type annotation
|
49 | def f(x: "List['List[str]']") -> None:
| ^^^^ UP006
Expand All @@ -189,7 +189,7 @@ UP006.py:49:17: UP006 [*] Use `list` instead of `List` for type annotation
51 51 |
52 52 |

UP006.py:53:11: UP006 [*] Use `list` instead of `List` for type annotation
UP006_0.py:53:11: UP006 [*] Use `list` instead of `List` for type annotation
|
53 | def f(x: "List['Li' 'st[str]']") -> None:
| ^^^^ UP006
Expand All @@ -207,31 +207,31 @@ UP006.py:53:11: UP006 [*] Use `list` instead of `List` for type annotation
55 55 |
56 56 |

UP006.py:53:16: UP006 Use `list` instead of `List` for type annotation
UP006_0.py:53:16: UP006 Use `list` instead of `List` for type annotation
|
53 | def f(x: "List['Li' 'st[str]']") -> None:
| ^^^^^^^^^^^^^^ UP006
54 | ...
|
= help: Replace with `list`

UP006.py:57:10: UP006 Use `list` instead of `List` for type annotation
UP006_0.py:57:10: UP006 Use `list` instead of `List` for type annotation
|
57 | def f(x: "Li" "st['List[str]']") -> None:
| ^^^^^^^^^^^^^^^^^^^^^^ UP006
58 | ...
|
= help: Replace with `list`

UP006.py:57:10: UP006 Use `list` instead of `List` for type annotation
UP006_0.py:57:10: UP006 Use `list` instead of `List` for type annotation
|
57 | def f(x: "Li" "st['List[str]']") -> None:
| ^^^^^^^^^^^^^^^^^^^^^^ UP006
58 | ...
|
= help: Replace with `list`

UP006.py:61:10: UP006 [*] Use `collections.deque` instead of `typing.Deque` for type annotation
UP006_0.py:61:10: UP006 [*] Use `collections.deque` instead of `typing.Deque` for type annotation
|
61 | def f(x: typing.Deque[str]) -> None:
| ^^^^^^^^^^^^ UP006
Expand All @@ -257,7 +257,7 @@ UP006.py:61:10: UP006 [*] Use `collections.deque` instead of `typing.Deque` for
63 64 |
64 65 |

UP006.py:65:10: UP006 [*] Use `collections.defaultdict` instead of `typing.DefaultDict` for type annotation
UP006_0.py:65:10: UP006 [*] Use `collections.defaultdict` instead of `typing.DefaultDict` for type annotation
|
65 | def f(x: typing.DefaultDict[str, str]) -> None:
| ^^^^^^^^^^^^^^^^^^ UP006
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
---
source: crates/ruff/src/rules/pyupgrade/mod.rs
---
UP006_1.py:9:10: UP006 [*] Use `collections.defaultdict` instead of `typing.DefaultDict` for type annotation
|
9 | def f(x: typing.DefaultDict[str, str]) -> None:
| ^^^^^^^^^^^^^^^^^^ UP006
10 | ...
|
= help: Replace with `collections.defaultdict`

Suggested fix
6 6 | from collections import defaultdict
7 7 |
8 8 |
9 |-def f(x: typing.DefaultDict[str, str]) -> None:
9 |+def f(x: defaultdict[str, str]) -> None:
10 10 | ...


Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
source: crates/ruff/src/rules/pyupgrade/mod.rs
---
UP006_2.py:7:10: UP006 Use `collections.defaultdict` instead of `typing.DefaultDict` for type annotation
|
7 | def f(x: typing.DefaultDict[str, str]) -> None:
| ^^^^^^^^^^^^^^^^^^ UP006
8 | ...
|
= help: Replace with `collections.defaultdict`


Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
---
source: crates/ruff/src/rules/pyupgrade/mod.rs
---
UP006_3.py:7:11: UP006 [*] Use `collections.defaultdict` instead of `typing.DefaultDict` for type annotation
|
7 | def f(x: "typing.DefaultDict[str, str]") -> None:
| ^^^^^^^^^^^^^^^^^^ UP006
8 | ...
|
= help: Replace with `collections.defaultdict`

Suggested fix
4 4 | from collections import defaultdict
5 5 |
6 6 |
7 |-def f(x: "typing.DefaultDict[str, str]") -> None:
7 |+def f(x: "defaultdict[str, str]") -> None:
8 8 | ...


Loading

0 comments on commit c271db8

Please sign in to comment.