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

Warn about unreachable code following an expression with an uninhabited type #85556

Merged
merged 2 commits into from
Aug 24, 2021

Conversation

FabianWolff
Copy link
Contributor

This pull request fixes #85071. The issue is that liveness analysis currently is "smarter" than reachability analysis when it comes to detecting uninhabited types: Unreachable code is detected during type checking, where full type information is not yet available. Therefore, the check for type inhabitedness is quite crude:

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

i.e. it only checks for !, but not other, non-trivially uninhabited types, such as empty enums, structs containing an uninhabited type, etc. By contrast, liveness analysis, which runs after type checking, can benefit from the more sophisticated tcx.is_ty_uninhabited_from():

let succ = if self.ir.tcx.is_ty_uninhabited_from(

let succ = if self.ir.tcx.is_ty_uninhabited_from(

This can lead to confusing warnings when a variable is reported as unused, but the use of the variable is not reported as unreachable. For instance:

enum Foo {}
fn f() -> Foo {todo!()}

fn main() {
    let x = f();
    let _ = x;
}

currently leads to

warning: unused variable: `x`
 --> t1.rs:5:9
  |
5 |     let x = f();
  |         ^ help: if this is intentional, prefix it with an underscore: `_x`
  |
  = note: `#[warn(unused_variables)]` on by default

warning: 1 warning emitted

which is confusing, because x appears to be used in line 6. With my changes, I get:

warning: unreachable expression
 --> t1.rs:6:13
  |
5 |     let x = f();
  |             --- any code following this expression is unreachable
6 |     let _ = x;
  |             ^ unreachable expression
  |
  = note: `#[warn(unreachable_code)]` on by default
note: this expression has type `Foo`, which is uninhabited
 --> t1.rs:5:13
  |
5 |     let x = f();
  |             ^^^

warning: unused variable: `x`
 --> t1.rs:5:9
  |
5 |     let x = f();
  |         ^ help: if this is intentional, prefix it with an underscore: `_x`
  |
  = note: `#[warn(unused_variables)]` on by default

warning: 2 warnings emitted

My implementation is slightly inelegant because unreachable code warnings can now be issued in two different places (during type checking and during liveness analysis), but I think it is the solution with the least amount of unnecessary code duplication, given that the new warning integrates nicely with liveness analysis, where unreachable code is already implicitly detected for the purpose of finding unused variables.

@rust-highfive
Copy link
Collaborator

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 21, 2021
Comment on lines 983 to 1000
if let LiveNodeKind::ExprNode(succ_span, succ_id) = self.ir.lnks[succ] {
self.warn_about_unreachable(
expr.span,
ty,
succ_span,
succ_id,
"expression",
);
} else if let LiveNodeKind::VarDefNode(succ_span, succ_id) = self.ir.lnks[succ]
{
self.warn_about_unreachable(
expr.span,
ty,
succ_span,
succ_id,
"definition",
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if let LiveNodeKind::ExprNode(succ_span, succ_id) = self.ir.lnks[succ] {
self.warn_about_unreachable(
expr.span,
ty,
succ_span,
succ_id,
"expression",
);
} else if let LiveNodeKind::VarDefNode(succ_span, succ_id) = self.ir.lnks[succ]
{
self.warn_about_unreachable(
expr.span,
ty,
succ_span,
succ_id,
"definition",
);
}
match self.ir.lnks[succ] {
LiveNodeKind::ExprNode(succ_span, succ_id) => {
self.warn_about_unreachable(
expr.span,
ty,
succ_span,
succ_id,
"expression",
);
}
LiveNodeKind::VarDefNode(succ_span, succ_id) => {
self.warn_about_unreachable(
expr.span,
ty,
succ_span,
succ_id,
"definition",
);
}
_ => {}

) {
let ty = self.typeck_results.expr_ty(expr);
let succ = if self.ir.tcx.is_ty_uninhabited_from(m, ty, self.param_env) {
if let LiveNodeKind::ExprNode(succ_span, succ_id) = self.ir.lnks[succ] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, and given this is the second time you're doing this, maybe adding a method to LiveNodeKind might be warranted?

Comment on lines +1316 to +1325
if !orig_ty.is_never() {
// Unreachable code warnings are already emitted during type checking.
// However, during type checking, full type information is being
// calculated but not yet available, so the check for diverging
// expressions due to uninhabited result types is pretty crude and
// only checks whether ty.is_never(). Here, we have full type
// information available and can issue warnings for less obviously
// uninhabited types (e.g. empty enums). The check above is used so
// that we do not emit the same warning twice if the uninhabited type
// is indeed `!`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason not to remove the current, existing, crude check in favor of this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because type inference makes use of the crude divergence information available, e.g. here:

// Subtle: if there is no explicit tail expression,
// that is typically equivalent to a tail expression
// of `()` -- except if the block diverges. In that
// case, there is no value supplied from the tail
// expression (assuming there are no other breaks,
// this implies that the type of the block will be
// `!`).

The crude check is here:
// Any expression that produces a value of type `!` must have diverged
if ty.is_never() {
self.diverges.set(self.diverges.get() | Diverges::always(expr.span));
}

Unfortunately, we can't replace it with tcx.is_ty_uninhabited_from() because type information has not been committed to tcx yet at this point.

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

I'm ok with the code changes, but left some questions.

@FabianWolff
Copy link
Contributor Author

Thanks for your review and suggestions! I have slightly refactored the code now to implement your suggestions.

@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 11, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 27, 2021
@Dylan-DPC-zz
Copy link

@estebank this seems ready for a final review

@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 17, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 31, 2021
@joelpalmer joelpalmer added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 17, 2021
@jackh726
Copy link
Member

@bors r=estebank,jackh726

@bors
Copy link
Contributor

bors commented Aug 23, 2021

📌 Commit 7a98fd4 has been approved by estebank,jackh726

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 23, 2021
@bors
Copy link
Contributor

bors commented Aug 24, 2021

⌛ Testing commit 7a98fd4 with merge 5ca596f...

@bors
Copy link
Contributor

bors commented Aug 24, 2021

☀️ Test successful - checks-actions
Approved by: estebank,jackh726
Pushing 5ca596f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 24, 2021
@bors bors merged commit 5ca596f into rust-lang:master Aug 24, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 24, 2021
bors added a commit to rust-lang/miri that referenced this pull request Aug 24, 2021
Add `#[allow(unreachable_code)]` to `drop(x)` in `tests/run-pass/generator.rs`

The test [starts to trigger this warning](https://github.com/rust-lang/miri/runs/3408355084?check_suite_focus=true#step:8:264) (I guess it's caused by rust-lang/rust#85556). The warning seems correct, but the unreachable code in that test also seems reasonable.
@rylev
Copy link
Member

rylev commented Sep 1, 2021

This change casued a very large regression in instruction counts. It seems to only be happening in one benchmark webrender-wrench and is consistently affecting the extern_crate, incr_comp_intern_dep_graph_node, and metadata_decode_entry_extern_crate queries in multiple test case variants of that benchmark. Given the nature of the change which only impacts liveness checking, I'm unsure why this might be the case. I was tempted to not report this, but given that the regressions are fairly large, I do think additional investigation is worth looking into.

As part of the performance triage process, I'm marking this as a performance regression. If you believe this performance regression to be justifiable or once you have an issue or PR that addresses this regression, please mark this PR with the label perf-regression-triaged.

@rustbot label +perf-regression

@rustbot rustbot added the perf-regression Performance regression. label Sep 1, 2021
@Mark-Simulacrum
Copy link
Member

This regression comes from a new unreachable expression warning specifically in webrender-wrench, which triggers the standard diagnostic infra. That infrastructure is pretty expensive, in this case loading up information from disk in order to trim paths being the most expensive bit.

As such, marking as triaged -- we expect performance to be worse when emitting extra diagnostics.

@rustbot label +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Sep 20, 2021
@tamird
Copy link
Contributor

tamird commented Oct 11, 2021

This change seems to produce a warning even when no code follows the expression with the uninhabited type. e.g.

type Void = std::convert::Infallible;

fn foo() -> Result<Void, u64> {
    Err(5)
}

fn main() { 
 match foo().expect("should") {}
}

in that code the match on the inhabited type is used to enforce a correctness property; should foo start returning a different type, we'd really like main to not type check.

This pattern is taken straight out of a blog post by @nikomatsakis.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unused variable warning for used variable.