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

Add allow-by-default lint for unit bindings #112380

Merged
merged 1 commit into from
Nov 22, 2023
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
3 changes: 3 additions & 0 deletions compiler/rustc_lint/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -523,6 +523,9 @@ lint_undropped_manually_drops = calls to `std::mem::drop` with `std::mem::Manual
lint_ungated_async_fn_track_caller = `#[track_caller]` on async functions is a no-op
.label = this function will not propagate the caller location

lint_unit_bindings = binding has unit type `()`
.label = this pattern is inferred to be the unit type `()`

lint_unknown_gated_lint =
unknown lint: `{$name}`
.note = the `{$name}` lint is unstable
Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_lint/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ mod redundant_semicolon;
mod reference_casting;
mod traits;
mod types;
mod unit_bindings;
mod unused;

pub use array_into_iter::ARRAY_INTO_ITER;
Expand Down Expand Up @@ -123,6 +124,7 @@ use redundant_semicolon::*;
use reference_casting::*;
use traits::*;
use types::*;
use unit_bindings::*;
use unused::*;

/// Useful for other parts of the compiler / Clippy.
Expand Down Expand Up @@ -203,6 +205,7 @@ late_lint_methods!(
InvalidReferenceCasting: InvalidReferenceCasting,
// Depends on referenced function signatures in expressions
UnusedResults: UnusedResults,
UnitBindings: UnitBindings,
NonUpperCaseGlobals: NonUpperCaseGlobals,
NonShorthandFieldPatterns: NonShorthandFieldPatterns,
UnusedAllocation: UnusedAllocation,
Expand Down
7 changes: 7 additions & 0 deletions compiler/rustc_lint/src/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1845,3 +1845,10 @@ impl<'a> DecorateLint<'a, ()> for AsyncFnInTraitDiag {
fluent::lint_async_fn_in_trait
}
}

#[derive(LintDiagnostic)]
#[diag(lint_unit_bindings)]
pub struct UnitBindingsDiag {
#[label]
pub label: Span,
}
72 changes: 72 additions & 0 deletions compiler/rustc_lint/src/unit_bindings.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
use crate::lints::UnitBindingsDiag;
use crate::{LateLintPass, LintContext};
use rustc_hir as hir;
use rustc_middle::ty::Ty;

declare_lint! {
/// The `unit_bindings` lint detects cases where bindings are useless because they have
/// the unit type `()` as their inferred type. The lint is suppressed if the user explicitly
/// annotates the let binding with the unit type `()`, or if the let binding uses an underscore
/// wildcard pattern, i.e. `let _ = expr`, or if the binding is produced from macro expansions.
///
/// ### Example
///
/// ```rust,compile_fail
/// #![deny(unit_bindings)]
///
/// fn foo() {
/// println!("do work");
/// }
///
/// pub fn main() {
/// let x = foo(); // useless binding
/// }
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// Creating a local binding with the unit type `()` does not do much and can be a sign of a
/// user error, such as in this example:
///
/// ```rust,no_run
/// fn main() {
/// let mut x = [1, 2, 3];
/// x[0] = 5;
/// let y = x.sort(); // useless binding as `sort` returns `()` and not the sorted array.
/// println!("{:?}", y); // prints "()"
/// }
/// ```
pub UNIT_BINDINGS,
Allow,
"binding is useless because it has the unit `()` type"
}

declare_lint_pass!(UnitBindings => [UNIT_BINDINGS]);

impl<'tcx> LateLintPass<'tcx> for UnitBindings {
fn check_local(&mut self, cx: &crate::LateContext<'tcx>, local: &'tcx hir::Local<'tcx>) {
// Suppress warning if user:
// - explicitly ascribes a type to the pattern
// - explicitly wrote `let pat = ();`
// - explicitly wrote `let () = init;`.
if !local.span.from_expansion()
&& let Some(tyck_results) = cx.maybe_typeck_results()
&& let Some(init) = local.init
&& let init_ty = tyck_results.expr_ty(init)
&& let local_ty = tyck_results.node_type(local.hir_id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that that is a much more rare case, but should we maybe inspect the pattern instead to find bindings inside? example:

fn tuple<A, B>(a: A, b: B) -> (A, B) { (a, b) }

let (unit, _) = tuple((), 12);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late reply. I don't really have a strong opinion on this. I would expect that this kind of code pattern to be rather rare?

&& init_ty == Ty::new_unit(cx.tcx)
&& local_ty == Ty::new_unit(cx.tcx)
&& local.ty.is_none()
&& !matches!(init.kind, hir::ExprKind::Tup([]))
&& !matches!(local.pat.kind, hir::PatKind::Tuple([], ..))
{
cx.emit_spanned_lint(
UNIT_BINDINGS,
local.span,
UnitBindingsDiag { label: local.pat.span },
);
}
}
}
1 change: 1 addition & 0 deletions tests/ui/closures/issue-868.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// run-pass
#![allow(unused_parens)]
#![allow(unit_bindings)]
// pretty-expanded FIXME #23616

fn f<T, F>(g: F) -> T where F: FnOnce() -> T { g() }
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Variant of diverging-falllback-control-flow that tests
// Variant of diverging-fallback-control-flow that tests
// the specific case of a free function with an unconstrained
// return type. This captures the pattern we saw in the wild
// in the objc crate, where changing the fallback from `!` to `()`
Expand All @@ -9,7 +9,7 @@
// revisions: nofallback fallback

#![cfg_attr(fallback, feature(never_type, never_type_fallback))]

#![allow(unit_bindings)]

fn make_unit() {}

Expand Down
Loading