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.

Sign up for GitHub

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

Jump to bottom

Only warn about unused mut in user-written code #54787

Merged
merged 2 commits into from
Oct 6, 2018
Merged

Only warn about unused mut in user-written code #54787

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
113141b
Only warn about unused `mut` in user-written code
varkor Oct 3, 2018
ea3d8f5
Ignore desugarings when comparing duplicate trait error messages
varkor Oct 4, 2018
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
25 changes: 16 additions & 9 deletions src/librustc/hir/lowering.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4058,16 +4058,16 @@ impl<'a> LoweringContext<'a> {
// expand <head>
let head = self.lower_expr(head);
let head_sp = head.span;
let desugared_span = self.allow_internal_unstable(
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I right that desugared_span is just the renaming of next_sp ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's correct. I just moved it so that both the spans that were being referenced multiple times were in the same place.

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense

CompilerDesugaringKind::ForLoop,
head_sp,
);

let iter = self.str_to_ident("iter");

let next_ident = self.str_to_ident("__next");
let next_sp = self.allow_internal_unstable(
CompilerDesugaringKind::ForLoop,
head_sp,
);
let next_pat = self.pat_ident_binding_mode(
next_sp,
desugared_span,
next_ident,
hir::BindingAnnotation::Mutable,
);
Expand Down Expand Up @@ -4096,8 +4096,11 @@ impl<'a> LoweringContext<'a> {
};

// `mut iter`
let iter_pat =
self.pat_ident_binding_mode(head_sp, iter, hir::BindingAnnotation::Mutable);
let iter_pat = self.pat_ident_binding_mode(
desugared_span,
Copy link
Contributor

Choose a reason for hiding this comment

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

Must be because this is now using desugared_span and not head.span — we could perhaps "normalize" the spans we use to suppress duplicate trait errors by skipping over "compiler desugarings" into the underlying span.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nikomatsakis For my understanding could you clarify what does normalizing spans mean ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@blitzerr: notice that the problem in this failing test is that an error about a trait bound has been duplicated. The issue here is probably that to check whether these sorts of errors should be emitted (when they might be generated multiple times), the compiler checks against the spans of previously-emitted errors. In this case, one of the errors has span head_sp and the other has desugared_span, even though they really refer to the same issue. If we instead "normalised" the spans, so that head_sp and desugared_span were considered "the same", then we could suppress the other error.

Copy link
Contributor

Choose a reason for hiding this comment

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

@varkor Thanks a lot for the explanation. Makes sense.

iter,
hir::BindingAnnotation::Mutable
);

// `match ::std::iter::Iterator::next(&mut iter) { ... }`
let match_expr = {
Expand Down Expand Up @@ -4126,8 +4129,12 @@ impl<'a> LoweringContext<'a> {
let next_expr = P(self.expr_ident(head_sp, next_ident, next_pat.id));

// `let mut __next`
let next_let =
self.stmt_let_pat(head_sp, None, next_pat, hir::LocalSource::ForLoopDesugar);
let next_let = self.stmt_let_pat(
desugared_span,
None,
next_pat,
hir::LocalSource::ForLoopDesugar,
);

// `let <pat> = __next`
let pat = self.lower_pat(pat);
Expand Down
22 changes: 17 additions & 5 deletions src/librustc/traits/error_reporting.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ use ty::subst::Subst;
use ty::SubtypePredicate;
use util::nodemap::{FxHashMap, FxHashSet};

use syntax_pos::{DUMMY_SP, Span};
use syntax_pos::{DUMMY_SP, Span, ExpnInfo, ExpnFormat};

impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
pub fn report_fulfillment_errors(&self,
Expand All @@ -68,18 +68,30 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
}).collect();

for (index, error) in errors.iter().enumerate() {
error_map.entry(error.obligation.cause.span).or_default().push(
// We want to ignore desugarings here: spans are equivalent even
// if one is the result of a desugaring and the other is not.
let mut span = error.obligation.cause.span;
if let Some(ExpnInfo {
format: ExpnFormat::CompilerDesugaring(_),
def_site: Some(def_span),
..
}) = span.ctxt().outer().expn_info() {
span = def_span;
}

error_map.entry(span).or_default().push(
ErrorDescriptor {
predicate: error.obligation.predicate.clone(),
index: Some(index)
});
}
);

self.reported_trait_errors.borrow_mut()
.entry(error.obligation.cause.span).or_default()
.entry(span).or_default()
.push(error.obligation.predicate.clone());
}

// We do this in 2 passes because we want to display errors in order, tho
// We do this in 2 passes because we want to display errors in order, though
// maybe it *is* better to sort errors by span or something.
let mut is_suppressed = vec![false; errors.len()];
for (_, error_set) in error_map.iter() {
Expand Down
6 changes: 5 additions & 1 deletion src/librustc_borrowck/borrowck/unused.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,14 @@ impl<'a, 'tcx> UnusedMutCx<'a, 'tcx> {
}

let (hir_id, span) = ids[0];
let mut_span = tcx.sess.source_map().span_until_non_whitespace(span);
if span.compiler_desugaring_kind().is_some() {
// If the `mut` arises as part of a desugaring, we should ignore it.
continue;
}

// Ok, every name wasn't used mutably, so issue a warning that this
// didn't need to be mutable.
let mut_span = tcx.sess.source_map().span_until_non_whitespace(span);
tcx.struct_span_lint_hir(UNUSED_MUT,
hir_id,
span,
Expand Down
6 changes: 5 additions & 1 deletion src/librustc_mir/borrow_check/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,14 +316,18 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>(
}

let span = local_decl.source_info.span;
let mut_span = tcx.sess.source_map().span_until_non_whitespace(span);
if span.compiler_desugaring_kind().is_some() {
// If the `mut` arises as part of a desugaring, we should ignore it.
continue;
}

let mut err = tcx.struct_span_lint_node(
UNUSED_MUT,
vsi[local_decl.source_info.scope].lint_root,
span,
"variable does not need to be mutable",
);
let mut_span = tcx.sess.source_map().span_until_non_whitespace(span);
err.span_suggestion_short_with_applicability(
mut_span,
"remove this `mut`",
Expand Down
3 changes: 2 additions & 1 deletion src/librustc_mir/shim.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,8 @@ enum CallKind {
fn temp_decl(mutability: Mutability, ty: Ty, span: Span) -> LocalDecl {
let source_info = SourceInfo { scope: OUTERMOST_SOURCE_SCOPE, span };
LocalDecl {
mutability, ty,
mutability,
ty,
user_ty: None,
name: None,
source_info,
Expand Down
8 changes: 8 additions & 0 deletions src/test/ui/mut/no-mut-lint-for-desugared-mut.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// run-pass

#![deny(unused_mut)]
#![allow(unreachable_code)]

fn main() {
for _ in { return (); 0..3 } {} // ok
}