mirrors/ruff
1
0
Fork 0
mirror of https://github.com/astral-sh/ruff.git synced 2025-08-19 01:50:38 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

Avoid using typing-imported symbols for runtime edits (#4649)

Browse source
This commit is contained in:
Charlie Marsh 2023-05-26 20:36:37 -04:00 • committed by GitHub
parent ccca11839a
commit af433ac14d
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
14 changed files with 219 additions and 60 deletions
Download patch file Download diff file Expand all files Collapse all files

8
crates/ruff/resources/test/fixtures/pylint/sys_exit_alias_10.py vendored Normal file
View file

@ -0,0 +1,8 @@
from typing import TYPE_CHECKING
if TYPE_CHECKING:
from sys import exit as bar
def main():
exit(0)

0
crates/ruff/resources/test/fixtures/pyupgrade/UP006.py → crates/ruff/resources/test/fixtures/pyupgrade/UP006_0.py vendored
View file

10
crates/ruff/resources/test/fixtures/pyupgrade/UP006_1.py vendored Normal file
View file

@ -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
crates/ruff/resources/test/fixtures/pyupgrade/UP006_2.py vendored Normal file
View file

@ -0,0 +1,8 @@
import typing
if typing.TYPE_CHECKING:
from collections import defaultdict
def f(x: typing.DefaultDict[str, str]) -> None:
...

8
crates/ruff/resources/test/fixtures/pyupgrade/UP006_3.py vendored Normal file
View file

@ -0,0 +1,8 @@
import typing
if typing.TYPE_CHECKING:
from collections import defaultdict
def f(x: "typing.DefaultDict[str, str]") -> None:
...

51
crates/ruff/src/importer/mod.rs
View file

@ -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`].
@ -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
@ -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
@ -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
crates/ruff/src/rules/pylint/mod.rs
View file

@ -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")]

12
crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR1722_sys_exit_alias_10.py.snap Normal file
View file

@ -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
crates/ruff/src/rules/pyupgrade/mod.rs
View file

@ -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")]

34
crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP006.py.snap → crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP006_0.py.snap
View file

@ -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
@ -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
@ -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
@ -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
@ -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
@ -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
@ -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
@ -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
@ -145,7 +145,7 @@ 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
@ -153,7 +153,7 @@ UP006.py:45:10: UP006 Use `list` instead of `List` for type annotation
|
= 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
@ -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
@ -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
@ -207,7 +207,7 @@ 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
@ -215,7 +215,7 @@ UP006.py:53:16: UP006 Use `list` instead of `List` for type annotation
|
= 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
@ -223,7 +223,7 @@ UP006.py:57:10: UP006 Use `list` instead of `List` for type annotation
|
= 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
@ -231,7 +231,7 @@ UP006.py:57:10: UP006 Use `list` instead of `List` for type annotation
|
= 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
@ -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

20
crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP006_1.py.snap Normal file
View file

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

12
crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP006_2.py.snap Normal file
View file

@ -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`

20
crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP006_3.py.snap Normal file
View file

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

54
crates/ruff_python_semantic/src/model.rs
View file

@ -4,7 +4,7 @@ use std::path::Path;
use bitflags::bitflags;
use nohash_hasher::{BuildNoHashHasher, IntMap};
use ruff_text_size::TextRange;
use rustpython_parser::ast::{Expr, Stmt};
use rustpython_parser::ast::{Expr, Ranged, Stmt};
use smallvec::smallvec;
use ruff_python_ast::call_path::{collect_call_path, from_unqualified_name, CallPath};
@ -350,7 +350,7 @@ impl<'a> SemanticModel<'a> {
&self,
module: &str,
member: &str,
) -> Option<(&Stmt, String)> {
) -> Option<ImportedName> {
self.scopes().enumerate().find_map(|(scope_index, scope)| {
scope.binding_ids().find_map(|binding_id| {
let binding = &self.bindings[binding_id];
@ -360,14 +360,18 @@ impl<'a> SemanticModel<'a> {
// `import sys as sys2` -> `sys2.exit`
BindingKind::Importation(Importation { name, full_name }) => {
if full_name == &module {
if let Some(source) = binding.source {
// Verify that `sys` isn't bound in an inner scope.
if self
.scopes()
.take(scope_index)
.all(|scope| scope.get(name).is_none())
{
if let Some(source) = binding.source {
return Some((self.stmts[source], format!("{name}.{member}")));
return Some(ImportedName {
name: format!("{name}.{member}"),
range: self.stmts[source].range(),
context: binding.context,
});
}
}
}
@ -378,14 +382,18 @@ impl<'a> SemanticModel<'a> {
BindingKind::FromImportation(FromImportation { name, full_name }) => {
if let Some((target_module, target_member)) = full_name.split_once('.') {
if target_module == module && target_member == member {
if let Some(source) = binding.source {
// Verify that `join` isn't bound in an inner scope.
if self
.scopes()
.take(scope_index)
.all(|scope| scope.get(name).is_none())
{
if let Some(source) = binding.source {
return Some((self.stmts[source], (*name).to_string()));
return Some(ImportedName {
name: (*name).to_string(),
range: self.stmts[source].range(),
context: binding.context,
});
}
}
}
@ -395,14 +403,18 @@ impl<'a> SemanticModel<'a> {
// `import os.path ` -> `os.name`
BindingKind::SubmoduleImportation(SubmoduleImportation { name, .. }) => {
if name == &module {
if let Some(source) = binding.source {
// Verify that `os` isn't bound in an inner scope.
if self
.scopes()
.take(scope_index)
.all(|scope| scope.get(name).is_none())
{
if let Some(source) = binding.source {
return Some((self.stmts[source], format!("{name}.{member}")));
return Some(ImportedName {
name: format!("{name}.{member}"),
range: self.stmts[source].range(),
context: binding.context,
});
}
}
}
@ -911,3 +923,29 @@ pub enum ResolvedReference {
/// The reference is definitively unresolved.
NotFound,
}
#[derive(Debug)]
pub struct ImportedName {
/// The name to which the imported symbol is bound.
name: String,
/// The range at which the symbol is imported.
range: TextRange,
/// The context in which the symbol is imported.
context: ExecutionContext,
}
impl ImportedName {
pub fn into_name(self) -> String {
self.name
}
pub const fn context(&self) -> ExecutionContext {
self.context
}
}
impl Ranged for ImportedName {
fn range(&self) -> TextRange {
self.range
}
}