mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Auto merge of #11387 - y21:issue11371, r=blyxyas

Browse Source 
[`unnecessary_unwrap`]: lint on `.as_ref().unwrap()`

Closes #11371

This turned out to be a little more code than I originally thought, because the lint also makes sure to not lint if the user tries to mutate the option:
```rs
if option.is_some() {
  option = None;
  option.unwrap(); // don't lint here
}
```
... which means that even if we taught this lint to recognize `.as_mut()`, it would *still* not lint because that would count as a mutation. So we need to allow `.as_mut()` calls but reject other kinds of mutations.
Unfortunately it doesn't look like this is possible with `is_potentially_mutated` (seeing what kind of mutation happened).
This replaces it with a custom little visitor that does basically what it did before, but also allows `.as_mut()`.

changelog: [`unnecessary_unwrap`]: lint on `.as_ref().unwrap()`
This commit is contained in:
bors 2023-08-28 20:29:42 +00:00
commit b97eaab558
4 changed files with 235 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

111
clippy_lints/src/unwrap.rs
View File

@ -1,15 +1,18 @@
use clippy_utils::diagnostics::span_lint_hir_and_then;
use clippy_utils::ty::is_type_diagnostic_item;
use clippy_utils::usage::is_potentially_mutated;
use clippy_utils::usage::is_potentially_local_place;
use clippy_utils::{higher, path_to_local};
use if_chain::if_chain;
use rustc_errors::Applicability;
use rustc_hir::intravisit::{walk_expr, walk_fn, FnKind, Visitor};
use rustc_hir::{BinOpKind, Body, Expr, ExprKind, FnDecl, HirId, PathSegment, UnOp};
use rustc_hir::{BinOpKind, Body, Expr, ExprKind, FnDecl, HirId, Node, PathSegment, UnOp};
use rustc_hir_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, PlaceWithHirId};
use rustc_infer::infer::TyCtxtInferExt;
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::hir::nested_filter;
use rustc_middle::lint::in_external_macro;
use rustc_middle::ty::Ty;
use rustc_middle::mir::FakeReadCause;
use rustc_middle::ty::{self, Ty, TyCtxt};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::def_id::LocalDefId;
use rustc_span::source_map::Span;
@ -192,6 +195,55 @@ fn collect_unwrap_info<'tcx>(
Vec::new()
}
/// A HIR visitor delegate that checks if a local variable of type `Option<_>` is mutated,
/// *except* for if `Option::as_mut` is called.
/// The reason for why we allow that one specifically is that `.as_mut()` cannot change
/// the option to `None`, and that is important because this lint relies on the fact that
/// `is_some` + `unwrap` is equivalent to `if let Some(..) = ..`, which it would not be if
/// the option is changed to None between `is_some` and `unwrap`.
/// (And also `.as_mut()` is a somewhat common method that is still worth linting on.)
struct MutationVisitor<'tcx> {
is_mutated: bool,
local_id: HirId,
tcx: TyCtxt<'tcx>,
}
/// Checks if the parent of the expression pointed at by the given `HirId` is a call to
/// `Option::as_mut`.
///
/// Used by the mutation visitor to specifically allow `.as_mut()` calls.
/// In particular, the `HirId` that the visitor receives is the id of the local expression
/// (i.e. the `x` in `x.as_mut()`), and that is the reason for why we care about its parent
/// expression: that will be where the actual method call is.
fn is_option_as_mut_use(tcx: TyCtxt<'_>, expr_id: HirId) -> bool {
if let Node::Expr(mutating_expr) = tcx.hir().get_parent(expr_id)
&& let ExprKind::MethodCall(path, ..) = mutating_expr.kind
{
path.ident.name.as_str() == "as_mut"
} else {
false
}
}
impl<'tcx> Delegate<'tcx> for MutationVisitor<'tcx> {
fn borrow(&mut self, cat: &PlaceWithHirId<'tcx>, diag_expr_id: HirId, bk: ty::BorrowKind) {
if let ty::BorrowKind::MutBorrow = bk
&& is_potentially_local_place(self.local_id, &cat.place)
&& !is_option_as_mut_use(self.tcx, diag_expr_id)
{
self.is_mutated = true;
}
}
fn mutate(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {
self.is_mutated = true;
}
fn consume(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {}
fn fake_read(&mut self, _: &PlaceWithHirId<'tcx>, _: FakeReadCause, _: HirId) {}
}
impl<'a, 'tcx> UnwrappableVariablesVisitor<'a, 'tcx> {
fn visit_branch(
&mut self,
@ -202,10 +254,26 @@ impl<'a, 'tcx> UnwrappableVariablesVisitor<'a, 'tcx> {
) {
let prev_len = self.unwrappables.len();
for unwrap_info in collect_unwrap_info(self.cx, if_expr, cond, branch, else_branch, true) {
if is_potentially_mutated(unwrap_info.local_id, cond, self.cx)
|| is_potentially_mutated(unwrap_info.local_id, branch, self.cx)
{
// if the variable is mutated, we don't know whether it can be unwrapped:
let mut delegate = MutationVisitor {
tcx: self.cx.tcx,
is_mutated: false,
local_id: unwrap_info.local_id,
};
let infcx = self.cx.tcx.infer_ctxt().build();
let mut vis = ExprUseVisitor::new(
&mut delegate,
&infcx,
cond.hir_id.owner.def_id,
self.cx.param_env,
self.cx.typeck_results(),
);
vis.walk_expr(cond);
vis.walk_expr(branch);
if delegate.is_mutated {
// if the variable is mutated, we don't know whether it can be unwrapped.
// it might have been changed to `None` in between `is_some` + `unwrap`.
continue;
}
self.unwrappables.push(unwrap_info);
@ -215,6 +283,27 @@ impl<'a, 'tcx> UnwrappableVariablesVisitor<'a, 'tcx> {
}
}
enum AsRefKind {
AsRef,
AsMut,
}
/// Checks if the expression is a method call to `as_{ref,mut}` and returns the receiver of it.
/// If it isn't, the expression itself is returned.
fn consume_option_as_ref<'tcx>(expr: &'tcx Expr<'tcx>) -> (&'tcx Expr<'tcx>, Option<AsRefKind>) {
if let ExprKind::MethodCall(path, recv, ..) = expr.kind {
if path.ident.name == sym::as_ref {
(recv, Some(AsRefKind::AsRef))
} else if path.ident.name.as_str() == "as_mut" {
(recv, Some(AsRefKind::AsMut))
} else {
(expr, None)
}
} else {
(expr, None)
}
}
impl<'a, 'tcx> Visitor<'tcx> for UnwrappableVariablesVisitor<'a, 'tcx> {
type NestedFilter = nested_filter::OnlyBodies;
@ -233,6 +322,7 @@ impl<'a, 'tcx> Visitor<'tcx> for UnwrappableVariablesVisitor<'a, 'tcx> {
// find `unwrap[_err]()` calls:
if_chain! {
if let ExprKind::MethodCall(method_name, self_arg, ..) = expr.kind;
let (self_arg, as_ref_kind) = consume_option_as_ref(self_arg);
if let Some(id) = path_to_local(self_arg);
if [sym::unwrap, sym::expect, sym!(unwrap_err)].contains(&method_name.ident.name);
let call_to_unwrap = [sym::unwrap, sym::expect].contains(&method_name.ident.name);
@ -268,7 +358,12 @@ impl<'a, 'tcx> Visitor<'tcx> for UnwrappableVariablesVisitor<'a, 'tcx> {
unwrappable.check.span.with_lo(unwrappable.if_expr.span.lo()),
"try",
format!(
"if let {suggested_pattern} = {unwrappable_variable_name}",
"if let {suggested_pattern} = {borrow_prefix}{unwrappable_variable_name}",
borrow_prefix = match as_ref_kind {
Some(AsRefKind::AsRef) => "&",
Some(AsRefKind::AsMut) => "&mut ",
None => "",
},
),
// We don't track how the unwrapped value is used inside the
// block or suggest deleting the unwrap, so we can't offer a

13
clippy_utils/src/usage.rs
View File

@ -4,7 +4,7 @@ use core::ops::ControlFlow;
use hir::def::Res;
use rustc_hir::intravisit::{self, Visitor};
use rustc_hir::{self as hir, Expr, ExprKind, HirId, HirIdSet};
use rustc_hir_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId};
use rustc_hir_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, Place, PlaceBase, PlaceWithHirId};
use rustc_infer::infer::TyCtxtInferExt;
use rustc_lint::LateContext;
use rustc_middle::hir::nested_filter;
@ -37,6 +37,17 @@ pub fn is_potentially_mutated<'tcx>(variable: HirId, expr: &'tcx Expr<'_>, cx: &
mutated_variables(expr, cx).map_or(true, |mutated| mutated.contains(&variable))
}
pub fn is_potentially_local_place(local_id: HirId, place: &Place<'_>) -> bool {
match place.base {
PlaceBase::Local(id) => id == local_id,
PlaceBase::Upvar(_) => {
// Conservatively assume yes.
true
},
_ => false,
}
}
struct MutVarsDelegate {
used_mutably: HirIdSet,
skip: bool,

51
tests/ui/checked_unwrap/simple_conditionals.rs
View File

@ -128,6 +128,57 @@ fn main() {
assert!(x.is_ok(), "{:?}", x.unwrap_err());
}
fn issue11371() {
let option = Some(());
if option.is_some() {
option.as_ref().unwrap();
//~^ ERROR: called `unwrap` on `option` after checking its variant with `is_some`
} else {
option.as_ref().unwrap();
//~^ ERROR: this call to `unwrap()` will always panic
}
let result = Ok::<(), ()>(());
if result.is_ok() {
result.as_ref().unwrap();
//~^ ERROR: called `unwrap` on `result` after checking its variant with `is_ok`
} else {
result.as_ref().unwrap();
//~^ ERROR: this call to `unwrap()` will always panic
}
let mut option = Some(());
if option.is_some() {
option.as_mut().unwrap();
//~^ ERROR: called `unwrap` on `option` after checking its variant with `is_some`
} else {
option.as_mut().unwrap();
//~^ ERROR: this call to `unwrap()` will always panic
}
let mut result = Ok::<(), ()>(());
if result.is_ok() {
result.as_mut().unwrap();
//~^ ERROR: called `unwrap` on `result` after checking its variant with `is_ok`
} else {
result.as_mut().unwrap();
//~^ ERROR: this call to `unwrap()` will always panic
}
// This should not lint. Statics are, at the time of writing, not linted on anyway,
// but if at some point they are supported by this lint, it should correctly see that
// `X` is being mutated and not suggest `if let Some(..) = X {}`
static mut X: Option<i32> = Some(123);
unsafe {
if X.is_some() {
X = None;
X.unwrap();
}
}
}
fn check_expect() {
let x = Some(());
if x.is_some() {

70
tests/ui/checked_unwrap/simple_conditionals.stderr
View File

@ -168,5 +168,73 @@ LL | if x.is_err() {
LL | x.unwrap_err();
| ^^^^^^^^^^^^^^
error: aborting due to 17 previous errors
error: called `unwrap` on `option` after checking its variant with `is_some`
--> $DIR/simple_conditionals.rs:135:9
|
LL | if option.is_some() {
| ------------------- help: try: `if let Some(..) = &option`
LL | option.as_ref().unwrap();
| ^^^^^^^^^^^^^^^^^^^^^^^^
error: this call to `unwrap()` will always panic
--> $DIR/simple_conditionals.rs:138:9
|
LL | if option.is_some() {
| ---------------- because of this check
...
LL | option.as_ref().unwrap();
| ^^^^^^^^^^^^^^^^^^^^^^^^
error: called `unwrap` on `result` after checking its variant with `is_ok`
--> $DIR/simple_conditionals.rs:145:9
|
LL | if result.is_ok() {
| ----------------- help: try: `if let Ok(..) = &result`
LL | result.as_ref().unwrap();
| ^^^^^^^^^^^^^^^^^^^^^^^^
error: this call to `unwrap()` will always panic
--> $DIR/simple_conditionals.rs:148:9
|
LL | if result.is_ok() {
| -------------- because of this check
...
LL | result.as_ref().unwrap();
| ^^^^^^^^^^^^^^^^^^^^^^^^
error: called `unwrap` on `option` after checking its variant with `is_some`
--> $DIR/simple_conditionals.rs:154:9
|
LL | if option.is_some() {
| ------------------- help: try: `if let Some(..) = &mut option`
LL | option.as_mut().unwrap();
| ^^^^^^^^^^^^^^^^^^^^^^^^
error: this call to `unwrap()` will always panic
--> $DIR/simple_conditionals.rs:157:9
|
LL | if option.is_some() {
| ---------------- because of this check
...
LL | option.as_mut().unwrap();
| ^^^^^^^^^^^^^^^^^^^^^^^^
error: called `unwrap` on `result` after checking its variant with `is_ok`
--> $DIR/simple_conditionals.rs:163:9
|
LL | if result.is_ok() {
| ----------------- help: try: `if let Ok(..) = &mut result`
LL | result.as_mut().unwrap();
| ^^^^^^^^^^^^^^^^^^^^^^^^
error: this call to `unwrap()` will always panic
--> $DIR/simple_conditionals.rs:166:9
|
LL | if result.is_ok() {
| -------------- because of this check
...
LL | result.as_mut().unwrap();
| ^^^^^^^^^^^^^^^^^^^^^^^^
error: aborting due to 25 previous errors