nordic-dev.net/rust
nordic-dev.net/rust
2
0
Fork 0
mirror of https://github.com/rust-lang/rust.git synced 2025-02-22 03:44:24 +00:00
Code Issues Packages Projects Releases Wiki Activity

Changes to let_unit_value

Browse Source 
* View through locals in `let_unit_value` when determining if inference is required
* Don't remove typed let bindings for more functions
This commit is contained in:
Jason Newcomb 2022-06-17 22:21:42 -04:00
parent 54feac18d1
commit 196174ddad
6 changed files with 266 additions and 73 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

127
clippy_lints/src/unit_types/let_unit_value.rs
View File

@ -1,29 +1,24 @@
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::get_parent_node;
use clippy_utils::source::snippet_with_macro_callsite;
use clippy_utils::visitors::for_each_value_source;
use clippy_utils::visitors::{for_each_local_assignment, for_each_value_source};
use core::ops::ControlFlow;
use rustc_errors::Applicability;
use rustc_hir::def::{DefKind, Res};
use rustc_hir::{Expr, ExprKind, Local, PatKind};
use rustc_hir::{Expr, ExprKind, HirId, HirIdSet, Local, Node, PatKind, QPath, TyKind};
use rustc_lint::{LateContext, LintContext};
use rustc_middle::lint::in_external_macro;
use rustc_middle::ty::{self, Ty, TypeFoldable, TypeSuperFoldable, TypeVisitor};
use rustc_middle::ty;
use super::LET_UNIT_VALUE;
pub(super) fn check(cx: &LateContext<'_>, local: &Local<'_>) {
pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, local: &'tcx Local<'_>) {
if let Some(init) = local.init
&& !local.pat.span.from_expansion()
&& !in_external_macro(cx.sess(), local.span)
&& cx.typeck_results().pat_ty(local.pat).is_unit()
{
let needs_inferred = for_each_value_source(init, &mut |e| if needs_inferred_result_ty(cx, e) {
ControlFlow::Continue(())
} else {
ControlFlow::Break(())
}).is_continue();
if needs_inferred {
if local.ty.is_some() && expr_needs_inferred_result(cx, init) {
if !matches!(local.pat.kind, PatKind::Wild) {
span_lint_and_then(
cx,
@ -62,48 +57,106 @@ pub(super) fn check(cx: &LateContext<'_>, local: &Local<'_>) {
}
}
fn needs_inferred_result_ty(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
let id = match e.kind {
/// Checks sub-expressions which create the value returned by the given expression for whether
/// return value inference is needed. This checks through locals to see if they also need inference
/// at this point.
///
/// e.g.
/// ```rust,ignore
/// let bar = foo();
/// let x: u32 = if true { baz() } else { bar };
/// ```
/// Here the sources of the value assigned to `x` would be `baz()`, and `foo()` via the
/// initialization of `bar`. If both `foo` and `baz` have a return type which require type
/// inference then this function would return `true`.
fn expr_needs_inferred_result<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> bool {
// The locals used for initialization which have yet to be checked.
let mut locals_to_check = Vec::new();
// All the locals which have been added to `locals_to_check`. Needed to prevent cycles.
let mut seen_locals = HirIdSet::default();
if !each_value_source_needs_inference(cx, e, &mut locals_to_check, &mut seen_locals) {
return false;
}
while let Some(id) = locals_to_check.pop() {
if let Some(Node::Local(l)) = get_parent_node(cx.tcx, id) {
if !l.ty.map_or(true, |ty| matches!(ty.kind, TyKind::Infer)) {
return false;
}
if let Some(e) = l.init {
if !each_value_source_needs_inference(cx, e, &mut locals_to_check, &mut seen_locals) {
return false;
}
} else if for_each_local_assignment(cx, id, |e| {
if each_value_source_needs_inference(cx, e, &mut locals_to_check, &mut seen_locals) {
ControlFlow::Continue(())
} else {
ControlFlow::Break(())
}
})
.is_break()
{
return false;
}
}
}
true
}
fn each_value_source_needs_inference(
cx: &LateContext<'_>,
e: &Expr<'_>,
locals_to_check: &mut Vec<HirId>,
seen_locals: &mut HirIdSet,
) -> bool {
for_each_value_source(e, &mut |e| {
if needs_inferred_result_ty(cx, e, locals_to_check, seen_locals) {
ControlFlow::Continue(())
} else {
ControlFlow::Break(())
}
})
.is_continue()
}
fn needs_inferred_result_ty(
cx: &LateContext<'_>,
e: &Expr<'_>,
locals_to_check: &mut Vec<HirId>,
seen_locals: &mut HirIdSet,
) -> bool {
let (id, args) = match e.kind {
ExprKind::Call(
Expr {
kind: ExprKind::Path(ref path),
hir_id,
..
},
_,
args,
) => match cx.qpath_res(path, *hir_id) {
Res::Def(DefKind::AssocFn | DefKind::Fn, id) => id,
Res::Def(DefKind::AssocFn | DefKind::Fn, id) => (id, args),
_ => return false,
},
ExprKind::MethodCall(..) => match cx.typeck_results().type_dependent_def_id(e.hir_id) {
Some(id) => id,
ExprKind::MethodCall(_, args, _) => match cx.typeck_results().type_dependent_def_id(e.hir_id) {
Some(id) => (id, args),
None => return false,
},
ExprKind::Path(QPath::Resolved(None, path)) => {
if let Res::Local(id) = path.res
&& seen_locals.insert(id)
{
locals_to_check.push(id);
}
return true;
},
_ => return false,
};
let sig = cx.tcx.fn_sig(id).skip_binder();
if let ty::Param(output_ty) = *sig.output().kind() {
sig.inputs().iter().all(|&ty| !ty_contains_param(ty, output_ty.index))
sig.inputs().iter().zip(args).all(|(&ty, arg)| {
!ty.is_param(output_ty.index) || each_value_source_needs_inference(cx, arg, locals_to_check, seen_locals)
})
} else {
false
}
}
fn ty_contains_param(ty: Ty<'_>, index: u32) -> bool {
struct Visitor(u32);
impl<'tcx> TypeVisitor<'tcx> for Visitor {
type BreakTy = ();
fn visit_ty(&mut self, ty: Ty<'tcx>) -> ControlFlow<Self::BreakTy> {
if let ty::Param(ty) = *ty.kind() {
if ty.index == self.0 {
ControlFlow::BREAK
} else {
ControlFlow::CONTINUE
}
} else {
ty.super_visit_with(self)
}
}
}
ty.visit_with(&mut Visitor(index)).is_break()
}

4
clippy_lints/src/unit_types/mod.rs
View File

@ -98,8 +98,8 @@ declare_clippy_lint! {
declare_lint_pass!(UnitTypes => [LET_UNIT_VALUE, UNIT_CMP, UNIT_ARG]);
impl LateLintPass<'_> for UnitTypes {
fn check_local(&mut self, cx: &LateContext<'_>, local: &Local<'_>) {
impl<'tcx> LateLintPass<'tcx> for UnitTypes {
fn check_local(&mut self, cx: &LateContext<'tcx>, local: &'tcx Local<'tcx>) {
let_unit_value::check(cx, local);
}

46
clippy_utils/src/visitors.rs
View File

@ -617,3 +617,49 @@ pub fn any_temporaries_need_ordered_drop<'tcx>(cx: &LateContext<'tcx>, e: &'tcx
})
.is_break()
}
/// Runs the given function for each path expression referencing the given local which occur after
/// the given expression.
pub fn for_each_local_assignment<'tcx, B>(
cx: &LateContext<'tcx>,
local_id: HirId,
f: impl FnMut(&'tcx Expr<'tcx>) -> ControlFlow<B>,
) -> ControlFlow<B> {
struct V<'cx, 'tcx, F, B> {
cx: &'cx LateContext<'tcx>,
local_id: HirId,
res: ControlFlow<B>,
f: F,
}
impl<'cx, 'tcx, F: FnMut(&'tcx Expr<'tcx>) -> ControlFlow<B>, B> Visitor<'tcx> for V<'cx, 'tcx, F, B> {
type NestedFilter = nested_filter::OnlyBodies;
fn nested_visit_map(&mut self) -> Self::Map {
self.cx.tcx.hir()
}
fn visit_expr(&mut self, e: &'tcx Expr<'tcx>) {
if let ExprKind::Assign(lhs, rhs, _) = e.kind
&& self.res.is_continue()
&& path_to_local_id(lhs, self.local_id)
{
self.res = (self.f)(rhs);
self.visit_expr(rhs);
} else {
walk_expr(self, e);
}
}
}
if let Some(b) = get_enclosing_block(cx, local_id) {
let mut v = V {
cx,
local_id,
res: ControlFlow::Continue(()),
f,
};
v.visit_block(b);
v.res
} else {
ControlFlow::Continue(())
}
}

62
tests/ui/let_unit.fixed
View File

@ -2,8 +2,7 @@
#![feature(lint_reasons)]
#![warn(clippy::let_unit_value)]
#![allow(clippy::no_effect)]
#![allow(unused)]
#![allow(unused, clippy::no_effect, clippy::needless_late_init, path_statements)]
macro_rules! let_and_return {
($n:expr) => {{
@ -73,8 +72,8 @@ fn _returns_generic() {
fn f3<T>(x: T) -> T {
x
}
fn f4<T>(mut x: Vec<T>) -> T {
x.pop().unwrap()
fn f5<T: Default>(x: bool) -> Option<T> {
x.then(|| T::default())
}
let _: () = f(); // Ok
@ -86,8 +85,12 @@ fn _returns_generic() {
f3(()); // Lint
f3(()); // Lint
f4(vec![()]); // Lint
f4(vec![()]); // Lint
// Should lint:
// fn f4<T>(mut x: Vec<T>) -> T {
// x.pop().unwrap()
// }
// let _: () = f4(vec![()]);
// let x: () = f4(vec![()]);
// Ok
let _: () = {
@ -113,6 +116,53 @@ fn _returns_generic() {
Some(1) => f2(3),
Some(_) => (),
};
let _: () = f5(true).unwrap();
#[allow(clippy::let_unit_value)]
{
let x = f();
let y;
let z;
match 0 {
0 => {
y = f();
z = f();
},
1 => {
println!("test");
y = f();
z = f3(());
},
_ => panic!(),
}
let x1;
let x2;
if true {
x1 = f();
x2 = x1;
} else {
x2 = f();
x1 = x2;
}
let opt;
match f5(true) {
Some(x) => opt = x,
None => panic!(),
};
#[warn(clippy::let_unit_value)]
{
let _: () = x;
let _: () = y;
z;
let _: () = x1;
let _: () = x2;
let _: () = opt;
}
}
}
fn attributes() {

62
tests/ui/let_unit.rs
View File

@ -2,8 +2,7 @@
#![feature(lint_reasons)]
#![warn(clippy::let_unit_value)]
#![allow(clippy::no_effect)]
#![allow(unused)]
#![allow(unused, clippy::no_effect, clippy::needless_late_init, path_statements)]
macro_rules! let_and_return {
($n:expr) => {{
@ -73,8 +72,8 @@ fn _returns_generic() {
fn f3<T>(x: T) -> T {
x
}
fn f4<T>(mut x: Vec<T>) -> T {
x.pop().unwrap()
fn f5<T: Default>(x: bool) -> Option<T> {
x.then(|| T::default())
}
let _: () = f(); // Ok
@ -86,8 +85,12 @@ fn _returns_generic() {
let _: () = f3(()); // Lint
let x: () = f3(()); // Lint
let _: () = f4(vec![()]); // Lint
let x: () = f4(vec![()]); // Lint
// Should lint:
// fn f4<T>(mut x: Vec<T>) -> T {
// x.pop().unwrap()
// }
// let _: () = f4(vec![()]);
// let x: () = f4(vec![()]);
// Ok
let _: () = {
@ -113,6 +116,53 @@ fn _returns_generic() {
Some(1) => f2(3),
Some(_) => (),
};
let _: () = f5(true).unwrap();
#[allow(clippy::let_unit_value)]
{
let x = f();
let y;
let z;
match 0 {
0 => {
y = f();
z = f();
},
1 => {
println!("test");
y = f();
z = f3(());
},
_ => panic!(),
}
let x1;
let x2;
if true {
x1 = f();
x2 = x1;
} else {
x2 = f();
x1 = x2;
}
let opt;
match f5(true) {
Some(x) => opt = x,
None => panic!(),
};
#[warn(clippy::let_unit_value)]
{
let _: () = x;
let _: () = y;
let _: () = z;
let _: () = x1;
let _: () = x2;
let _: () = opt;
}
}
}
fn attributes() {

38
tests/ui/let_unit.stderr
View File

@ -1,5 +1,5 @@
error: this let-binding has unit value
--> $DIR/let_unit.rs:15:5
--> $DIR/let_unit.rs:14:5
|
LL | let _x = println!("x");
| ^^^^^^^^^^^^^^^^^^^^^^^ help: omit the `let` binding: `println!("x");`
@ -7,13 +7,13 @@ LL | let _x = println!("x");
= note: `-D clippy::let-unit-value` implied by `-D warnings`
error: this let-binding has unit value
--> $DIR/let_unit.rs:19:9
--> $DIR/let_unit.rs:18:9
|
LL | let _a = ();
| ^^^^^^^^^^^^ help: omit the `let` binding: `();`
error: this let-binding has unit value
--> $DIR/let_unit.rs:54:5
--> $DIR/let_unit.rs:53:5
|
LL | / let _ = v
LL | | .into_iter()
@ -36,7 +36,7 @@ LL + .unwrap();
|
error: this let-binding has unit value
--> $DIR/let_unit.rs:81:5
--> $DIR/let_unit.rs:80:5
|
LL | let x: () = f(); // Lint.
| ^^^^-^^^^^^^^^^^
@ -44,7 +44,7 @@ LL | let x: () = f(); // Lint.
| help: use a wild (`_`) binding: `_`
error: this let-binding has unit value
--> $DIR/let_unit.rs:84:5
--> $DIR/let_unit.rs:83:5
|
LL | let x: () = f2(0i32); // Lint.
| ^^^^-^^^^^^^^^^^^^^^^
@ -52,31 +52,19 @@ LL | let x: () = f2(0i32); // Lint.
| help: use a wild (`_`) binding: `_`
error: this let-binding has unit value
--> $DIR/let_unit.rs:86:5
--> $DIR/let_unit.rs:85:5
|
LL | let _: () = f3(()); // Lint
| ^^^^^^^^^^^^^^^^^^^ help: omit the `let` binding: `f3(());`
error: this let-binding has unit value
--> $DIR/let_unit.rs:87:5
--> $DIR/let_unit.rs:86:5
|
LL | let x: () = f3(()); // Lint
| ^^^^^^^^^^^^^^^^^^^ help: omit the `let` binding: `f3(());`
error: this let-binding has unit value
--> $DIR/let_unit.rs:89:5
|
LL | let _: () = f4(vec![()]); // Lint
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: omit the `let` binding: `f4(vec![()]);`
error: this let-binding has unit value
--> $DIR/let_unit.rs:90:5
|
LL | let x: () = f4(vec![()]); // Lint
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: omit the `let` binding: `f4(vec![()]);`
error: this let-binding has unit value
--> $DIR/let_unit.rs:99:5
--> $DIR/let_unit.rs:102:5
|
LL | let x: () = if true { f() } else { f2(0) }; // Lint
| ^^^^-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@ -84,7 +72,7 @@ LL | let x: () = if true { f() } else { f2(0) }; // Lint
| help: use a wild (`_`) binding: `_`
error: this let-binding has unit value
--> $DIR/let_unit.rs:110:5
--> $DIR/let_unit.rs:113:5
|
LL | / let _: () = match Some(0) {
LL | | None => f2(1),
@ -104,5 +92,11 @@ LL + Some(_) => (),
LL + };
|
error: aborting due to 11 previous errors
error: this let-binding has unit value
--> $DIR/let_unit.rs:160:13
|
LL | let _: () = z;
| ^^^^^^^^^^^^^^ help: omit the `let` binding: `z;`
error: aborting due to 10 previous errors