[refurb] Implement readlines_in_for lint (FURB129) (#9880)

## Summary
Implement [implicit readlines
(FURB129)](https://github.com/dosisod/refurb/blob/master/refurb/checks/iterable/implicit_readlines.py)
lint.

## Notes
I need a help/an opinion about suggested implementations.

This implementation differs from the original one from `refurb` in the
following way. This implementation checks syntactically the call of the
method with the name `readlines()` inside `for` {loop|generator
expression}. The implementation from refurb also
[checks](https://github.com/dosisod/refurb/blob/master/refurb/checks/iterable/implicit_readlines.py#L43)
that callee is a variable with a type `io.TextIOWrapper` or
`io.BufferedReader`.

- I do not see a simple way to implement the same logic.
- The best I can have is something like
```rust
checker.semantic().binding(checker.semantic().resolve_name(attr_expr.value.as_name_expr()?)?).statement(checker.semantic())
```
and analyze cases. But this will be not about types, but about guessing
the type by assignment (or with) expression.
- Also this logic has several false negatives, when the callee is not a
variable, but the result of function call (e.g. `open(...)`).
- On the other side, maybe it is good to lint this on other things,
where this suggestion is not safe, and push the developers to change
their interfaces to be less surprising, comparing with the standard
library.
- Anyway while the current implementation has false-positives (I
mentioned some of them in the test) I marked the fixes to be unsafe.
This commit is contained in:
Aleksei Latyshev 2024-02-13 04:28:35 +01:00 committed by GitHub
parent 609d0a9a65
commit dd0ba16a79
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
10 changed files with 484 additions and 7 deletions

View file

@ -406,7 +406,7 @@ where
}
/// Abstraction for a type checker, conservatively checks for the intended type(s).
trait TypeChecker {
pub trait TypeChecker {
/// Check annotation expression to match the intended type(s).
fn match_annotation(annotation: &Expr, semantic: &SemanticModel) -> bool;
/// Check initializer expression to match the intended type(s).
@ -441,6 +441,24 @@ fn check_type<T: TypeChecker>(binding: &Binding, semantic: &SemanticModel) -> bo
_ => false,
},
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)
}
_ => false,
},
BindingKind::Argument => match binding.statement(semantic) {
// ```python
// def foo(x: annotation):
@ -565,35 +583,125 @@ impl BuiltinTypeChecker for TupleChecker {
const EXPR_TYPE: PythonType = PythonType::Tuple;
}
/// Test whether the given binding (and the given name) can be considered a list.
pub struct IoBaseChecker;
impl TypeChecker for IoBaseChecker {
fn match_annotation(annotation: &Expr, semantic: &SemanticModel) -> bool {
semantic
.resolve_call_path(annotation)
.is_some_and(|call_path| {
if semantic.match_typing_call_path(&call_path, "IO") {
return true;
}
if semantic.match_typing_call_path(&call_path, "BinaryIO") {
return true;
}
if semantic.match_typing_call_path(&call_path, "TextIO") {
return true;
}
matches!(
call_path.as_slice(),
[
"io",
"IOBase"
| "RawIOBase"
| "BufferedIOBase"
| "TextIOBase"
| "BytesIO"
| "StringIO"
| "BufferedReader"
| "BufferedWriter"
| "BufferedRandom"
| "BufferedRWPair"
| "TextIOWrapper"
] | ["os", "Path" | "PathLike"]
| [
"pathlib",
"Path" | "PurePath" | "PurePosixPath" | "PureWindowsPath"
]
)
})
}
fn match_initializer(initializer: &Expr, semantic: &SemanticModel) -> bool {
let Expr::Call(ast::ExprCall { func, .. }) = initializer else {
return false;
};
// Ex) `pathlib.Path("file.txt")`
if let Expr::Attribute(ast::ExprAttribute { value, attr, .. }) = func.as_ref() {
if attr.as_str() == "open" {
if let Expr::Call(ast::ExprCall { func, .. }) = value.as_ref() {
return semantic.resolve_call_path(func).is_some_and(|call_path| {
matches!(
call_path.as_slice(),
[
"pathlib",
"Path" | "PurePath" | "PurePosixPath" | "PureWindowsPath"
]
)
});
}
}
}
// Ex) `open("file.txt")`
semantic
.resolve_call_path(func.as_ref())
.is_some_and(|call_path| {
matches!(
call_path.as_slice(),
["io", "open" | "open_code"] | ["os" | "", "open"]
)
})
}
}
/// Test whether the given binding can be considered a list.
///
/// For this, we check what value might be associated with it through it's initialization and
/// what annotation it has (we consider `list` and `typing.List`).
pub fn is_list(binding: &Binding, semantic: &SemanticModel) -> bool {
check_type::<ListChecker>(binding, semantic)
}
/// Test whether the given binding (and the given name) can be considered a dictionary.
/// Test whether the given binding can be considered a dictionary.
///
/// For this, we check what value might be associated with it through it's initialization and
/// what annotation it has (we consider `dict` and `typing.Dict`).
pub fn is_dict(binding: &Binding, semantic: &SemanticModel) -> bool {
check_type::<DictChecker>(binding, semantic)
}
/// Test whether the given binding (and the given name) can be considered a set.
/// Test whether the given binding can be considered a set.
///
/// For this, we check what value might be associated with it through it's initialization and
/// what annotation it has (we consider `set` and `typing.Set`).
pub fn is_set(binding: &Binding, semantic: &SemanticModel) -> bool {
check_type::<SetChecker>(binding, semantic)
}
/// Test whether the given binding (and the given name) can be considered a
/// tuple. For this, we check what value might be associated with it through
/// Test whether the given binding can be considered a tuple.
///
/// For this, we check what value might be associated with it through
/// it's initialization and what annotation it has (we consider `tuple` and
/// `typing.Tuple`).
pub fn is_tuple(binding: &Binding, semantic: &SemanticModel) -> bool {
check_type::<TupleChecker>(binding, semantic)
}
/// Test whether the given binding can be considered a file-like object (i.e., a type that
/// implements `io.IOBase`).
pub fn is_io_base(binding: &Binding, semantic: &SemanticModel) -> bool {
check_type::<IoBaseChecker>(binding, semantic)
}
/// Test whether the given expression can be considered a file-like object (i.e., a type that
/// implements `io.IOBase`).
pub fn is_io_base_expr(expr: &Expr, semantic: &SemanticModel) -> bool {
IoBaseChecker::match_initializer(expr, semantic)
}
/// Find the [`ParameterWithDefault`] corresponding to the given [`Binding`].
#[inline]
fn find_parameter<'a>(
@ -699,6 +807,18 @@ pub fn find_binding_value<'a>(binding: &Binding, semantic: &'a SemanticModel) ->
_ => {}
}
}
// 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 {
return items.iter().find_map(|item| {
let target = item.optional_vars.as_ref()?;
let value = &item.context_expr;
match_value(binding, target, value)
});
}
}
_ => {}
}
None