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 all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 18 additions & 3 deletions src/librustc_typeck/check/_match.rs
Expand Up @@ -43,7 +43,7 @@ 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(expr.span));
return tcx.types.never;
}

Expand All @@ -69,7 +69,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 +167,21 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
prior_arm_ty = Some(arm_ty);
}

// If all of the arms in the `match` diverge,
// and we're dealing with an actual `match` block
// (as opposed to a `match` desugared from something else'),
// 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
if let (Diverges::Always { .. }, hir::MatchSource::Normal) = (all_arms_diverge, match_src) {
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 All @@ -176,7 +191,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
/// When the previously checked expression (the scrutinee) diverges,
/// warn the user about the match arms being unreachable.
fn warn_arms_when_scrutinee_diverges(&self, arms: &'tcx [hir::Arm], source: hir::MatchSource) {
if self.diverges.get().always() {
if self.diverges.get().is_always() {
use hir::MatchSource::*;
let msg = match source {
IfDesugar { .. } | IfLetDesugar { .. } => "block in `if` expression",
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_typeck/check/expr.rs
Expand Up @@ -170,7 +170,7 @@ 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(expr.span));
}

// Record the type, which applies it effects.
Expand Down
55 changes: 45 additions & 10 deletions src/librustc_typeck/check/mod.rs
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).
span: Span,
/// In some cases (e.g. a `match` expression
/// where all arms diverge), we may be
/// able to provide a more informative
/// message to the user.
/// If this is `None`, a default messsage
/// will be generated, which is suitable
/// for most cases.
custom_note: Option<&'static str>
},

/// Same as `Always` but with a reachability
/// warning already emitted.
Expand Down Expand Up @@ -486,8 +499,22 @@ impl ops::BitOrAssign for Diverges {
}

impl Diverges {
fn always(self) -> bool {
self >= Diverges::Always
/// Creates a `Diverges::Always` with the provided `span` and the default note message.
fn always(span: Span) -> Diverges {
Diverges::Always {
span,
custom_note: None
}
}

fn is_always(self) -> bool {
// Enum comparison ignores the
// contents of fields, so we just
// fill them in with garbage here.
self >= Diverges::Always {
span: DUMMY_SP,
custom_note: None
}
}
}

Expand Down Expand Up @@ -2307,17 +2334,25 @@ 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);
self.tcx().struct_span_lint_hir(lint::builtin::UNREACHABLE_CODE, id, span, &msg)
.span_note(
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you change this to be a span_label instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do it in a follow up since my rollup is running. :)

Also, @Aaron1011, can you look at @estebank's PR #64624 and close the relevant additional issues?

orig_span,
custom_note.unwrap_or("any code following this expression is unreachable")
)
.emit();
}
}
}

Expand Down Expand Up @@ -3825,7 +3860,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
//
// #41425 -- label the implicit `()` as being the
// "found type" here, rather than the "expected type".
if !self.diverges.get().always() {
if !self.diverges.get().is_always() {
// #50009 -- Do not point at the entire fn block span, point at the return type
// span, as it is the cause of the requirement, and
// `consider_hint_about_removing_semicolon` will point at the last expression
Expand Down
5 changes: 5 additions & 0 deletions src/test/ui/dead-code-ret.stderr
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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