Skip to content

Commit

Permalink
Respect tuple assignments in typing analyzer (astral-sh#9969)
Browse files Browse the repository at this point in the history 
## Summary

Just addressing some discrepancies between the analyzers like `is_dict`
and the logic that's matured in `find_binding_value`.
  • Loading branch information
@charliermarsh @nkxxll
charliermarsh authored and nkxxll committed Mar 4, 2024
1 parent 8e2e020 commit 5ccd272
Show file tree
Hide file tree
Showing 3 changed files with 92 additions and 47 deletions.
20 changes: 14 additions & 6 deletions crates/ruff_linter/resources/test/fixtures/refurb/FURB129.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,38 +21,46 @@
pass


def good1():
def func():
f = Path("FURB129.py").open()
for _line in f.readlines():
pass
f.close()


def good2(f: io.BytesIO):
def func(f: io.BytesIO):
for _line in f.readlines():
pass


def func():
with (open("FURB129.py") as f, foo as bar):
for _line in f.readlines():
pass
for _line in bar.readlines():
pass


# False positives
def bad(f):
def func(f):
for _line in f.readlines():
pass


def worse(f: codecs.StreamReader):
def func(f: codecs.StreamReader):
for _line in f.readlines():
pass


def foo():
def func():
class A:
def readlines(self) -> list[str]:
return ["a", "b", "c"]

return A()


for _line in foo().readlines():
for _line in func().readlines():
pass

# OK
Expand Down
29 changes: 25 additions & 4 deletions ...ter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB129_FURB129.py.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ FURB129.py:20:14: FURB129 [*] Instead of calling `readlines()`, iterate over fil

FURB129.py:26:18: FURB129 [*] Instead of calling `readlines()`, iterate over file object directly
|
24 | def good1():
24 | def func():
25 | f = Path("FURB129.py").open()
26 | for _line in f.readlines():
| ^^^^^^^^^^^^^ FURB129
Expand All @@ -156,7 +156,7 @@ FURB129.py:26:18: FURB129 [*] Instead of calling `readlines()`, iterate over fil

ℹ Unsafe fix
23 23 |
24 24 | def good1():
24 24 | def func():
25 25 | f = Path("FURB129.py").open()
26 |- for _line in f.readlines():
26 |+ for _line in f:
Expand All @@ -166,7 +166,7 @@ FURB129.py:26:18: FURB129 [*] Instead of calling `readlines()`, iterate over fil

FURB129.py:32:18: FURB129 [*] Instead of calling `readlines()`, iterate over file object directly
|
31 | def good2(f: io.BytesIO):
31 | def func(f: io.BytesIO):
32 | for _line in f.readlines():
| ^^^^^^^^^^^^^ FURB129
33 | pass
Expand All @@ -176,11 +176,32 @@ FURB129.py:32:18: FURB129 [*] Instead of calling `readlines()`, iterate over fil
ℹ Unsafe fix
29 29 |
30 30 |
31 31 | def good2(f: io.BytesIO):
31 31 | def func(f: io.BytesIO):
32 |- for _line in f.readlines():
32 |+ for _line in f:
33 33 | pass
34 34 |
35 35 |

FURB129.py:38:22: FURB129 [*] Instead of calling `readlines()`, iterate over file object directly
|
36 | def func():
37 | with (open("FURB129.py") as f, foo as bar):
38 | for _line in f.readlines():
| ^^^^^^^^^^^^^ FURB129
39 | pass
40 | for _line in bar.readlines():
|
= help: Remove `readlines()`

ℹ Unsafe fix
35 35 |
36 36 | def func():
37 37 | with (open("FURB129.py") as f, foo as bar):
38 |- for _line in f.readlines():
38 |+ for _line in f:
39 39 | pass
40 40 | for _line in bar.readlines():
41 41 | pass


90 changes: 53 additions & 37 deletions crates/ruff_python_semantic/src/analyze/typing.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -421,14 +421,17 @@ pub trait TypeChecker {
fn check_type<T: TypeChecker>(binding: &Binding, semantic: &SemanticModel) -> bool {
match binding.kind {
BindingKind::Assignment => match binding.statement(semantic) {
// Given:
//
// ```python
// x = init_expr
// ```
//
// The type checker might know how to infer the type based on `init_expr`.
Some(Stmt::Assign(ast::StmtAssign { value, .. })) => {
T::match_initializer(value.as_ref(), semantic)
}
Some(Stmt::Assign(ast::StmtAssign { targets, value, .. })) => targets
.iter()
.find_map(|target| match_value(binding, target, value.as_ref()))
.is_some_and(|value| T::match_initializer(value, semantic)),

// ```python
// x: annotation = some_expr
Expand All @@ -438,24 +441,40 @@ fn check_type<T: TypeChecker>(binding: &Binding, semantic: &SemanticModel) -> bo
Some(Stmt::AnnAssign(ast::StmtAnnAssign { annotation, .. })) => {
T::match_annotation(annotation.as_ref(), semantic)
}

_ => false,
},

BindingKind::NamedExprAssignment => {
// ```python
// if (x := some_expr) is not None:
// ...
// ```
binding.source.is_some_and(|source| {
semantic
.expressions(source)
.find_map(|expr| expr.as_named_expr_expr())
.and_then(|ast::ExprNamedExpr { target, value, .. }| {
match_value(binding, target.as_ref(), value.as_ref())
})
.is_some_and(|value| T::match_initializer(value, semantic))
})
}

BindingKind::WithItemVar => match binding.statement(semantic) {
// ```python
// with open("file.txt") as x:
// ...
// ...
// ```
Some(Stmt::With(ast::StmtWith { items, .. })) => {
let Some(item) = items.iter().find(|item| {
item.optional_vars
.as_ref()
.is_some_and(|vars| vars.range().contains_range(binding.range))
}) else {
return false;
};
T::match_initializer(&item.context_expr, semantic)
}
Some(Stmt::With(ast::StmtWith { items, .. })) => items
.iter()
.find_map(|item| {
let target = item.optional_vars.as_ref()?;
let value = &item.context_expr;
match_value(binding, target, value)
})
.is_some_and(|value| T::match_initializer(value, semantic)),

_ => false,
},

Expand All @@ -475,6 +494,7 @@ fn check_type<T: TypeChecker>(binding: &Binding, semantic: &SemanticModel) -> bo
};
T::match_annotation(annotation.as_ref(), semantic)
}

_ => false,
},

Expand Down Expand Up @@ -775,6 +795,7 @@ pub fn find_assigned_value<'a>(symbol: &str, semantic: &'a SemanticModel<'a>) ->
///
/// This function will return a `NumberLiteral` with value `Int(42)` when called with `foo` and a
/// `StringLiteral` with value `"str"` when called with `bla`.
#[allow(clippy::single_match)]
pub fn find_binding_value<'a>(binding: &Binding, semantic: &'a SemanticModel) -> Option<&'a Expr> {
match binding.kind {
// Ex) `x := 1`
Expand All @@ -788,37 +809,32 @@ pub fn find_binding_value<'a>(binding: &Binding, semantic: &'a SemanticModel) ->
}
}
// Ex) `x = 1`
BindingKind::Assignment => {
let parent_id = binding.source?;
let parent = semantic.statement(parent_id);
match parent {
Stmt::Assign(ast::StmtAssign { value, targets, .. }) => {
return targets
.iter()
.find_map(|target| match_value(binding, target, value.as_ref()))
}
Stmt::AnnAssign(ast::StmtAnnAssign {
value: Some(value),
target,
..
}) => {
return match_value(binding, target, value.as_ref());
}
_ => {}
BindingKind::Assignment => match binding.statement(semantic) {
Some(Stmt::Assign(ast::StmtAssign { value, targets, .. })) => {
return targets
.iter()
.find_map(|target| match_value(binding, target, value.as_ref()))
}
}
Some(Stmt::AnnAssign(ast::StmtAnnAssign {
value: Some(value),
target,
..
})) => {
return match_value(binding, target, value.as_ref());
}
_ => {}
},
// Ex) `with open("file.txt") as f:`
BindingKind::WithItemVar => {
let parent_id = binding.source?;
let parent = semantic.statement(parent_id);
if let Stmt::With(ast::StmtWith { items, .. }) = parent {
BindingKind::WithItemVar => match binding.statement(semantic) {
Some(Stmt::With(ast::StmtWith { items, .. })) => {
return items.iter().find_map(|item| {
let target = item.optional_vars.as_ref()?;
let value = &item.context_expr;
match_value(binding, target, value)
});
}
}
_ => {}
},
_ => {}
}
None
Expand Down

0 comments on commit 5ccd272

Please sign in to comment.