melt the ICE when lowering an impossible range #32267

durka · 2016-03-15T17:11:53Z

Emit a fatal error instead of panicking when HIR lowering encounters a range with no end point.

This involved adding a method to wire up LoweringContext::span_fatal.

Fixes #32245 (cc @nodakai).

r? @nrc

durka · 2016-03-15T17:16:16Z

Should I define an error code and use span_fatal_with_code instead?

alexcrichton · 2016-03-15T17:17:37Z

"melt the ICE" -- this is an amazing phrase

🍧

durka · 2016-03-15T17:18:32Z

Also, if span_fatal is for things that shouldn't happen... you can write a lone ... no problem and it doesn't get caught until here. (Whereas a... cannot be produced without a broken syntax extension.) So should I try to catch lone ... in the parser as well, so that it is recoverable?

nrc · 2016-03-16T01:55:46Z

Should I define an error code and use span_fatal_with_code instead?

Ideally yes, but I don't think it is essential.

Also, if span_fatal is for things that shouldn't happen... you can write a lone ... no problem and it doesn't get caught until here. (Whereas a... cannot be produced without a broken syntax extension.) So should I try to catch lone ... in the parser as well, so that it is recoverable?

span_fatal is for things we can't recover from, rather than things that shouldn't happen. It's nice if those two things align, since unrecoverable errors are not great for the user, so should be rare.

I think we should try and catch a lone ... before here. If we can do so in the parser then that would be good. (Note that since we can recover in the parser, we won't abort compilation until lowering and we shouldn't report the error twice. Usually we have some mechanism for that - e.g., we don't emit errors that involve TyErr, but in this case we might have to do something ad hoc).

nrc · 2016-03-16T01:58:00Z

src/librustc/session/mod.rs

@@ -336,6 +336,10 @@ impl NodeIdAssigner for Session {
    fn peek_node_id(&self) -> NodeId {
        self.next_node_id.get().checked_add(1).unwrap()
    }
+
+    fn span_fatal(&self, sp: MultiSpan, msg: &str) -> ! {


NodeIdAssigner could have a GetDiagnostic method returning Option, that avoids having error methods for every kind of error in NodeIdAssigner.

Or just a Diagnostic and panic if it can't be done.

Hmm so bring in DiagnosticBuilder from libsyntax?

syntax::errors::Handler (sorry, naming has changed somewhat); and GetHandler.

Done (except this code path is now untested because the parse error was made fatal).

durka · 2016-03-16T04:26:38Z

I think we should try and catch a lone ... before here. If we can do so in the parser then that would be good. (Note that since we can recover in the parser, we won't abort compilation until lowering and we shouldn't report the error twice. Usually we have some mechanism for that - e.g., we don't emit errors that involve TyErr, but in this case we might have to do something ad hoc).

I was wondering about this. What does the parser leave behind in the AST when it recovers? Does it just skip the line of code? I tried to inspect the AST, but apparently semicolon recovery isn't enough to get to --pretty=expanded.

nrc · 2016-03-16T05:33:02Z

In the AST there isn't an 'error' node. We only recover if we can make a valid AST.

durka · 2016-03-16T05:37:34Z

src/test/compile-fail/impossible_range.rs

+    ..1;
+    0..1;
+
+    ...; //~ERROR inclusive range with no end


I'm a little confused. We get to this error during lowering, so I assumed the one on the next line (which is a parse error) was recovered. But what valid AST could be left behind?

A range with neither start nor end - the AST doesn't have to be well-formed (for some definition), it just must be constructable. We don't get the lowering error there because the first error was fatal. I suspect if you have a test case with just that line, you would get both a parsing and lowering error.

durka · 2016-03-16T14:13:00Z

in this case we might have to do something ad hoc

Here's my plan (can't work on it until tonight): add a field to ast::RangeLimits (perhaps renamed to ast::RangeKind) which the parser sets if it emits an error while constructing a bad range. I can set this field and emit the error in mk_range. Then in HIR lowering we will emit an error if this field isn't set, but if it is... abort silently? This is still a bad experience if a syntax extension sets the field by mistake though.

Anyway, this situation will probably go away when @aturon and I get around to writing that RFC to change the syntax and allow all types of ranges :)

durka · 2016-03-17T01:38:32Z

I added the parse error. Haven't implemented the NodeIdAssigner change yet.

durka · 2016-03-17T03:29:39Z

OK, ready for review.

durka · 2016-03-17T04:28:08Z

src/librustc/session/mod.rs

@@ -336,6 +336,10 @@ impl NodeIdAssigner for Session {
    fn peek_node_id(&self) -> NodeId {
        self.next_node_id.get().checked_add(1).unwrap()
    }
+
+    fn diagnostic(&self) -> &errors::Handler {
+        self.diagnostic()


Um. This looks... weird. Did I introduce an infinite recursion (I didn't see a warning) or it's OK because inherent fns take precedence over trait fns?

yeah, inherent has precedence.

and looking more at this has made me realized that one cannot resort to UFCS as a way to try to "clarify" that the inherent method is what is intended. There's no way to say "I want an inherent impl and only an inherent impl", at least not via UFCS...

I think you can.

There's no way to say "I want an inherent impl and only an inherent impl"

That seems good. Then you can factor out an inherent method into a trait method without a breaking change, is that true?

nrc · 2016-03-18T01:05:24Z

src/librustc_front/lowering.rs

@@ -140,6 +141,11 @@ impl<'a, 'hir> LoweringContext<'a> {
            result
        }
    }
+
+    // panics if this LoweringContext's NodeIdAssigner is not a Session


I think the bit about being a Session is too much detail, one could imagine implementing NodeIdAssigner with something that is not a Session but can still provide a diagnostic (also, nit, comments should start with a capital and end with a period).

nrc · 2016-03-18T01:09:06Z

looks good, r+ with the minor comments addressed

durka · 2016-03-18T23:05:28Z

2/3 nits addressed, 1/3 nits quibbled with.

durka · 2016-03-23T17:10:03Z

Should be good to go now.

bors · 2016-03-23T22:52:57Z

☔ The latest upstream changes (presumably #32454) made this pull request unmergeable. Please resolve the merge conflicts.

nrc · 2016-03-24T01:14:07Z

r+, but needs a rebase

End-less ranges (`a...`) don't parse but bad syntax extensions could conceivably produce them. Unbounded ranges (`...`) do parse and are caught here. The other panics in HIR lowering are all for unexpanded macros, which cannot be constructed by bad syntax extensions.

durka · 2016-03-24T05:39:41Z

Guh. The untry conversion broke a bunch of alignment in libsyntax. Don't know why y'all let that through without reformatting.

Now it is impossible for `...` or `a...` to reach HIR lowering without a rogue syntax extension in play.

durka · 2016-03-24T05:42:42Z

Rebased.

durka · 2016-03-24T05:44:40Z

src/libsyntax/parse/parser.rs

+                .map(|x|{
+                    hi = x.span.hi;
+                    x
+                })?)


Just for the record, I think this is much harder to read than the old version with try!. You have to parse the entire expression backwards from ? to see what it applies to.

nrc · 2016-03-28T19:30:35Z

@bors: r+

bors · 2016-03-28T19:30:37Z

📌 Commit 861644f has been approved by nrc

bors · 2016-03-28T22:08:49Z

⌛ Testing commit 861644f with merge 44a77f6...

@nodakai

melt the ICE when lowering an impossible range Emit a fatal error instead of panicking when HIR lowering encounters a range with no `end` point. This involved adding a method to wire up `LoweringContext::span_fatal`. Fixes #32245 (cc @nodakai). r? @nrc

bors · 2016-03-29T00:25:41Z

rust-highfive assigned nrc Mar 15, 2016

nrc reviewed Mar 16, 2016
View reviewed changes

durka reviewed Mar 16, 2016
View reviewed changes

durka force-pushed the inclusive-range-error branch from cf055e3 to fb88d61 Compare March 17, 2016 01:36

durka force-pushed the inclusive-range-error branch from fb88d61 to d8ca46e Compare March 17, 2016 03:29

durka force-pushed the inclusive-range-error branch from d8ca46e to 9f4b098 Compare March 17, 2016 04:03

durka reviewed Mar 17, 2016
View reviewed changes

durka force-pushed the inclusive-range-error branch from 9f4b098 to bbe56ac Compare March 17, 2016 15:08

nrc reviewed Mar 18, 2016
View reviewed changes

durka force-pushed the inclusive-range-error branch from 411c740 to 0046090 Compare March 23, 2016 14:13

durka mentioned this pull request Mar 23, 2016

Regression on beta: three dots in a trait leads to an infinite loop(?) #32446

Closed

durka added 2 commits March 24, 2016 01:33

test errors for malformed inclusive ranges

cd80c1b

durka added 2 commits March 24, 2016 01:42

error during parsing for malformed inclusive range

9f899a6

Now it is impossible for `...` or `a...` to reach HIR lowering without a rogue syntax extension in play.

address nits

861644f

durka force-pushed the inclusive-range-error branch from 0046090 to 861644f Compare March 24, 2016 05:42

durka reviewed Mar 24, 2016
View reviewed changes

bors merged commit 861644f into rust-lang:master Mar 29, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

melt the ICE when lowering an impossible range #32267

melt the ICE when lowering an impossible range #32267

durka commented Mar 15, 2016

durka commented Mar 15, 2016

alexcrichton commented Mar 15, 2016

durka commented Mar 15, 2016

nrc commented Mar 16, 2016

nrc Mar 16, 2016

nrc Mar 16, 2016

durka Mar 16, 2016

nrc Mar 16, 2016

durka Mar 17, 2016

durka commented Mar 16, 2016

nrc commented Mar 16, 2016

durka Mar 16, 2016

nrc Mar 16, 2016

durka commented Mar 16, 2016

durka commented Mar 17, 2016

durka commented Mar 17, 2016

durka Mar 17, 2016

pnkfelix Mar 18, 2016

durka Mar 18, 2016

durka Mar 19, 2016

nrc Mar 18, 2016

durka Mar 18, 2016

nrc commented Mar 18, 2016

durka commented Mar 18, 2016

durka commented Mar 23, 2016

bors commented Mar 23, 2016

nrc commented Mar 24, 2016

durka commented Mar 24, 2016

durka commented Mar 24, 2016

durka Mar 24, 2016

nrc commented Mar 28, 2016

bors commented Mar 28, 2016

bors commented Mar 28, 2016

bors commented Mar 29, 2016

melt the ICE when lowering an impossible range #32267

melt the ICE when lowering an impossible range #32267

Conversation

durka commented Mar 15, 2016

durka commented Mar 15, 2016

alexcrichton commented Mar 15, 2016

durka commented Mar 15, 2016

nrc commented Mar 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durka commented Mar 16, 2016

nrc commented Mar 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durka commented Mar 16, 2016

durka commented Mar 17, 2016

durka commented Mar 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrc commented Mar 18, 2016

durka commented Mar 18, 2016

durka commented Mar 23, 2016

bors commented Mar 23, 2016

nrc commented Mar 24, 2016

durka commented Mar 24, 2016

durka commented Mar 24, 2016

Choose a reason for hiding this comment

nrc commented Mar 28, 2016

bors commented Mar 28, 2016

bors commented Mar 28, 2016

bors commented Mar 29, 2016