rustc: Start moving toward "try_get is a bug" for incremental #44071

alexcrichton · 2017-08-24T14:39:05Z

This PR is an effort to burn down some of the work items on #42633. The basic change here was to leave the try_get function exposed but have it return a DiagnosticBuilder instead of a CycleError. This means that it should be a compiler bug to not handle the error as dropping a diagnostic should result in a complier panic.

After that change it was then necessary to update the compiler's callsites of try_get to handle the error coming out. These were handled as:

The sized_constraint and needs_drop_raw checks take the diagnostic and defer it as a compiler bug. This was a new piece of functionality added to the error handling infrastructure, and the idea is that for both these checks a "real" compiler error should be emitted elsewhere, so it's only a bug if we don't actually emit the complier error elsewhere.
MIR inlining was updated to just ignore the diagnostic. This is being tracked by change how MIR inlining handles cycles #43542 which sounded like it either already had some work underway or was planning to change regardless.
The final case, item_path, is still sort of up for debate. At the time of this writing this PR simply removes the invocations of try_get there, assuming that the query will always succeed. This turns out to be true for the test suite anyway! It sounds like, though, that this logic was intended to assist in "weird" situations like RUST_LOG where debug implementations can trigger at any time. This PR would therefore, however, break those implementations.

I'm unfortunately sort of out of ideas on how to handle item_path, but other thoughts would be welcome!

Closes #42633

rust-highfive · 2017-08-24T14:39:09Z

r? @arielb1

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

alexcrichton · 2017-08-24T14:39:16Z

r? @nikomatsakis

nikomatsakis · 2017-08-24T17:19:56Z

@bors r+

bors · 2017-08-24T17:19:57Z

📌 Commit b704502 has been approved by nikomatsakis

eddyb · 2017-08-24T20:24:15Z

The sized_constraint and needs_drop_raw checks take the diagnostic and defer it as a compiler bug

Hadn't considered this, very nice solution! As for everything else, I'm fine with non-user regressions.

eddyb · 2017-08-24T20:26:37Z

src/librustc/ty/maps.rs

@@ -205,7 +206,9 @@ pub struct CycleError<'a, 'tcx: 'a> {
 }

 impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
-    pub fn report_cycle(self, CycleError { span, cycle }: CycleError) {
+    pub fn report_cycle(self, CycleError { span, cycle }: CycleError)


Wait, can this and CycleError be private? The only reason they're public is for try_get, I think?
Also, CycleError can probably be decomposed into its fields now? Not sure though.

Sure yeah, I'll try to do this.

alexcrichton · 2017-08-24T22:14:57Z

@bors: r=nikomatsakis

bors · 2017-08-24T22:14:58Z

📌 Commit b26eb6c has been approved by nikomatsakis

bors · 2017-08-25T22:54:57Z

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

This adds a function to `DiagnosticBuilder` to delay the entire diagnostic as a bug to be emitted at a later time. This'll end up getting used in the compiler in the subsequent commits...

This alters the return value of the `try_get` function so the error contains a diagnostic rather than a `CycleError`. This way consumers are forced to take *some* action (else they get a bug to an un-emitted diagnostic). This action could be to emit the error itself, or in some cases delay the diagnostic as a bug and continue.

The `sized_constraint` and `needs_drop_raw` queries both use `try_get` to detect cycles, but in both of these cases the cycle indicates an error has happened elsewhere in compilation. In these cases we can just delay the diagnostic to get emitted as a bug later if we ended up forgetting to emit the error diagnostic.

It sounds like this is being handled elsewhere, so for now just preserve the existing behavior of ignoring th error.

This seems like it may be likely to cause bugs with `RUST_LOG` and other "interesting" scenarios, but it removes the usage of `try_get` for now!

alexcrichton · 2017-08-25T23:08:50Z

@bors: r=nikomatsakis

bors · 2017-08-25T23:08:50Z

📌 Commit 11b9170 has been approved by nikomatsakis

aidanhs · 2017-08-26T00:32:18Z

@bors r-

[00:06:44] error[E0252]: the name `DiagnosticBuilder` is defined multiple times
[00:06:44]   --> /checkout/src/librustc/ty/maps.rs:13:26
[00:06:44]    |
[00:06:44] 11 | use errors::DiagnosticBuilder;
[00:06:44]    |     ------------------------- previous import of the type `DiagnosticBuilder` here
[00:06:44] 12 | use dep_graph::{DepConstructor, DepNode, DepNodeIndex};
[00:06:44] 13 | use errors::{Diagnostic, DiagnosticBuilder};
[00:06:44]    |                          ^^^^^^^^^^^^^^^^^ `DiagnosticBuilder` reimported here

alexcrichton · 2017-08-26T00:37:33Z

@bors: r=nikomatsakis

bors · 2017-08-26T00:37:34Z

📌 Commit c766aa4 has been approved by nikomatsakis

bors · 2017-08-26T08:25:51Z

⌛ Testing commit c766aa4 with merge e7070dd...

rustc: Start moving toward "try_get is a bug" for incremental This PR is an effort to burn down some of the work items on #42633. The basic change here was to leave the `try_get` function exposed but have it return a `DiagnosticBuilder` instead of a `CycleError`. This means that it should be a compiler bug to *not* handle the error as dropping a diagnostic should result in a complier panic. After that change it was then necessary to update the compiler's callsites of `try_get` to handle the error coming out. These were handled as: * The `sized_constraint` and `needs_drop_raw` checks take the diagnostic and defer it as a compiler bug. This was a new piece of functionality added to the error handling infrastructure, and the idea is that for both these checks a "real" compiler error should be emitted elsewhere, so it's only a bug if we don't actually emit the complier error elsewhere. * MIR inlining was updated to just ignore the diagnostic. This is being tracked by #43542 which sounded like it either already had some work underway or was planning to change regardless. * The final case, `item_path`, is still sort of up for debate. At the time of this writing this PR simply removes the invocations of `try_get` there, assuming that the query will always succeed. This turns out to be true for the test suite anyway! It sounds like, though, that this logic was intended to assist in "weird" situations like `RUST_LOG` where debug implementations can trigger at any time. This PR would therefore, however, break those implementations. I'm unfortunately sort of out of ideas on how to handle `item_path`, but other thoughts would be welcome! Closes #42633

bors · 2017-08-26T11:11:25Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing e7070dd to master...

rust-highfive assigned arielb1 Aug 24, 2017

rust-highfive assigned nikomatsakis and unassigned arielb1 Aug 24, 2017

alexcrichton force-pushed the no-cycles branch 2 times, most recently from d2a5977 to b704502 Compare August 24, 2017 14:41

nikomatsakis approved these changes Aug 24, 2017

View reviewed changes

eddyb reviewed Aug 24, 2017

View reviewed changes

shepmaster added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Aug 25, 2017

alexcrichton added 5 commits August 25, 2017 16:08

rustc_errors: Add the ability to delay as bugs

64b0b2b

This adds a function to `DiagnosticBuilder` to delay the entire diagnostic as a bug to be emitted at a later time. This'll end up getting used in the compiler in the subsequent commits...

rustc: Add a FIXME for try_get in MIR inlining

82bad95

It sounds like this is being handled elsewhere, so for now just preserve the existing behavior of ignoring th error.

rustc: Skip cyclic checks in item_path

c77213d

This seems like it may be likely to cause bugs with `RUST_LOG` and other "interesting" scenarios, but it removes the usage of `try_get` for now!

alexcrichton force-pushed the no-cycles branch from b26eb6c to 11b9170 Compare August 25, 2017 23:08

rustc: Make report_cycle and CycleError private

c766aa4

alexcrichton force-pushed the no-cycles branch from 11b9170 to c766aa4 Compare August 26, 2017 00:37

alexcrichton mentioned this pull request Aug 26, 2017

Rollup of 14 pull requests #44093

Closed

bors merged commit c766aa4 into rust-lang:master Aug 26, 2017

alexcrichton deleted the no-cycles branch August 27, 2017 01:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustc: Start moving toward "try_get is a bug" for incremental #44071

rustc: Start moving toward "try_get is a bug" for incremental #44071

alexcrichton commented Aug 24, 2017

rust-highfive commented Aug 24, 2017

alexcrichton commented Aug 24, 2017

nikomatsakis commented Aug 24, 2017

bors commented Aug 24, 2017

eddyb commented Aug 24, 2017 •

edited

Loading

eddyb Aug 24, 2017

alexcrichton Aug 24, 2017

alexcrichton commented Aug 24, 2017

bors commented Aug 24, 2017

bors commented Aug 25, 2017

alexcrichton commented Aug 25, 2017

bors commented Aug 25, 2017

aidanhs commented Aug 26, 2017 •

edited

Loading

alexcrichton commented Aug 26, 2017

bors commented Aug 26, 2017

bors commented Aug 26, 2017

bors commented Aug 26, 2017

rustc: Start moving toward "try_get is a bug" for incremental #44071

rustc: Start moving toward "try_get is a bug" for incremental #44071

Conversation

alexcrichton commented Aug 24, 2017

rust-highfive commented Aug 24, 2017

alexcrichton commented Aug 24, 2017

nikomatsakis commented Aug 24, 2017

bors commented Aug 24, 2017

eddyb commented Aug 24, 2017 • edited Loading

eddyb Aug 24, 2017

Choose a reason for hiding this comment

alexcrichton Aug 24, 2017

Choose a reason for hiding this comment

alexcrichton commented Aug 24, 2017

bors commented Aug 24, 2017

bors commented Aug 25, 2017

alexcrichton commented Aug 25, 2017

bors commented Aug 25, 2017

aidanhs commented Aug 26, 2017 • edited Loading

alexcrichton commented Aug 26, 2017

bors commented Aug 26, 2017

bors commented Aug 26, 2017

bors commented Aug 26, 2017

eddyb commented Aug 24, 2017 •

edited

Loading

aidanhs commented Aug 26, 2017 •

edited

Loading