From d96a0dbe5740ba82cb558918001dcd21bc8bd767 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 13 Feb 2024 00:02:52 -0500 Subject: [PATCH] Respect tuple assignments in typing analyzer (#9969) ## Summary Just addressing some discrepancies between the analyzers like `is_dict` and the logic that's matured in `find_binding_value`. --- .../resources/test/fixtures/refurb/FURB129.py | 20 +++-- ...es__refurb__tests__FURB129_FURB129.py.snap | 29 +++++- .../src/analyze/typing.rs | 90 +++++++++++-------- 3 files changed, 92 insertions(+), 47 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB129.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB129.py index 1f42b1a10b..afa58e9798 100644 --- a/crates/ruff_linter/resources/test/fixtures/refurb/FURB129.py +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB129.py @@ -21,30 +21,38 @@ for _line in Path("FURB129.py").open().readlines(): 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"] @@ -52,7 +60,7 @@ def foo(): return A() -for _line in foo().readlines(): +for _line in func().readlines(): pass # OK diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB129_FURB129.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB129_FURB129.py.snap index 2d3c00155b..e24fa164ed 100644 --- a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB129_FURB129.py.snap +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB129_FURB129.py.snap @@ -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 @@ -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: @@ -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 @@ -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 + diff --git a/crates/ruff_python_semantic/src/analyze/typing.rs b/crates/ruff_python_semantic/src/analyze/typing.rs index 48d3b597f1..a625110da2 100644 --- a/crates/ruff_python_semantic/src/analyze/typing.rs +++ b/crates/ruff_python_semantic/src/analyze/typing.rs @@ -421,14 +421,17 @@ pub trait TypeChecker { fn check_type(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 @@ -438,24 +441,40 @@ fn check_type(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, }, @@ -475,6 +494,7 @@ fn check_type(binding: &Binding, semantic: &SemanticModel) -> bo }; T::match_annotation(annotation.as_ref(), semantic) } + _ => false, }, @@ -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` @@ -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