-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Improve query cycle error message #49950
Conversation
src/bootstrap/builder.rs
Outdated
@@ -312,7 +312,7 @@ impl<'a> Builder<'a> { | |||
tool::RustInstaller, tool::Cargo, tool::Rls, tool::Rustdoc, tool::Clippy, | |||
native::Llvm, tool::Rustfmt, tool::Miri, native::Lld), | |||
Kind::Check => describe!(check::Std, check::Test, check::Rustc), | |||
Kind::Test => describe!(test::Tidy, test::Bootstrap, test::Ui, test::RunPass, | |||
Kind::Test => describe!(test::Tidy,/* test::Bootstrap,*/ test::Ui, test::RunPass, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spurious change. I should investigate...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bootstrap tests look for cc
, which do not exist on Windows and I need to run tests locally to update them.
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This now changes query cycle error messages. For the following file: trait A: B {}
trait B: C {}
trait C: B {}
fn main() {} It will output:
It was previously:
|
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
src/test/ui/issue-23302-1.rs
Outdated
@@ -11,7 +11,7 @@ | |||
// Check that an enum with recursion in the discriminant throws | |||
// the appropriate error (rather than, say, blowing the stack). | |||
enum X { | |||
A = X::A as isize, //~ ERROR E0391 | |||
A = X::A as isize, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These annotations should not be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error doesn't have a span, so compiletest just ignores them here :/
src/librustc/ty/maps/plumbing.rs
Outdated
for &QueryInfo { span, ref query, .. } in &stack[1..] { | ||
err.span_note(self.sess.codemap().def_span(span), | ||
&format!("...which then requires {}...", query.describe(self))); | ||
let mut err = struct_err!(self.sess, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could just use the span of stack[0]
, since that's the one mentioned in the error message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to print the same span twice. The message is quite noisy already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can start the iteration at i=1, the it's not printed twice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should avoid diagnostics without a main span. If verbosity is the problem, lets rewrite the diagnostic so that the redundant span note is turned into a note for the main span. This might require a complete rewrite of the text to make sense, specially in cases where a chain of causality is tried to be shown and explained.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I can get away with just shifting the spans around without changing any messages. The text sufficiently unclear to allow that...
I changed the error message to it now emits:
Could we get rid of the
It doesn't seem terribly useful and would be annoying to make deterministic for parallel rustc. You can get the full query stack to the cycle by setting |
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The |
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
It may also make sense to mark queries as public, and require a string description of those. Non-public queries could then be stripped from query cycles presented to users. So this error:
Could become:
|
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how the output looks. Left some drive by nitpicks.
--> $DIR/issue-36163.rs:11:18 | ||
| | ||
LL | const A: isize = Foo::B as isize; | ||
| ^^^^^^ | ||
= note: ...which then again requires const-evaluating `Foo::B::{{initializer}}`, completing the cycle. | ||
note: ...which requires computing layout of `Foo`... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be pointing at line 13?
note: ...which requires computing layout of `Foo`...
|
LL | enum Foo {
| ^^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The computing layout of ...
(layout_raw) query has no default span and no span is provided. So there isn't a span to print.
| ^ cyclic reference | ||
| | ||
note: the cycle begins when const checking if rvalue is promotable to static `A`... | ||
error[E0391]: cycle detected when const checking if rvalue is promotable to static `A` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit jargon heavy. @nikomatsakis what would you recommend? I'm thinking something along the lines of
error[E0391]: cycle detected when taking `A` as static
--> $DIR/issue-23302-3.rs:11:1
|
LL | const A: i32 = B; //~ ERROR cycle detected
| ^^^^^^^^^^^^^^^^^
|
note: ...which requires checking which parts of `A` could be static...
--> $DIR/issue-23302-3.rs:11:16
|
LL | const A: i32 = B; //~ ERROR cycle detected
| ^
note: ...which requires taking `B` as static...
--> $DIR/issue-23302-3.rs:13:1
|
LL | const B: i32 = A;
| ^^^^^^^^^^^^^^^^^
note: ...which requires checking which parts of `B` could be static...
--> $DIR/issue-23302-3.rs:13:16
|
LL | const B: i32 = A;
| ^
= note: ...which again requires taking `A` as static, completing the cycle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea what taking as static
or being static
means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From
cycle detected when const checking if rvalue is promotable to static `A`
I understand
cycle detected when [checking during compilation] if [the value being assigned] is [able to be stored to `const A` in static memory instead of stack slots]
That is too long for an error message, IMO, and I might also be wrong. That being said, not everybody, and certainly no newcomers know what "const checking", "rvalue" and "promotable to static" mean.
@nrc, @eddyb, @steveklabnik would you have a suggestion on a message with less jargon for this error?
That being said, I wouldn't block this PR on this at all.
--> $DIR/issue-23302-2.rs:14:9 | ||
| | ||
LL | A = Y::B as isize, //~ ERROR E0391 | ||
| ^^^^ | ||
= note: ...which then again requires const-evaluating `Y::A::{{initializer}}`, completing the cycle. | ||
| | ||
note: ...which requires computing layout of `Y`... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this note should have a span pointing at line 13.
--> $DIR/issue-23305.rs:15:12 | ||
| | ||
LL | impl ToNbt<Self> {} | ||
| ^^^^ cyclic reference | ||
| ^^^^ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to remove the label in the primary span?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There isn't a reason to print it just on the first span, since it's a cycle. It should be on all the spans or none of the spans.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly, at the moment it is impossible to have span labels on anything other than the primary diagnostic. All note/help sections can only have spans without labels. (Not that it would improve this diagnostic.)
if span == DUMMY_SP && stringify!($name) != "def_span" { | ||
span = key.default_span(tcx); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh, so much nicer! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot the remove the other copy of this code in try_get_with
, that will be gone too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the [WIP]
from the title once you're ready for merging.
@bors r+ |
📌 Commit 9cbe3b7 has been approved by |
Improve query cycle error message r? @michaelwoerister
☀️ Test successful - status-appveyor, status-travis |
@Mark-Simulacrum Is http://perf.rust-lang.org/?start=9a59133c09980122ce026d20e7832d63b106a927&end=b91e6a2672a6f69e404d87fa62a5900a390622cf&absolute=true&stat=instructions%3Au the right URL to look at the performance impact here? |
I believe so, yes. |
I don't think the order in which the cycle is reported is deterministic. I'm getting random failures of the ui tests
where the order of the reported stack keeps changing between builds. |
I don't see any sources of non-determinism here. @oli-obk Do you have the output of these tests? |
one example: ---- [ui] ui/issue-23302-2.rs stdout ----
diff of stderr:
- error[E0391]: cycle detected when processing `Y::A::{{initializer}}`
+ error[E0391]: cycle detected when const-evaluating `Y::A::{{initializer}}`
2 --> $DIR/issue-23302-2.rs:14:9
3 |
4 LL | A = Y::B as isize, //~ ERROR E0391
- | ^^^^^^^^^^^^^
+ | ^^^^
6 |
- = note: ...which again requires processing `Y::A::{{initializer}}`, completing the cycle
- note: cycle used when const-evaluating `Y::A::{{initializer}}`
- --> $DIR/issue-23302-2.rs:14:9
- |
- LL | A = Y::B as isize, //~ ERROR E0391
- | ^^^^^^^^^^^^^
+ note: ...which requires computing layout of `Y`...
+ = note: ...which again requires const-evaluating `Y::A::{{initializer}}`, completing the cycle |
Warn on all erroneous constants fixes #49791 fixes #47054 @Zoxc this PR triggers the nondeterministic errors of #49950 (comment) really often (at least on stage1).
@Mark-Simulacrum The results here look pretty green, but incomplete. What is up with that? |
I am uncertain; it's possible the left commit had some error on the server which lead to it being poorly benchmarked. I can try to rerun it if it's critical, please ping me if so. |
r? @michaelwoerister