Change unreachable pattern ICEs to warnings #39127

canndrew · Jan 17, 2017

Allow code with unreachable ? and for patterns to compile.
Add some tests.

rust-highfive · Jan 17, 2017

(rust_highfive has picked a reviewer for you, use r? to override)

canndrew · Jan 17, 2017

Ugh, bloody make tidy. Every single time.

canndrew · Jan 17, 2017

So to summarise: An unreachable pattern in a ForLoopDesugar is now a warning instead of a span_bug!. An unreachable pattern in a TryDesugar is nothing, not even a warning (because it should be fine to ? a Result<!, T>). ? now desguars to a match expression which has #[allow(unreachable_patterns)] and #[allow(unreachable_code)] on it.

eddyb · Jan 17, 2017

cc @rust-lang/compiler

nikomatsakis · Jan 17, 2017

nikomatsakis approved these changes Jan 17, 2017

View changes

nikomatsakis · Jan 17, 2017

nikomatsakis reviewed Jan 17, 2017

View changes

nikomatsakis · Jan 17, 2017

src/librustc/hir/lowering.rs

@@ -1833,8 +1835,12 @@ impl<'a> LoweringContext<'a> {
                ExprKind::Try(ref sub_expr) => {
                    // to:
                    //
+                    // #[allow(unreachable_patterns)]


Hmm, so, this #[allow(unreachable_patterns)] annotation presumably affects the <expr>, right? That seems unfortunate. i.e., if I do (match { ... })?, do we still see warnings from the inner match?

Good point, I'll need to look into this :/

Can't you key things directly on the MatchSource?

I still seem to get warnings without these. At least the unreachable_code attribute seems to have an effect. I was thinking I'll change the code to this:

match Carrier::translate(<expr>) { #[allow(unused)] Ok(val) => val, #[allow(unused)] Err(err) => return Carrier::from_error(From::from(err)), }

The unreachable_code attribute is indeed fine - no user-defined code exists under it. It's the unreachable_patterns attribute that is the problem - could you check why is it needed?

nikomatsakis · Jan 17, 2017

r? @nikomatsakis (I'm assuming @arielb1 doesn't have time)

durka · Jan 17, 2017

Wait, do expr-level #[allow] annotations work? Last I checked, they have no effect.

arielb1 · Jan 18, 2017

I do have time.

nikomatsakis · Jan 19, 2017

@arielb1 ok :) either way. In any case, I think we both agree that we should try to avoid having warnings get squelched in user code somehow. I agree with the suggestion of using the MatchSource to squelch the lints, does that not work?

nikomatsakis · Jan 19, 2017

@canndrew I just want to clarify something. Given #39151, is this PR necessary for backwards compatibility? I'm trying to assess the priority of landing it with respect to the release timeline.

arielb1 · Jan 19, 2017

@nikomatsakis

The PR is required. #39151 just gates the uninhabitableness of references, it does not revert the changes for matching Result<X, Void>.

arielb1 · Jan 19, 2017

arielb1 reviewed Jan 19, 2017

View changes

arielb1 · Jan 19, 2017

src/librustc/hir/lowering.rs

+                                                                    val_ident,
+                                                                    val_pat.id,
+                                                                    attrs));
+                        let val_block = P(self.block_expr(val_expr));


Do we need the block_expr/expr_block pair?

No.. for some reason I thought we did.

canndrew · Jan 20, 2017

Sorry for spamming travis.

I've now changed the try desugaring to this:

match Carrier::translate(<expr>) {
    Ok(val) => #[allow(unreachable_code)] val,
    Err(err) => #[allow(unreachable_code)] return Carrier::from_error(From::from(err)),
}

That seems to be all we need to crush the warnings.

arielb1 · Jan 20, 2017

arielb1 reviewed Jan 20, 2017

View changes

arielb1 · Jan 20, 2017

src/librustc/hir/lowering.rs

+                    //     Ok(val) => {
+                    //         #[allow(unreachable_code)]
+                    //         val
+                    //     }
                    //     Err(err) => return Carrier::from_error(From::from(err))


nit: update the comment here (including the absence of BlockExpr/ExprBlock pair.

arielb1 · Jan 20, 2017

arielb1 reviewed Jan 20, 2017

View changes

arielb1 · Jan 20, 2017

src/librustc/hir/lowering.rs

+                        let val_expr = P(self.expr_ident_with_attrs(e.span,
+                                                                    val_ident,
+                                                                    val_pat.id,
+                                                                    From::from(attrs.clone())));


I prefer ThinVec::from, but whatever you want.

arielb1 · Jan 20, 2017

arielb1 reviewed Jan 20, 2017

View changes

arielb1 · Jan 20, 2017

src/librustc_const_eval/check_match.rs

-                        hir::MatchSource::TryDesugar => {
-                            span_bug!(pat.span, "unreachable try pattern")
-                        },
+                        hir::MatchSource::TryDesugar => {}


A comment would be nice, for example:

// unreachable patterns in a try expression occur when one of the Result // variants is uninhabited, and should cause a warning.

arielb1 · Jan 20, 2017

arielb1 requested changes Jan 20, 2017

View changes

nice modulo comment nits.

arielb1 · Jan 22, 2017

@bors r+

bors · Jan 22, 2017

📌 Commit 0aad529 has been approved by arielb1

bors · Jan 22, 2017

⌛️ Testing commit 0aad529 with merge 98c3128...

bors · Jan 22, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing 98c3128 to master...

Change unreachable patterns ICEs to warnings
Allow code with unreachable `?` and `for` patterns to compile. Add some tests.

Loading status checks…

1c51f60

rust-highfive assigned arielb1 Jan 17, 2017

Fix make tidy

Loading status checks…

dd13cc3

rust-highfive assigned nikomatsakis and unassigned arielb1 Jan 17, 2017

nikomatsakis assigned arielb1 Jan 19, 2017

canndrew added some commits Jan 20, 2017

Remove attribute on match

Loading status checks…

cc0d63c

Remove redundant block

Loading status checks…

c07aa33

Add unreachable attribute to Err branch

Loading status checks…

e7e7c36

canndrew added some commits Jan 21, 2017

Fix some nits

Loading status checks…

ef9f0ab

Fix comment

Loading status checks…

0aad529

bors merged commit 0aad529 into rust-lang:master Jan 22, 2017
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

This was referenced Jan 26, 2017

Closed

ICE in empty-0.0.4, Rust 1.16, unreachable for-loop pattern #38969

Closed

Irrefutable while-let pattern in log4rs-rolling-file-0.2.0, Rust 1.16 #38972

rust-lang/rust

Join GitHub today

Change unreachable pattern ICEs to warnings #39127

Conversation

canndrew commented Jan 17, 2017

rust-highfive assigned arielb1 Jan 17, 2017

This comment has been minimized.

rust-highfive commented Jan 17, 2017

This comment has been minimized.

canndrew commented Jan 17, 2017

This comment has been minimized.

canndrew commented Jan 17, 2017

This comment has been minimized.

eddyb commented Jan 17, 2017

nikomatsakis approved these changes Jan 17, 2017 View changes

nikomatsakis reviewed Jan 17, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nikomatsakis commented Jan 17, 2017

rust-highfive assigned nikomatsakis and unassigned arielb1 Jan 17, 2017

This comment has been minimized.

durka commented Jan 17, 2017

This comment has been minimized.

arielb1 commented Jan 18, 2017

nikomatsakis assigned arielb1 Jan 19, 2017

This comment has been minimized.

nikomatsakis commented Jan 19, 2017

This comment has been minimized.

nikomatsakis commented Jan 19, 2017

This comment has been minimized.

arielb1 commented Jan 19, 2017

arielb1 reviewed Jan 19, 2017 View changes

This comment has been minimized.

This comment has been minimized.

canndrew added some commits Jan 20, 2017

This comment has been minimized.

canndrew commented Jan 20, 2017

arielb1 reviewed Jan 20, 2017 View changes

This comment has been minimized.

arielb1 Jan 20, 2017 • edited

arielb1 reviewed Jan 20, 2017 View changes

This comment has been minimized.

arielb1 reviewed Jan 20, 2017 View changes

This comment has been minimized.

arielb1 Jan 20, 2017 • edited

arielb1 requested changes Jan 20, 2017 View changes

arielb1 left a comment

canndrew added some commits Jan 21, 2017

This comment has been minimized.

arielb1 commented Jan 22, 2017

This comment has been minimized.

bors commented Jan 22, 2017

This comment has been minimized.

bors commented Jan 22, 2017

bors added a commit that referenced this pull request Jan 22, 2017

This comment has been minimized.

bors commented Jan 22, 2017

Hide details View details bors merged commit 0aad529 into rust-lang:master Jan 22, 2017 2 checks passed

2 checks passed

This was referenced Jan 26, 2017

nikomatsakis approved these changes Jan 17, 2017

View changes

nikomatsakis reviewed Jan 17, 2017

View changes

arielb1 reviewed Jan 19, 2017

View changes

arielb1 reviewed Jan 20, 2017

View changes

arielb1 Jan 20, 2017 •

edited

arielb1 reviewed Jan 20, 2017

View changes

arielb1 reviewed Jan 20, 2017

View changes

arielb1 Jan 20, 2017 •

edited

arielb1 requested changes Jan 20, 2017

View changes

bors merged commit `0aad529` into rust-lang:master Jan 22, 2017
2 checks passed