-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Point at original span when emitting unreachable lint #64592
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
Changes from all commits
822393d
cd4b468
9e777eb
6edcfbe
a8ce93e
41e1128
034a8fd
d67528f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
@@ -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 | ||
} | ||
} | ||
} | ||
|
||
|
@@ -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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you change this to be a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
} | ||
} | ||
} | ||
|
||
|
@@ -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 | ||
|
There was a problem hiding this comment.
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
).There was a problem hiding this comment.
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
, anddiscrim_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 ofself.diverges
in this method).There was a problem hiding this comment.
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: