Stop overriding locations for expressions within f-strings (#1494)

This commit is contained in:
Harutaka Kawamura 2022-12-31 13:43:59 +09:00 committed by GitHub
parent 01c74e0629
commit 3e23fd1487
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
12 changed files with 32 additions and 95 deletions

8
Cargo.lock generated
View file

@ -2010,7 +2010,7 @@ dependencies = [
[[package]]
name = "rustpython-ast"
version = "0.1.0"
source = "git+https://github.com/RustPython/RustPython.git?rev=68d26955b3e24198a150315e7959719b03709dee#68d26955b3e24198a150315e7959719b03709dee"
source = "git+https://github.com/RustPython/RustPython.git?rev=8cb2b8292062adf13bde1b863a9b02c9f0bda3dd#8cb2b8292062adf13bde1b863a9b02c9f0bda3dd"
dependencies = [
"num-bigint",
"rustpython-common",
@ -2020,7 +2020,7 @@ dependencies = [
[[package]]
name = "rustpython-common"
version = "0.0.0"
source = "git+https://github.com/RustPython/RustPython.git?rev=68d26955b3e24198a150315e7959719b03709dee#68d26955b3e24198a150315e7959719b03709dee"
source = "git+https://github.com/RustPython/RustPython.git?rev=8cb2b8292062adf13bde1b863a9b02c9f0bda3dd#8cb2b8292062adf13bde1b863a9b02c9f0bda3dd"
dependencies = [
"ascii",
"cfg-if 1.0.0",
@ -2043,7 +2043,7 @@ dependencies = [
[[package]]
name = "rustpython-compiler-core"
version = "0.1.2"
source = "git+https://github.com/RustPython/RustPython.git?rev=68d26955b3e24198a150315e7959719b03709dee#68d26955b3e24198a150315e7959719b03709dee"
source = "git+https://github.com/RustPython/RustPython.git?rev=8cb2b8292062adf13bde1b863a9b02c9f0bda3dd#8cb2b8292062adf13bde1b863a9b02c9f0bda3dd"
dependencies = [
"bincode",
"bitflags",
@ -2060,7 +2060,7 @@ dependencies = [
[[package]]
name = "rustpython-parser"
version = "0.1.2"
source = "git+https://github.com/RustPython/RustPython.git?rev=68d26955b3e24198a150315e7959719b03709dee#68d26955b3e24198a150315e7959719b03709dee"
source = "git+https://github.com/RustPython/RustPython.git?rev=8cb2b8292062adf13bde1b863a9b02c9f0bda3dd#8cb2b8292062adf13bde1b863a9b02c9f0bda3dd"
dependencies = [
"ahash",
"anyhow",

View file

@ -53,9 +53,9 @@ regex = { version = "1.6.0" }
ropey = { version = "1.5.0", features = ["cr_lines", "simd"], default-features = false }
ruff_macros = { version = "0.0.203", path = "ruff_macros" }
rustc-hash = { version = "1.1.0" }
rustpython-ast = { features = ["unparse"], git = "https://github.com/RustPython/RustPython.git", rev = "68d26955b3e24198a150315e7959719b03709dee" }
rustpython-common = { git = "https://github.com/RustPython/RustPython.git", rev = "68d26955b3e24198a150315e7959719b03709dee" }
rustpython-parser = { features = ["lalrpop"], git = "https://github.com/RustPython/RustPython.git", rev = "68d26955b3e24198a150315e7959719b03709dee" }
rustpython-ast = { features = ["unparse"], git = "https://github.com/RustPython/RustPython.git", rev = "8cb2b8292062adf13bde1b863a9b02c9f0bda3dd" }
rustpython-common = { git = "https://github.com/RustPython/RustPython.git", rev = "8cb2b8292062adf13bde1b863a9b02c9f0bda3dd" }
rustpython-parser = { features = ["lalrpop"], git = "https://github.com/RustPython/RustPython.git", rev = "8cb2b8292062adf13bde1b863a9b02c9f0bda3dd" }
schemars = { version = "0.8.11" }
semver = { version = "1.0.16" }
serde = { version = "1.0.147", features = ["derive"] }

View file

@ -11,9 +11,9 @@ itertools = { version = "0.10.5" }
libcst = { git = "https://github.com/charliermarsh/LibCST", rev = "f2f0b7a487a8725d161fe8b3ed73a6758b21e177" }
once_cell = { version = "1.16.0" }
ruff = { path = ".." }
rustpython-ast = { features = ["unparse"], git = "https://github.com/RustPython/RustPython.git", rev = "68d26955b3e24198a150315e7959719b03709dee" }
rustpython-common = { git = "https://github.com/RustPython/RustPython.git", rev = "68d26955b3e24198a150315e7959719b03709dee" }
rustpython-parser = { features = ["lalrpop"], git = "https://github.com/RustPython/RustPython.git", rev = "68d26955b3e24198a150315e7959719b03709dee" }
rustpython-ast = { features = ["unparse"], git = "https://github.com/RustPython/RustPython.git", rev = "8cb2b8292062adf13bde1b863a9b02c9f0bda3dd" }
rustpython-common = { git = "https://github.com/RustPython/RustPython.git", rev = "8cb2b8292062adf13bde1b863a9b02c9f0bda3dd" }
rustpython-parser = { features = ["lalrpop"], git = "https://github.com/RustPython/RustPython.git", rev = "8cb2b8292062adf13bde1b863a9b02c9f0bda3dd" }
schemars = { version = "0.8.11" }
serde_json = {version="1.0.91"}
strum = { version = "0.24.1", features = ["strum_macros"] }

View file

@ -87,7 +87,6 @@ pub struct Checker<'a> {
deferred_assignments: Vec<DeferralContext<'a>>,
// Internal, derivative state.
visible_scope: VisibleScope,
in_f_string: Option<Range>,
in_annotation: bool,
in_type_definition: bool,
in_deferred_string_type_definition: bool,
@ -144,7 +143,6 @@ impl<'a> Checker<'a> {
modifier: Modifier::Module,
visibility: module_visibility(path),
},
in_f_string: None,
in_annotation: false,
in_type_definition: false,
in_deferred_string_type_definition: false,
@ -161,14 +159,7 @@ impl<'a> Checker<'a> {
}
/// Add a `Check` to the `Checker`.
pub(crate) fn add_check(&mut self, mut check: Check) {
// If we're in an f-string, override the location. RustPython doesn't produce
// reliable locations for expressions within f-strings, so we use the
// span of the f-string itself as a best-effort default.
if let Some(range) = self.in_f_string {
check.location = range.location;
check.end_location = range.end_location;
}
pub(crate) fn add_check(&mut self, check: Check) {
self.checks.push(check);
}
@ -184,16 +175,7 @@ impl<'a> Checker<'a> {
pub fn patch(&self, code: &CheckCode) -> bool {
// TODO(charlie): We can't fix errors in f-strings until RustPython adds
// location data.
matches!(self.autofix, flags::Autofix::Enabled)
&& self.in_f_string.is_none()
&& self.settings.fixable.contains(code)
}
/// Return the amended `Range` from a `Located`.
pub fn range_for<T>(&self, located: &Located<T>) -> Range {
// If we're in an f-string, override the location.
self.in_f_string
.unwrap_or_else(|| Range::from_located(located))
matches!(self.autofix, flags::Autofix::Enabled) && self.settings.fixable.contains(code)
}
/// Return `true` if the `Expr` is a reference to `typing.${target}`.
@ -1479,7 +1461,6 @@ where
self.push_expr(expr);
let prev_in_f_string = self.in_f_string;
let prev_in_literal = self.in_literal;
let prev_in_type_definition = self.in_type_definition;
@ -2166,10 +2147,9 @@ where
}
ExprKind::JoinedStr { values } => {
if self.settings.enabled.contains(&CheckCode::F541) {
if self.in_f_string.is_none()
&& !values
.iter()
.any(|value| matches!(value.node, ExprKind::FormattedValue { .. }))
if !values
.iter()
.any(|value| matches!(value.node, ExprKind::FormattedValue { .. }))
{
self.add_check(Check::new(
CheckKind::FStringMissingPlaceholders,
@ -2177,7 +2157,6 @@ where
));
}
}
self.in_f_string = Some(Range::from_located(expr));
}
ExprKind::BinOp {
left,
@ -2687,7 +2666,6 @@ where
self.in_type_definition = prev_in_type_definition;
self.in_literal = prev_in_literal;
self.in_f_string = prev_in_f_string;
self.pop_expr();
}

View file

@ -12,8 +12,6 @@ use crate::checks::{Check, CheckKind};
struct LoadedNamesVisitor<'a> {
// Tuple of: name, defining expression, and defining range.
names: Vec<(&'a str, &'a Expr, Range)>,
// If we're in an f-string, the range of the defining expression.
in_f_string: Option<Range>,
}
/// `Visitor` to collect all used identifiers in a statement.
@ -24,18 +22,10 @@ where
fn visit_expr(&mut self, expr: &'b Expr) {
match &expr.node {
ExprKind::JoinedStr { .. } => {
let prev_in_f_string = self.in_f_string;
self.in_f_string = Some(Range::from_located(expr));
visitor::walk_expr(self, expr);
self.in_f_string = prev_in_f_string;
}
ExprKind::Name { id, ctx } if matches!(ctx, ExprContext::Load) => {
self.names.push((
id,
expr,
self.in_f_string
.unwrap_or_else(|| Range::from_located(expr)),
));
self.names.push((id, expr, Range::from_located(expr)));
}
_ => visitor::walk_expr(self, expr),
}

View file

@ -166,10 +166,10 @@ expression: checks
FunctionUsesLoopVariable: i
location:
row: 82
column: 12
column: 15
end_location:
row: 82
column: 18
column: 16
fix: ~
parent: ~

View file

@ -19,8 +19,6 @@ pub struct Stack<'a> {
#[derive(Default)]
pub struct ReturnVisitor<'a> {
pub stack: Stack<'a>,
// If we're in an f-string, the location of the defining expression.
in_f_string: Option<Location>,
}
impl<'a> ReturnVisitor<'a> {
@ -37,7 +35,7 @@ impl<'a> ReturnVisitor<'a> {
.assigns
.entry(id)
.or_insert_with(Vec::new)
.push(self.in_f_string.unwrap_or(expr.location));
.push(expr.location);
return;
}
_ => {}
@ -78,7 +76,7 @@ impl<'a> Visitor<'a> for ReturnVisitor<'a> {
.refs
.entry(id)
.or_insert_with(Vec::new)
.push(self.in_f_string.unwrap_or(value.location));
.push(value.location);
}
visitor::walk_expr(self, value);
@ -123,7 +121,7 @@ impl<'a> Visitor<'a> for ReturnVisitor<'a> {
.refs
.entry(name)
.or_insert_with(Vec::new)
.push(self.in_f_string.unwrap_or(expr.location));
.push(expr.location);
}
}
ExprKind::Name { id, .. } => {
@ -131,13 +129,10 @@ impl<'a> Visitor<'a> for ReturnVisitor<'a> {
.refs
.entry(id)
.or_insert_with(Vec::new)
.push(self.in_f_string.unwrap_or(expr.location));
.push(expr.location);
}
ExprKind::JoinedStr { .. } => {
let prev_in_f_string = self.in_f_string;
self.in_f_string = Some(expr.location);
visitor::walk_expr(self, expr);
self.in_f_string = prev_in_f_string;
}
_ => visitor::walk_expr(self, expr),
}

View file

@ -66,20 +66,20 @@ expression: checks
UndefinedName: B
location:
row: 87
column: 4
column: 7
end_location:
row: 87
column: 10
column: 8
fix: ~
parent: ~
- kind:
UndefinedName: B
location:
row: 89
column: 4
row: 90
column: 7
end_location:
row: 90
column: 10
column: 8
fix: ~
parent: ~
- kind:

View file

@ -2,31 +2,5 @@
source: src/pyflakes/mod.rs
expression: checks
---
- kind: DuplicateArgumentName
location:
row: 1
column: 24
end_location:
row: 1
column: 30
fix: ~
parent: ~
- kind: DuplicateArgumentName
location:
row: 5
column: 27
end_location:
row: 5
column: 33
fix: ~
parent: ~
- kind: DuplicateArgumentName
location:
row: 9
column: 26
end_location:
row: 9
column: 32
fix: ~
parent: ~
[]

View file

@ -13,7 +13,7 @@ pub fn used_prior_global_declaration(checker: &mut Checker, name: &str, expr: &E
_ => return,
};
if let Some(stmt) = globals.get(name) {
if checker.range_for(expr).location < stmt.location {
if expr.location < stmt.location {
checker.add_check(Check::new(
CheckKind::UsedPriorGlobalDeclaration(name.to_string(), stmt.location.row()),
Range::from_located(expr),

View file

@ -152,10 +152,10 @@ expression: checks
- 114
location:
row: 113
column: 10
column: 13
end_location:
row: 113
column: 17
column: 14
fix: ~
parent: ~

View file

@ -57,7 +57,7 @@ pub fn native_literals(
.slice_source_code_range(&Range::from_located(arg));
if lexer::make_tokenizer(&arg_code)
.flatten()
.filter(|(_, tok, _)| matches!(tok, Tok::String { .. } | Tok::Bytes { .. }))
.filter(|(_, tok, _)| matches!(tok, Tok::String { .. }))
.count()
> 1
{