Skip to content

Add a fatal_cycle attribute for queries which indicates that they will cause a fatal error on query cycles #47906

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Feb 17, 2018

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Jan 31, 2018

This moves us towards the goal of having cycle errors be non-fatal by not relying on the default implementation of ty::maps::values::Value which aborts on errors.

r? @nikomatsakis

@kennytm kennytm added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 31, 2018
@@ -193,7 +205,7 @@ macro_rules! define_maps {

define_map_struct! {
tcx: $tcx,
input: ($(([$($modifiers)*] [$($attr)*] [$name]))*)
input: ($(([$($attr)*] [$name]))*)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to remove this use of $modifiers otherwise I got this error:

error: no rules expected the token `nocycle`
   --> librustc\ty\maps\mod.rs:270:6
    |
270 |     [nocycle] fn is_panic_runtime: IsPanicRuntime(CrateNum) -> bool,
    |      ^^^^^^^

error: aborting due to previous error

I'm not sure why this happens. define_map_struct should accept any tokens there. 😕

Copy link
Contributor

Choose a reason for hiding this comment

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

see my comment below :)

@pietroalbini
Copy link
Member

@nikomatsakis this PR needs your review :)

@eddyb
Copy link
Member

eddyb commented Feb 5, 2018

I've mentioned on IRC that I prefer calling this [fatal_cycle] and using the current default implementation of Value::from_cycle_error in those cases, to avoid regressing UX with it.

@nikomatsakis
Copy link
Contributor

Sorry for being slow. I meant to leave a comment on this branch -- I guess in the end I don't have strong opinions here, but being able to identify queries that we really don't expect to participate in a cycle seems like a fine thing to me. I'd be tempted to reverse the default, and instead mark those queries that do expect to be in cycles, but @eddyb thinks that will wind up being brittle -- that maybe so. Still, I know there are a lot of queries where I've made no effort to make a nice "debug msg" precisely because I don't expect them to show up in cycle error messages. I guess I could mark all of those as no-cycle.

@@ -583,7 +595,7 @@ macro_rules! define_maps {

macro_rules! define_map_struct {
(tcx: $tcx:tt,
input: ($(([$(modifiers:tt)*] [$($attr:tt)*] [$name:ident]))*)) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

the problem was here. This should have been:

$($modifiers:tt)*

note the $ before modifiers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

$($())*

@nikomatsakis
Copy link
Contributor

I have no opinion about nocycle vs fatal_cycle -- well, I guess I mildly prefer fatal_cycle, since fatal carries rather specific connotations (and I prefer an _ between words).

r=me with the name changed.

I'm indifferent about whether we remove the [$($modifiers:tt)*] from define_map_struct -- we can always add it back in if we need it again.

@nikomatsakis nikomatsakis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 6, 2018
@Zoxc
Copy link
Contributor Author

Zoxc commented Feb 10, 2018

@bors r=nikomatsakis

@bors
Copy link
Collaborator

bors commented Feb 10, 2018

📌 Commit e236994 has been approved by nikomatsakis

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 10, 2018
@eddyb
Copy link
Member

eddyb commented Feb 10, 2018

@Zoxc Can you also update the PR title/description?

@Zoxc Zoxc changed the title Add a nocycle attribute for queries which indicates that they cannot result in cycle errors Add a fatal_cycle attribute for queries which indicates that they will cause a fatal error on query cycles Feb 10, 2018
@bors
Copy link
Collaborator

bors commented Feb 13, 2018

⌛ Testing commit e236994 with merge dbc75ddfb1e9515b08a941111938dbf570dc5911...

@bors
Copy link
Collaborator

bors commented Feb 13, 2018

💔 Test failed - status-travis

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 13, 2018
@kennytm
Copy link
Member

kennytm commented Feb 13, 2018

@bors retry #48116

@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 Feb 13, 2018
@bors
Copy link
Collaborator

bors commented Feb 13, 2018

⌛ Testing commit e236994 with merge d5e730fbb979a1923adc034005740d3b8cbcc0d1...

@bors
Copy link
Collaborator

bors commented Feb 13, 2018

💔 Test failed - status-appveyor

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 13, 2018
@kennytm
Copy link
Member

kennytm commented Feb 13, 2018

@bors retry

3 hour timeout

@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 Feb 13, 2018
@bors
Copy link
Collaborator

bors commented Feb 13, 2018

⌛ Testing commit e236994 with merge dc971d16ed76f9e9485b642cd90616a24de23640...

@bors
Copy link
Collaborator

bors commented Feb 13, 2018

💔 Test failed - status-appveyor

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 13, 2018
@pietroalbini
Copy link
Member

[01:55:45] failures:
[01:55:45]     [compile-fail] compile-fail\rfc-2126-extern-in-paths\single-segment.rs
[01:55:45]     [compile-fail] compile-fail\use-keyword.rs
[01:55:45]     [compile-fail] compile-fail\use-mod-2.rs

cc #48116 @kennytm

@kennytm
Copy link
Member

kennytm commented Feb 13, 2018

@bors retry #48116

@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 Feb 13, 2018
@bors
Copy link
Collaborator

bors commented Feb 17, 2018

⌛ Testing commit e236994 with merge 554fe71...

bors added a commit that referenced this pull request Feb 17, 2018
Add a `fatal_cycle` attribute for queries which indicates that they will cause a fatal error on query cycles

This moves us towards the goal of having cycle errors be non-fatal by not relying on the default implementation of `ty::maps::values::Value` which aborts on errors.

r? @nikomatsakis
@bors
Copy link
Collaborator

bors commented Feb 17, 2018

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

@bors bors merged commit e236994 into rust-lang:master Feb 17, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants