-
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
Don't trim paths when printing const eval frame #100144
Conversation
Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri |
(rust-highfive has picked a reviewer for you, use r? to override) |
I don't think this will fix #100114. It makes very little sense that changing the way the path is rendered would affect that regression test. My prediction is that if you undo the |
Ah no the reason it passes now is that you get a "Future incompatibility report". But this does not fix the ICE for rustc invocations that don't print these reports. Note that there still is no regular diagnostic being printed, only the future-incompat-report. That basically circumvents the |
Ah, the original ICE does mention something about path trimming?
The error doesn't even say why that is an error though. Pretty useless. :( Still I am not convinced that we want the verbose paths here. |
I verified that this UI tests fails locally without the compiler changes, and passes with them applied.
Yes, the ICE is because we format a string that uses the trimmed path (e.g. |
Is "good path bug" the same thing as |
@RalfJung I actually didn't even notice that "Future incompatibility report" being printed. We can probably use the fact that that's printed to defuse the trimmed paths bug. |
Ah no "good_path_bug" is something else, a new thing I haven't encountered yet. |
Good path bugs are the opposite of span bugs, they trigger when no diagnostic is emitted (as opposed to triggering when no error is emitted). I can probably just fix the good path bug to consider the fact that a future incompatibility warning is emitted. |
Okay so the proper fix then would be to only do this work when the lint is actually going to be emitted. Your patch as-is will also affect diagnostics we want, and it will affect Miri, so I don't think this is the right patch. |
So they panic! if any error is omitted? ;) (delay_span_bug panics if no error is omitted so that's what I would expect the opposite to be) From the docs, seems like it's like a less aggressive version of delay_span_bug that is also "happy" with just a warning having been emitted. |
@RalfJung, so I'm actually not sure if I agree that the diagnostic changes are regressions. We're already being pretty verbose in this Display impl by printing the file/line/etc., so I'm not sure why printing the full path of the instance is bad? that is, I don't see why
is worse than
The fact that we're currently trimming these paths seems like it was a side-effect of the printing logic having this behavior by default, and not done intentionally? 🤔 |
Why is printing the full path ever bad in any diagnostic? The same reason applies here. |
The job Click to see the possible cause of the failure (guessed by this bot)
|
To put it differently -- yes, our messages are quite verbose. We have been working on reducing that (more in the Miri side than the CTFE side). So I'd appreciate if we didn't make them even more verbose than they already are. :) |
@RalfJung whoops, sorry, yeah I didn't mean opposite. Your understanding is correct and what I meant, didn't know why that wording came to me. The diagnostics concerns w/ miri makes sense, let me try an alternative approach to suppressing this ICE. I have a few options... |
Thanks! Fundamentally the problem is that this code
gets executed even if the lint is allowed. But that file is a mess and refactoring it to delay this work is not worth it, since in a few weeks const_err will become a hard error anyway... |
Yeah, I'm aware of the problem... but if you say it'll be hard error soon, then maybe I won't bother 😄 |
It's a really expensive operation, like +10-50% to whole compile time, that's why there's a special compiler mechanism for preventing it from slipping through. |
r? @RalfJung |
Fixes #100114
Some UI test output is more verbose now, but it's not immediately clear that we should've been trimming those paths anyways. Verbosity in this case is useful, I think.