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

Suppress unused_mut if unused_variables is reported #101977

Closed
wants to merge 2 commits into from
Closed
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
2 changes: 1 addition & 1 deletion compiler/rustc_borrowck/src/consumers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,6 @@ pub fn get_body_with_borrowck_facts<'tcx>(
tcx.infer_ctxt().with_opaque_type_inference(DefiningAnchor::Bind(def.did)).enter(|infcx| {
let input_body: &Body<'_> = &input_body.borrow();
let promoted: &IndexVec<_, _> = &promoted.borrow();
*super::do_mir_borrowck(&infcx, input_body, promoted, true).1.unwrap()
*super::do_mir_borrowck(&infcx, input_body, promoted, &None, true).1.unwrap()
})
}
21 changes: 18 additions & 3 deletions compiler/rustc_borrowck/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ extern crate rustc_middle;
#[macro_use]
extern crate tracing;

use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_data_structures::fx::{FxHashMap, FxHashSet, FxIndexSet};
use rustc_data_structures::graph::dominators::Dominators;
use rustc_errors::{Diagnostic, DiagnosticBuilder, ErrorGuaranteed};
use rustc_hir as hir;
Expand Down Expand Up @@ -129,16 +129,22 @@ fn mir_borrowck<'tcx>(
def: ty::WithOptConstParam<LocalDefId>,
) -> &'tcx BorrowCheckResult<'tcx> {
let (input_body, promoted) = tcx.mir_promoted(def);
debug!("run query mir_borrowck: {}", tcx.def_path_str(def.did.to_def_id()));

let def_id = def.did.to_def_id();
debug!("run query mir_borrowck: {}", tcx.def_path_str(def_id));

let hir_owner = tcx.hir().local_def_id_to_hir_id(def.did).owner;

let is_fn = matches!(tcx.hir().body_owner_kind(def.did), hir::BodyOwnerKind::Fn);
let unused_variables_spans = if is_fn { tcx.check_liveness(def_id) } else { &None };

let opt_closure_req = tcx
.infer_ctxt()
.with_opaque_type_inference(DefiningAnchor::Bind(hir_owner))
.enter(|infcx| {
let input_body: &Body<'_> = &input_body.borrow();
let promoted: &IndexVec<_, _> = &promoted.borrow();
do_mir_borrowck(&infcx, input_body, promoted, false).0
do_mir_borrowck(&infcx, input_body, promoted, unused_variables_spans, false).0
});
debug!("mir_borrowck done");

Expand All @@ -155,6 +161,7 @@ fn do_mir_borrowck<'a, 'tcx>(
infcx: &InferCtxt<'a, 'tcx>,
input_body: &Body<'tcx>,
input_promoted: &IndexVec<Promoted, Body<'tcx>>,
unused_variables_spans: &Option<FxIndexSet<Span>>,
return_body_with_facts: bool,
) -> (BorrowCheckResult<'tcx>, Option<Box<BodyWithBorrowckFacts<'tcx>>>) {
let def = input_body.source.with_opt_param().as_local().unwrap();
Expand Down Expand Up @@ -428,6 +435,14 @@ fn do_mir_borrowck<'a, 'tcx>(
}

let mut_span = tcx.sess.source_map().span_until_non_whitespace(span);
let ident_span = span.with_lo(mut_span.hi());

// Suppress lints if we already reported unused variables
if let Some(unused_variables_spans) = unused_variables_spans {
if unused_variables_spans.contains(&ident_span) {
continue;
}
}

tcx.emit_spanned_lint(UNUSED_MUT, lint_root, span, VarNeedNotMut { span: mut_span })
}
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_lint_defs/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1558,6 +1558,7 @@ declare_lint! {
///
/// ```rust
/// let mut x = 5;
/// drop(x);
/// ```
///
/// {{produces}}
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_middle/src/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -830,7 +830,8 @@ rustc_queries! {
desc { |tcx| "checking privacy in {}", describe_as_module(key, tcx) }
}

query check_liveness(key: DefId) {
query check_liveness(key: DefId) -> Option<FxIndexSet<Span>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Spans are not a very robust source of information. Do we have a better, more "semantic" source of information?

Copy link
Member Author

@sanxiyn sanxiyn Sep 20, 2022

Choose a reason for hiding this comment

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

I tried, but couldn't find one. Rather, there is a lot on liveness side working on HIR, but they are missing on borrowck side working on MIR, which needs to use this information.

arena_cache
desc { |tcx| "checking liveness of variables in {}", tcx.def_path_str(key) }
}

Expand Down
52 changes: 34 additions & 18 deletions compiler/rustc_passes/src/liveness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ use self::LiveNodeKind::*;
use self::VarKind::*;

use rustc_ast::InlineAsmOptions;
use rustc_data_structures::fx::FxIndexMap;
use rustc_data_structures::fx::{FxIndexMap, FxIndexSet};
use rustc_errors::Applicability;
use rustc_hir as hir;
use rustc_hir::def::*;
Expand All @@ -99,6 +99,7 @@ use rustc_session::lint;
use rustc_span::symbol::{kw, sym, Symbol};
use rustc_span::{BytePos, Span};

use std::cell::RefCell;
use std::collections::VecDeque;
use std::io;
use std::io::prelude::*;
Expand Down Expand Up @@ -138,9 +139,9 @@ fn live_node_kind_to_string(lnk: LiveNodeKind, tcx: TyCtxt<'_>) -> String {
}
}

fn check_liveness(tcx: TyCtxt<'_>, def_id: DefId) {
fn check_liveness(tcx: TyCtxt<'_>, def_id: DefId) -> Option<FxIndexSet<Span>> {
let local_def_id = match def_id.as_local() {
None => return,
None => return None,
Some(def_id) => def_id,
};

Expand All @@ -149,12 +150,12 @@ fn check_liveness(tcx: TyCtxt<'_>, def_id: DefId) {
if let DefKind::Impl = tcx.def_kind(parent)
&& tcx.has_attr(parent.to_def_id(), sym::automatically_derived)
{
return;
return None;
}

// Don't run unused pass for #[naked]
if tcx.has_attr(def_id, sym::naked) {
return;
return None;
}

let mut maps = IrMaps::new(tcx);
Expand Down Expand Up @@ -182,6 +183,8 @@ fn check_liveness(tcx: TyCtxt<'_>, def_id: DefId) {
lsets.visit_body(body);
lsets.warn_about_unused_upvars(entry_ln);
lsets.warn_about_unused_args(body, entry_ln);

Some(lsets.unused_variables_spans.into_inner())
}

pub fn provide(providers: &mut Providers) {
Expand Down Expand Up @@ -517,6 +520,8 @@ struct Liveness<'a, 'tcx> {
// it probably doesn't now)
break_ln: HirIdMap<LiveNode>,
cont_ln: HirIdMap<LiveNode>,

unused_variables_spans: RefCell<FxIndexSet<Span>>,
}

impl<'a, 'tcx> Liveness<'a, 'tcx> {
Expand All @@ -541,6 +546,7 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> {
exit_ln,
break_ln: Default::default(),
cont_ln: Default::default(),
unused_variables_spans: Default::default(),
}
}

Expand Down Expand Up @@ -1504,6 +1510,7 @@ impl<'tcx> Liveness<'_, 'tcx> {
}
} else {
if let Some(name) = self.should_warn(var) {
self.unused_variables_spans.borrow_mut().insert(span);
self.ir.tcx.struct_span_lint_hir(
lint::builtin::UNUSED_VARIABLES,
var_hir_id,
Expand Down Expand Up @@ -1594,24 +1601,29 @@ impl<'tcx> Liveness<'_, 'tcx> {
if ln == self.exit_ln { false } else { self.assigned_on_exit(ln, var) };

if is_assigned {
let spans = hir_ids_and_spans
.into_iter()
.map(|(_, _, ident_span)| ident_span)
.collect::<Vec<_>>();
self.unused_variables_spans.borrow_mut().extend(&spans);
Copy link
Contributor

Choose a reason for hiding this comment

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

This will populate unused_variables_spans even if we do not emit an unused_variable lint, if explicitly allowed for instance. This may be surprising, not to have a lint at all in those cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. It is troublesome indeed. I will think about it.

self.ir.tcx.struct_span_lint_hir(
lint::builtin::UNUSED_VARIABLES,
first_hir_id,
hir_ids_and_spans
.into_iter()
.map(|(_, _, ident_span)| ident_span)
.collect::<Vec<_>>(),
spans,
|lint| {
lint.build(&format!("variable `{}` is assigned to, but never used", name))
.note(&format!("consider using `_{}` instead", name))
.emit();
},
)
} else if can_remove {
let spans =
hir_ids_and_spans.iter().map(|(_, pat_span, _)| *pat_span).collect::<Vec<_>>();
self.unused_variables_spans.borrow_mut().extend(&spans);
self.ir.tcx.struct_span_lint_hir(
lint::builtin::UNUSED_VARIABLES,
first_hir_id,
hir_ids_and_spans.iter().map(|(_, pat_span, _)| *pat_span).collect::<Vec<_>>(),
spans,
|lint| {
let mut err = lint.build(&format!("unused variable: `{}`", name));
err.multipart_suggestion(
Expand Down Expand Up @@ -1654,13 +1666,15 @@ impl<'tcx> Liveness<'_, 'tcx> {
)
.collect::<Vec<_>>();

let spans = hir_ids_and_spans
.iter()
.map(|(_, pat_span, _)| *pat_span)
.collect::<Vec<_>>();
self.unused_variables_spans.borrow_mut().extend(&spans);
self.ir.tcx.struct_span_lint_hir(
lint::builtin::UNUSED_VARIABLES,
first_hir_id,
hir_ids_and_spans
.iter()
.map(|(_, pat_span, _)| *pat_span)
.collect::<Vec<_>>(),
spans,
|lint| {
let mut err = lint.build(&format!("unused variable: `{}`", name));
err.multipart_suggestion(
Expand All @@ -1677,13 +1691,15 @@ impl<'tcx> Liveness<'_, 'tcx> {
.map(|(_, _, ident_span)| (ident_span, format!("_{}", name)))
.collect::<Vec<_>>();

let spans = hir_ids_and_spans
.iter()
.map(|(_, _, ident_span)| *ident_span)
.collect::<Vec<_>>();
self.unused_variables_spans.borrow_mut().extend(&spans);
self.ir.tcx.struct_span_lint_hir(
lint::builtin::UNUSED_VARIABLES,
first_hir_id,
hir_ids_and_spans
.iter()
.map(|(_, _, ident_span)| *ident_span)
.collect::<Vec<_>>(),
spans,
|lint| {
let mut err = lint.build(&format!("unused variable: `{}`", name));
if self.has_added_lit_match_name_span(&name, opt_body, &mut err) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,22 +21,22 @@ mod foo {
}

#[expect(
unused_mut,
unused_variables,
//~^ WARNING this lint expectation is unfulfilled [unfulfilled_lint_expectations]
//~| NOTE this `expect` is overridden by a `warn` attribute before the `unused_mut` lint is triggered
reason = "this `expect` is overridden by a `warn` attribute before the `unused_mut` lint is triggered"
//~| NOTE this `expect` is overridden by a `warn` attribute before the `unused_variables` lint is triggered
reason = "this `expect` is overridden by a `warn` attribute before the `unused_variables` lint is triggered"
)]
mod oof {
#[warn(
unused_mut,
unused_variables,
//~^ NOTE the lint level is defined here
reason = "this overrides the previous `expect` lint level and warns about the `unused_mut` lint here"
reason = "this overrides the previous `expect` lint level and warns about the `unused_variables` lint here"
)]
fn bar() {
let mut v = 0;
//~^ WARNING variable does not need to be mutable [unused_mut]
//~| NOTE this overrides the previous `expect` lint level and warns about the `unused_mut` lint here
//~| HELP remove this `mut`
//~^ WARNING unused variable: `v` [unused_variables]
//~| NOTE this overrides the previous `expect` lint level and warns about the `unused_variables` lint here
//~| HELP if this is intentional, prefix it with an underscore
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,16 @@
warning: unused variable: `v`
--> $DIR/expect_nested_lint_levels.rs:36:17
|
LL | let mut v = 0;
| ^ help: if this is intentional, prefix it with an underscore: `_v`
|
= note: this overrides the previous `expect` lint level and warns about the `unused_variables` lint here
note: the lint level is defined here
--> $DIR/expect_nested_lint_levels.rs:31:9
|
LL | unused_variables,
| ^^^^^^^^^^^^^^^^

error: unused variable: `this_is_my_function`
--> $DIR/expect_nested_lint_levels.rs:48:9
|
Expand All @@ -10,21 +23,6 @@ note: the lint level is defined here
LL | #[forbid(unused_variables)]
| ^^^^^^^^^^^^^^^^

warning: variable does not need to be mutable
--> $DIR/expect_nested_lint_levels.rs:36:13
|
LL | let mut v = 0;
| ----^
| |
| help: remove this `mut`
|
= note: this overrides the previous `expect` lint level and warns about the `unused_mut` lint here
note: the lint level is defined here
--> $DIR/expect_nested_lint_levels.rs:31:9
|
LL | unused_mut,
| ^^^^^^^^^^

warning: this lint expectation is unfulfilled
--> $DIR/expect_nested_lint_levels.rs:7:5
|
Expand All @@ -37,10 +35,10 @@ LL | unused_mut,
warning: this lint expectation is unfulfilled
--> $DIR/expect_nested_lint_levels.rs:24:5
|
LL | unused_mut,
| ^^^^^^^^^^
LL | unused_variables,
| ^^^^^^^^^^^^^^^^
|
= note: this `expect` is overridden by a `warn` attribute before the `unused_mut` lint is triggered
= note: this `expect` is overridden by a `warn` attribute before the `unused_variables` lint is triggered

warning: this lint expectation is unfulfilled
--> $DIR/expect_nested_lint_levels.rs:43:10
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,10 @@ fn main() {

let mut mut_unused_var = 1;
//~^ WARNING unused variable: `mut_unused_var`
//~| WARNING variable does not need to be mutable

let (mut var, unused_var) = (1, 2);
//~^ WARNING unused variable: `var`
//~| WARNING unused variable: `unused_var`
//~| WARNING variable does not need to be mutable
// NOTE: `var` comes after `unused_var` lexicographically yet the warning
// for `var` will be emitted before the one for `unused_var`. We use an
// `IndexMap` to ensure this is the case instead of a `BTreeMap`.
Expand Down
Loading