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

Point at original span when emitting unreachable lint #64592

Merged
merged 8 commits into from
Sep 20, 2019
Merged
Show file tree
Hide file tree
Changes from 2 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
26 changes: 24 additions & 2 deletions src/librustc_typeck/check/_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,10 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

// If there are no arms, that is a diverging match; a special case.
if arms.is_empty() {
self.diverges.set(self.diverges.get() | Diverges::Always);
self.diverges.set(self.diverges.get() | Diverges::Always {
Centril marked this conversation as resolved.
Show resolved Hide resolved
span: expr.span,
custom_note: None
});
return tcx.types.never;
}

Expand All @@ -69,7 +72,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
// warnings).
match all_pats_diverge {
Diverges::Maybe => Diverges::Maybe,
Diverges::Always | Diverges::WarnedAlways => Diverges::WarnedAlways,
Diverges::Always { .. } | Diverges::WarnedAlways => Diverges::WarnedAlways,
}
}).collect();

Expand Down Expand Up @@ -167,6 +170,25 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
prior_arm_ty = Some(arm_ty);
}

// If all of the arms in the 'match' diverge,
Aaron1011 marked this conversation as resolved.
Show resolved Hide resolved
// and we're dealing with an actual 'match' block
Aaron1011 marked this conversation as resolved.
Show resolved Hide resolved
// (as opposed to a 'match' desugared from something else'),
Aaron1011 marked this conversation as resolved.
Show resolved Hide resolved
// we can emit a better note. Rather than pointing
// at a diverging expression in an arbitrary arm,
// we can point at the entire 'match' expression
Aaron1011 marked this conversation as resolved.
Show resolved Hide resolved
match (all_arms_diverge, match_src) {
Centril marked this conversation as resolved.
Show resolved Hide resolved
(Diverges::Always { .. }, hir::MatchSource::Normal) => {
all_arms_diverge = Diverges::Always {
span: expr.span,
custom_note: Some(
"any code following this `match` expression is unreachable, \
as all arms diverge"
)
};
},
_ => {}
}

// We won't diverge unless the discriminant or all arms diverge.
self.diverges.set(discrim_diverges | all_arms_diverge);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this + the new bit above be refactored into a method? (I'd like to avoid too much non-spec logic in fn check_match).

Copy link
Member Author

Choose a reason for hiding this comment

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

Such a method would need to take all_arms_diverge, match_src, expr, and discrim_diverges as parameters. I think that would make the code much harder to read, and would split the diverge logic over two methods (there are several other uses of self.diverges in this method).

Copy link
Contributor

Choose a reason for hiding this comment

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

Well... it depends on what you intend to read. If you are writing text for the reference or auditing the reviewing of the language you don't care about Diverges at all because it only concerns a lint and so it is more readable that it not obstruct anything.

Rustfmt would display the following on a single line:

fn f1() {
    impl I {
        fn f2() {
            self.set_diverges_match(expr, match_src, discrim_diverges, all_arms_diverge);
        }
    }
}


Expand Down
5 changes: 4 additions & 1 deletion src/librustc_typeck/check/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,10 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

// Any expression that produces a value of type `!` must have diverged
if ty.is_never() {
self.diverges.set(self.diverges.get() | Diverges::Always);
self.diverges.set(self.diverges.get() | Diverges::Always {
span: expr.span,
custom_note: None
});
}

// Record the type, which applies it effects.
Expand Down
44 changes: 36 additions & 8 deletions src/librustc_typeck/check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -450,7 +450,20 @@ pub enum Diverges {

/// Definitely known to diverge and therefore
/// not reach the next sibling or its parent.
Always,
Always {
/// The `Span` points to the expression
/// that caused us to diverge
/// (e.g. `return`, `break`, etc)
Aaron1011 marked this conversation as resolved.
Show resolved Hide resolved
span: Span,
/// In some cases (e.g. a 'match' expression
Aaron1011 marked this conversation as resolved.
Show resolved Hide resolved
/// where all arms diverge), we may be
/// able to provide a more informative
/// message to the user.
/// If this is None, a default messsage
Aaron1011 marked this conversation as resolved.
Show resolved Hide resolved
/// will be generated, which is suitable
/// for most cases
Aaron1011 marked this conversation as resolved.
Show resolved Hide resolved
custom_note: Option<&'static str>
},

/// Same as `Always` but with a reachability
/// warning already emitted.
Expand Down Expand Up @@ -487,7 +500,13 @@ impl ops::BitOrAssign for Diverges {

impl Diverges {
fn always(self) -> bool {
self >= Diverges::Always
// Enum comparison ignores the
// contents of fields, so we just
// fill them in with garbage here
Aaron1011 marked this conversation as resolved.
Show resolved Hide resolved
self >= Diverges::Always {
span: DUMMY_SP,
custom_note: None
}
}
}

Expand Down Expand Up @@ -2307,17 +2326,26 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
/// Produces warning on the given node, if the current point in the
/// function is unreachable, and there hasn't been another warning.
fn warn_if_unreachable(&self, id: hir::HirId, span: Span, kind: &str) {
if self.diverges.get() == Diverges::Always &&
// FIXME: Combine these two 'if' expressions into one once
// let chains are implemented
if let Diverges::Always { span: orig_span, custom_note } = self.diverges.get() {
// If span arose from a desugaring of `if` or `while`, then it is the condition itself,
// which diverges, that we are about to lint on. This gives suboptimal diagnostics.
// Instead, stop here so that the `if`- or `while`-expression's block is linted instead.
!span.is_desugaring(DesugaringKind::CondTemporary) {
self.diverges.set(Diverges::WarnedAlways);
if !span.is_desugaring(DesugaringKind::CondTemporary) {
self.diverges.set(Diverges::WarnedAlways);

debug!("warn_if_unreachable: id={:?} span={:?} kind={}", id, span, kind);
debug!("warn_if_unreachable: id={:?} span={:?} kind={}", id, span, kind);

let msg = format!("unreachable {}", kind);
self.tcx().lint_hir(lint::builtin::UNREACHABLE_CODE, id, span, &msg);
let msg = format!("unreachable {}", kind);
let mut err = self.tcx().struct_span_lint_hir(lint::builtin::UNREACHABLE_CODE,
id, span, &msg);
err.span_note(
orig_span,
custom_note.unwrap_or("any code following this expression is unreachable")
);
err.emit();
Centril marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

Expand Down
5 changes: 5 additions & 0 deletions src/test/ui/dead-code-ret.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@ note: lint level defined here
|
LL | #![deny(unreachable_code)]
| ^^^^^^^^^^^^^^^^
note: any code following this expression is unreachable
--> $DIR/dead-code-ret.rs:6:5
|
LL | return;
| ^^^^^^
= note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error: aborting due to previous error
Expand Down
5 changes: 5 additions & 0 deletions src/test/ui/if-ret.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,9 @@ LL | fn foo() { if (return) { } }
| ^^^
|
= note: `#[warn(unreachable_code)]` on by default
note: any code following this expression is unreachable
--> $DIR/if-ret.rs:6:15
|
LL | fn foo() { if (return) { } }
| ^^^^^^^^

6 changes: 6 additions & 0 deletions src/test/ui/issues/issue-2150.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,12 @@ note: lint level defined here
|
LL | #![deny(unreachable_code)]
| ^^^^^^^^^^^^^^^^
note: any code following this expression is unreachable
--> $DIR/issue-2150.rs:7:5
|
LL | panic!();
| ^^^^^^^^^
= note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error: aborting due to previous error

5 changes: 5 additions & 0 deletions src/test/ui/issues/issue-7246.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@ note: lint level defined here
|
LL | #![deny(unreachable_code)]
| ^^^^^^^^^^^^^^^^
note: any code following this expression is unreachable
--> $DIR/issue-7246.rs:6:5
|
LL | return;
| ^^^^^^

error: aborting due to previous error

5 changes: 5 additions & 0 deletions src/test/ui/lint/lint-attr-non-item-node.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@ note: lint level defined here
|
LL | #[deny(unreachable_code)]
| ^^^^^^^^^^^^^^^^
note: any code following this expression is unreachable
--> $DIR/lint-attr-non-item-node.rs:6:9
|
LL | break;
| ^^^^^

error: aborting due to previous error

5 changes: 5 additions & 0 deletions src/test/ui/liveness/liveness-unused.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ note: lint level defined here
LL | #![warn(unused)]
| ^^^^^^
= note: `#[warn(unreachable_code)]` implied by `#[warn(unused)]`
note: any code following this expression is unreachable
--> $DIR/liveness-unused.rs:91:9
|
LL | continue;
| ^^^^^^^^

error: unused variable: `x`
--> $DIR/liveness-unused.rs:8:7
Expand Down
5 changes: 5 additions & 0 deletions src/test/ui/match/match-no-arms-unreachable-after.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@ note: lint level defined here
|
LL | #![deny(unreachable_code)]
| ^^^^^^^^^^^^^^^^
note: any code following this expression is unreachable
--> $DIR/match-no-arms-unreachable-after.rs:7:5
|
LL | match v { }
| ^^^^^^^^^^^

error: aborting due to previous error

12 changes: 12 additions & 0 deletions src/test/ui/never-assign-dead-code.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,24 @@ note: lint level defined here
LL | #![warn(unused)]
| ^^^^^^
= note: `#[warn(unreachable_code)]` implied by `#[warn(unused)]`
note: any code following this expression is unreachable
--> $DIR/never-assign-dead-code.rs:9:16
|
LL | let x: ! = panic!("aah");
| ^^^^^^^^^^^^^
= note: this warning originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

warning: unreachable call
--> $DIR/never-assign-dead-code.rs:10:5
|
LL | drop(x);
| ^^^^
|
note: any code following this expression is unreachable
--> $DIR/never-assign-dead-code.rs:10:10
|
LL | drop(x);
| ^

warning: unused variable: `x`
--> $DIR/never-assign-dead-code.rs:9:9
Expand Down
5 changes: 5 additions & 0 deletions src/test/ui/reachable/expr_add.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@ note: lint level defined here
|
LL | #![deny(unreachable_code)]
| ^^^^^^^^^^^^^^^^
note: any code following this expression is unreachable
--> $DIR/expr_add.rs:17:19
|
LL | let x = Foo + return;
| ^^^^^^

error: aborting due to previous error

5 changes: 5 additions & 0 deletions src/test/ui/reachable/expr_again.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@ note: lint level defined here
|
LL | #![deny(unreachable_code)]
| ^^^^^^^^^^^^^^^^
note: any code following this expression is unreachable
--> $DIR/expr_again.rs:7:9
|
LL | continue;
| ^^^^^^^^
= note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error: aborting due to previous error
Expand Down
11 changes: 11 additions & 0 deletions src/test/ui/reachable/expr_array.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,23 @@ note: lint level defined here
|
LL | #![deny(unreachable_code)]
| ^^^^^^^^^^^^^^^^
note: any code following this expression is unreachable
--> $DIR/expr_array.rs:9:26
|
LL | let x: [usize; 2] = [return, 22];
| ^^^^^^

error: unreachable expression
--> $DIR/expr_array.rs:14:25
|
LL | let x: [usize; 2] = [22, return];
| ^^^^^^^^^^^^
|
note: any code following this expression is unreachable
--> $DIR/expr_array.rs:14:30
|
LL | let x: [usize; 2] = [22, return];
| ^^^^^^

error: aborting due to 2 previous errors

17 changes: 17 additions & 0 deletions src/test/ui/reachable/expr_assign.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,35 @@ note: lint level defined here
|
LL | #![deny(unreachable_code)]
| ^^^^^^^^^^^^^^^^
note: any code following this expression is unreachable
--> $DIR/expr_assign.rs:10:9
|
LL | x = return;
| ^^^^^^

error: unreachable expression
--> $DIR/expr_assign.rs:20:14
|
LL | *p = return;
| ^^^^^^
|
note: any code following this expression is unreachable
--> $DIR/expr_assign.rs:20:9
|
LL | *p = return;
| ^^

error: unreachable expression
--> $DIR/expr_assign.rs:26:15
|
LL | *{return; &mut i} = 22;
| ^^^^^^
|
note: any code following this expression is unreachable
--> $DIR/expr_assign.rs:26:7
|
LL | *{return; &mut i} = 22;
| ^^^^^^

error: aborting due to 3 previous errors

10 changes: 10 additions & 0 deletions src/test/ui/reachable/expr_block.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,23 @@ note: lint level defined here
|
LL | #![deny(unreachable_code)]
| ^^^^^^^^^^^^^^^^
note: any code following this expression is unreachable
--> $DIR/expr_block.rs:9:9
|
LL | return;
| ^^^^^^

error: unreachable statement
--> $DIR/expr_block.rs:25:9
|
LL | println!("foo");
| ^^^^^^^^^^^^^^^^
|
note: any code following this expression is unreachable
--> $DIR/expr_block.rs:24:9
|
LL | return;
| ^^^^^^
= note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error: aborting due to 2 previous errors
Expand Down
5 changes: 5 additions & 0 deletions src/test/ui/reachable/expr_box.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@ note: lint level defined here
|
LL | #![deny(unreachable_code)]
| ^^^^^^^^^^^^^^^^
note: any code following this expression is unreachable
--> $DIR/expr_box.rs:6:17
|
LL | let x = box return;
| ^^^^^^

error: aborting due to previous error

11 changes: 11 additions & 0 deletions src/test/ui/reachable/expr_call.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,23 @@ note: lint level defined here
|
LL | #![deny(unreachable_code)]
| ^^^^^^^^^^^^^^^^
note: any code following this expression is unreachable
--> $DIR/expr_call.rs:13:9
|
LL | foo(return, 22);
| ^^^^^^

error: unreachable call
--> $DIR/expr_call.rs:18:5
|
LL | bar(return);
| ^^^
|
note: any code following this expression is unreachable
--> $DIR/expr_call.rs:18:9
|
LL | bar(return);
| ^^^^^^

error: aborting due to 2 previous errors

5 changes: 5 additions & 0 deletions src/test/ui/reachable/expr_cast.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@ note: lint level defined here
|
LL | #![deny(unreachable_code)]
| ^^^^^^^^^^^^^^^^
note: any code following this expression is unreachable
--> $DIR/expr_cast.rs:9:14
|
LL | let x = {return} as !;
| ^^^^^^

error: aborting due to previous error

Loading