Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't lint let_unit_value when needed for type inferenece #8563

Merged
merged 3 commits into from
Apr 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions clippy_lints/src/lib.register_all.rs
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
LintId::of(uninit_vec::UNINIT_VEC),
LintId::of(unit_hash::UNIT_HASH),
LintId::of(unit_return_expecting_ord::UNIT_RETURN_EXPECTING_ORD),
LintId::of(unit_types::LET_UNIT_VALUE),
LintId::of(unit_types::UNIT_ARG),
LintId::of(unit_types::UNIT_CMP),
LintId::of(unnamed_address::FN_ADDRESS_COMPARISONS),
Expand Down
1 change: 0 additions & 1 deletion clippy_lints/src/lib.register_pedantic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ store.register_group(true, "clippy::pedantic", Some("clippy_pedantic"), vec![
LintId::of(types::LINKEDLIST),
LintId::of(types::OPTION_OPTION),
LintId::of(unicode::UNICODE_NOT_NFC),
LintId::of(unit_types::LET_UNIT_VALUE),
LintId::of(unnecessary_wraps::UNNECESSARY_WRAPS),
LintId::of(unnested_or_patterns::UNNESTED_OR_PATTERNS),
LintId::of(unused_async::UNUSED_ASYNC),
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/lib.register_style.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ store.register_group(true, "clippy::style", Some("clippy_style"), vec![
LintId::of(single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS),
LintId::of(tabs_in_doc_comments::TABS_IN_DOC_COMMENTS),
LintId::of(to_digit_is_some::TO_DIGIT_IS_SOME),
LintId::of(unit_types::LET_UNIT_VALUE),
LintId::of(unnecessary_owned_empty_strings::UNNECESSARY_OWNED_EMPTY_STRINGS),
LintId::of(unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME),
LintId::of(unused_unit::UNUSED_UNIT),
Expand Down
80 changes: 75 additions & 5 deletions clippy_lints/src/unit_types/let_unit_value.rs
Original file line number Diff line number Diff line change
@@ -1,18 +1,46 @@
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::source::snippet_with_macro_callsite;
use clippy_utils::visitors::for_each_value_source;
use core::ops::ControlFlow;
use rustc_errors::Applicability;
use rustc_hir::{Stmt, StmtKind};
use rustc_hir::{Expr, ExprKind, PatKind, Stmt, StmtKind};
use rustc_lint::{LateContext, LintContext};
use rustc_middle::lint::in_external_macro;
use rustc_middle::ty::{self, Ty, TypeFoldable, TypeVisitor};

use super::LET_UNIT_VALUE;

pub(super) fn check(cx: &LateContext<'_>, stmt: &Stmt<'_>) {
if let StmtKind::Local(local) = stmt.kind {
if cx.typeck_results().pat_ty(local.pat).is_unit() {
if in_external_macro(cx.sess(), stmt.span) || local.pat.span.from_expansion() {
return;
if let StmtKind::Local(local) = stmt.kind
&& let Some(init) = local.init
&& !local.pat.span.from_expansion()
&& !in_external_macro(cx.sess(), stmt.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 !matches!(local.pat.kind, PatKind::Wild) {
span_lint_and_then(
cx,
LET_UNIT_VALUE,
stmt.span,
"this let-binding has unit value",
|diag| {
diag.span_suggestion(
local.pat.span,
"use a wild (`_`) binding",
"_".into(),
Applicability::MaybeIncorrect, // snippet
);
},
);
}
} else {
span_lint_and_then(
cx,
LET_UNIT_VALUE,
Expand All @@ -33,3 +61,45 @@ pub(super) fn check(cx: &LateContext<'_>, stmt: &Stmt<'_>) {
}
}
}

fn needs_inferred_result_ty(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
let id = match e.kind {
ExprKind::Call(
Expr {
kind: ExprKind::Path(ref path),
hir_id,
..
},
_,
) => cx.qpath_res(path, *hir_id).opt_def_id(),
ExprKind::MethodCall(..) => cx.typeck_results().type_dependent_def_id(e.hir_id),
_ => return false,
};
if let Some(id) = id
&& let sig = cx.tcx.fn_sig(id).skip_binder()
&& let ty::Param(output_ty) = *sig.output().kind()
{
sig.inputs().iter().all(|&ty| !ty_contains_param(ty, output_ty.index))
} 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()
}
2 changes: 1 addition & 1 deletion clippy_lints/src/unit_types/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ declare_clippy_lint! {
/// ```
#[clippy::version = "pre 1.29.0"]
pub LET_UNIT_VALUE,
pedantic,
style,
"creating a `let` binding to a value of unit type, which usually can't be used afterwards"
}

Expand Down
34 changes: 34 additions & 0 deletions clippy_utils/src/visitors.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use crate::path_to_local_id;
use core::ops::ControlFlow;
use rustc_hir as hir;
use rustc_hir::def::{DefKind, Res};
use rustc_hir::intravisit::{self, walk_block, walk_expr, Visitor};
Expand Down Expand Up @@ -402,3 +403,36 @@ pub fn contains_unsafe_block<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'tcx>)
v.visit_expr(e);
v.found_unsafe
}

/// Runs the given function for each sub-expression producing the final value consumed by the parent
/// of the give expression.
///
/// e.g. for the following expression
/// ```rust,ignore
/// if foo {
/// f(0)
/// } else {
/// 1 + 1
/// }
/// ```
/// this will pass both `f(0)` and `1+1` to the given function.
pub fn for_each_value_source<'tcx, B>(
e: &'tcx Expr<'tcx>,
f: &mut impl FnMut(&'tcx Expr<'tcx>) -> ControlFlow<B>,
) -> ControlFlow<B> {
match e.kind {
ExprKind::Block(Block { expr: Some(e), .. }, _) => for_each_value_source(e, f),
ExprKind::Match(_, arms, _) => {
for arm in arms {
for_each_value_source(arm.body, f)?;
}
ControlFlow::Continue(())
},
ExprKind::If(_, if_expr, Some(else_expr)) => {
for_each_value_source(if_expr, f)?;
for_each_value_source(else_expr, f)
},
ExprKind::DropTemps(e) => for_each_value_source(e, f),
_ => f(e),
}
}
2 changes: 1 addition & 1 deletion tests/ui-internal/interning_defined_symbol.fixed
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// run-rustfix
#![deny(clippy::internal)]
#![allow(clippy::missing_clippy_version_attribute)]
#![allow(clippy::missing_clippy_version_attribute, clippy::let_unit_value)]
#![feature(rustc_private)]

extern crate rustc_span;
Expand Down
2 changes: 1 addition & 1 deletion tests/ui-internal/interning_defined_symbol.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// run-rustfix
#![deny(clippy::internal)]
#![allow(clippy::missing_clippy_version_attribute)]
#![allow(clippy::missing_clippy_version_attribute, clippy::let_unit_value)]
#![feature(rustc_private)]

extern crate rustc_span;
Expand Down
1 change: 1 addition & 0 deletions tests/ui-toml/functions_maxlines/test.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#![warn(clippy::too_many_lines)]
#![allow(clippy::let_unit_value)]

// This function should be considered one line.
fn many_comments_but_one_line_of_code() {
Expand Down
8 changes: 4 additions & 4 deletions tests/ui-toml/functions_maxlines/test.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: this function has too many lines (2/1)
--> $DIR/test.rs:18:1
--> $DIR/test.rs:19:1
|
LL | / fn too_many_lines() {
LL | | println!("This is bad.");
Expand All @@ -10,7 +10,7 @@ LL | | }
= note: `-D clippy::too-many-lines` implied by `-D warnings`

error: this function has too many lines (4/1)
--> $DIR/test.rs:24:1
--> $DIR/test.rs:25:1
|
LL | / async fn async_too_many_lines() {
LL | | println!("This is bad.");
Expand All @@ -19,7 +19,7 @@ LL | | }
| |_^

error: this function has too many lines (4/1)
--> $DIR/test.rs:30:1
--> $DIR/test.rs:31:1
|
LL | / fn closure_too_many_lines() {
LL | | let _ = {
Expand All @@ -30,7 +30,7 @@ LL | | }
| |_^

error: this function has too many lines (2/1)
--> $DIR/test.rs:52:1
--> $DIR/test.rs:53:1
|
LL | / fn comment_before_code() {
LL | | let _ = "test";
Expand Down
6 changes: 3 additions & 3 deletions tests/ui/cast_alignment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ fn main() {
let _ = core::ptr::read_unaligned(ptr as *const u16);
let _ = core::intrinsics::unaligned_volatile_load(ptr as *const u16);
let ptr = &mut data as *mut [u8; 2] as *mut u8;
let _ = (ptr as *mut u16).write_unaligned(0);
let _ = core::ptr::write_unaligned(ptr as *mut u16, 0);
let _ = core::intrinsics::unaligned_volatile_store(ptr as *mut u16, 0);
(ptr as *mut u16).write_unaligned(0);
core::ptr::write_unaligned(ptr as *mut u16, 0);
core::intrinsics::unaligned_volatile_store(ptr as *mut u16, 0);
}
}
2 changes: 2 additions & 0 deletions tests/ui/cast_slice_different_sizes.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#![allow(clippy::let_unit_value)]

fn main() {
let x: [i32; 3] = [1_i32, 2, 3];
let r_x = &x;
Expand Down
12 changes: 6 additions & 6 deletions tests/ui/cast_slice_different_sizes.stderr
Original file line number Diff line number Diff line change
@@ -1,31 +1,31 @@
error: casting between raw pointers to `[i32]` (element size 4) and `[u8]` (element size 1) does not adjust the count
--> $DIR/cast_slice_different_sizes.rs:7:13
--> $DIR/cast_slice_different_sizes.rs:9:13
|
LL | let b = a as *const [u8];
| ^^^^^^^^^^^^^^^^ help: replace with `ptr::slice_from_raw_parts`: `core::ptr::slice_from_raw_parts(a as *const u8, ..)`
|
= note: `#[deny(clippy::cast_slice_different_sizes)]` on by default

error: casting between raw pointers to `[u8]` (element size 1) and `[u32]` (element size 4) does not adjust the count
--> $DIR/cast_slice_different_sizes.rs:8:13
--> $DIR/cast_slice_different_sizes.rs:10:13
|
LL | let c = b as *const [u32];
| ^^^^^^^^^^^^^^^^^ help: replace with `ptr::slice_from_raw_parts`: `core::ptr::slice_from_raw_parts(b as *const u32, ..)`

error: casting between raw pointers to `[i32]` (element size 4) and `[u8]` (element size 1) does not adjust the count
--> $DIR/cast_slice_different_sizes.rs:11:16
--> $DIR/cast_slice_different_sizes.rs:13:16
|
LL | let loss = r_x as *const [i32] as *const [u8];
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with `ptr::slice_from_raw_parts`: `core::ptr::slice_from_raw_parts(r_x as *const [i32] as *const u8, ..)`

error: casting between raw pointers to `[i32]` (element size 4) and `[u8]` (element size 1) does not adjust the count
--> $DIR/cast_slice_different_sizes.rs:18:24
--> $DIR/cast_slice_different_sizes.rs:20:24
|
LL | let loss_block_1 = { r_x as *const [i32] } as *const [u8];
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with `ptr::slice_from_raw_parts`: `core::ptr::slice_from_raw_parts({ r_x as *const [i32] } as *const u8, ..)`

error: casting between raw pointers to `[i32]` (element size 4) and `[u8]` (element size 1) does not adjust the count
--> $DIR/cast_slice_different_sizes.rs:19:24
--> $DIR/cast_slice_different_sizes.rs:21:24
|
LL | let loss_block_2 = {
| ________________________^
Expand All @@ -43,7 +43,7 @@ LL ~ } as *const u8, ..);
|

error: casting between raw pointers to `[i32]` (element size 4) and `[u8]` (element size 1) does not adjust the count
--> $DIR/cast_slice_different_sizes.rs:36:27
--> $DIR/cast_slice_different_sizes.rs:38:27
|
LL | let long_chain_loss = r_x as *const [i32] as *const [u32] as *const [u16] as *const [i8] as *const [u8];
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with `ptr::slice_from_raw_parts`: `core::ptr::slice_from_raw_parts(r_x as *const [i32] as *const [u32] as *const [u16] as *const [i8] as *const u8, ..)`
Expand Down
1 change: 1 addition & 0 deletions tests/ui/crashes/ice-5944.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#![warn(clippy::repeat_once)]
#![allow(clippy::let_unit_value)]

trait Repeat {
fn repeat(&self) {}
Expand Down
15 changes: 9 additions & 6 deletions tests/ui/default_numeric_fallback_f64.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,15 @@
// aux-build:macro_rules.rs

#![warn(clippy::default_numeric_fallback)]
#![allow(unused)]
#![allow(clippy::never_loop)]
#![allow(clippy::no_effect)]
#![allow(clippy::unnecessary_operation)]
#![allow(clippy::branches_sharing_code)]
#![allow(clippy::match_single_binding)]
#![allow(
unused,
clippy::never_loop,
clippy::no_effect,
clippy::unnecessary_operation,
clippy::branches_sharing_code,
clippy::match_single_binding,
clippy::let_unit_value
)]

#[macro_use]
extern crate macro_rules;
Expand Down
15 changes: 9 additions & 6 deletions tests/ui/default_numeric_fallback_f64.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,15 @@
// aux-build:macro_rules.rs

#![warn(clippy::default_numeric_fallback)]
#![allow(unused)]
#![allow(clippy::never_loop)]
#![allow(clippy::no_effect)]
#![allow(clippy::unnecessary_operation)]
#![allow(clippy::branches_sharing_code)]
#![allow(clippy::match_single_binding)]
#![allow(
unused,
clippy::never_loop,
clippy::no_effect,
clippy::unnecessary_operation,
clippy::branches_sharing_code,
clippy::match_single_binding,
clippy::let_unit_value
)]

#[macro_use]
extern crate macro_rules;
Expand Down
Loading