revert #48337 (--explain usage note)#48621
Closed
zackmdavis wants to merge 3 commits intorust-lang:masterfrom
Closed
revert #48337 (--explain usage note)#48621zackmdavis wants to merge 3 commits intorust-lang:masterfrom
--explain usage note)#48621zackmdavis wants to merge 3 commits intorust-lang:masterfrom
Conversation
This reverts commit 1dc2015, which switched to compiling the UI tests twice rather than extracting the humanized output from a field in the JSON output. A conflict in this revert commit had to be fixed manually where changes introduced in rust-lang#48449 collide with the change we're trying to revert (from rust-lang#48337). This is in the matter of rust-lang#48550. Conflicts: src/tools/compiletest/src/runtest.rs
This reverts commit 9b597a1. That commit added a message informing users about the `rustc --explain` functionality inside of a `Drop` implementation for `EmitterWriter`. In addition to exhibiting questionable semantics (printing a hopefully-helpful message for the user does not seem like a "cleanup" action), this resulted in divergent behavior between humanized output and JSON output, because the latter actually instantiates an `EmitterWriter` for every diagnostic. Several conflicts in this revert commit had to be fixed manually, again owing to code-area collision between rust-lang#48449 and rust-lang#48337. This is in the matter of rust-lang#48550. Conflicts: src/librustc_errors/emitter.rs
rust-highfive
added
the
S-waiting-on-review
This is in the matter of rust-lang#48550.
zackmdavis
force-pushed
the
ui_test_efficiencies
branch
from
84588c1 to
a46a511
Compare
Member
|
I'm not a big fan of a revert but if we're in a hurry, we don't have a better idea... I have a work in progress to fix this which would allow to just make it work with json output. |
Contributor
Ok, let's wait for " |
petrochenkov
added
S-blocked
Member
|
The only remaining issue for the fix is some invalid error emission but otherwise it's almost done. |
Contributor
Author
|
@GuillaumeGomez awesome, let's close this in favor of #48684 💖 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See discussion on #48550 for justification.
(As the commit messages note, there were some conflicts; this wasn't a sheerly mechanical revert. But this should—pending Travis's verdict—get us back to the old extract-rendered-from-JSON UI testing regime, after which an
--explainusage note can be reimplemented in a cleaner way at leisure.)cc @GuillaumeGomez @estebank
r? @petrochenkov