Deduplicate compiler diagnostics. by ehuss · Pull Request #9675 · rust-lang/carg...
source link: https://github.com/rust-lang/cargo/pull/9675
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
This adds some logic to deduplicate diagnostics emitted across rustc invocations. There are some situations where different targets can emit the same message, and that has caused confusion particularly for new users. A prominent example is running cargo test which will build the library twice concurrently (once as a normal library, and once as a test).
As part of this, the "N warnings emitted" message sent by rustc is intercepted, and instead cargo generates its own summary. This is to prevent potentially confusing situations where that message is either deduplicated, or wrong. It also provides a little more context to explain exactly what issued the warnings. Example:
warning: `foo` (lib) generated 1 warning
This only impacts human-readable output, it does not change JSON behavior.
Closes #9104
(rust-highfive has picked a reviewer for you, use r? to override)
I'm opening this in Draft status because there are some additional things to consider and change:
-
Which hashing algorithm to use? Rustc itself uses SipHash 128 (here). Cargo could use the same (either copying the implementation here, or using siphasher from crates.io which I understand is essentially a copy from rustc, or publishing a new package). Or, we could investigate using a different algorithm. There is significant discussion of alternatives at rust-lang/rust#41215 and rust-lang/rust#51054. An often cited alternative is HighwayHash. Or we could use any stronger hash like blake3, I'm not sure if the performance here is really critical.
-
The detection for rustc's summaries ("N warnings detected") is very primitive, checking the raw message string. In theory, this could cause a false positive, though I don't see how. Should this be blocked on a stronger mechanism?
-
I have not put significant thought into the wording of the new warning summary. I'm not sure how it works out.
-
The testsuite is below my regular level of thoroughness, but I can't particularly think of interesting things to test (maybe more rustdoc tests? maybe examples with hard errors?).
-
The "could not compile" error message doesn't mention duplicates. I'm not sure if it is really important.
I've been personally wary historically of changes like this since it makes Cargo/rustc's relationship more bespoke over time and in theory makes it more difficult to integrate with other build systems that don't use Cargo. Arguably, though, that already happens in a major way for pipelining and I see that there's already a lot of interception of messages on Cargo's part. I think it's basically something to just be aware of at this point. I don't really want to cross a line where there's a fundamental change in how rustc/Cargo interact, but if it's just performance improvements or diagnostic tweaks that doesn't feel fundamental as to cause issues elsewhere.
Which hashing algorithm to use?
Is a HashSet<String> too prohibitive?
The detection for rustc's summaries ("N warnings detected") is very primitive
I think it would be best to improve this over time (in addition to improving the detection of "aborting due to..."). I don't think it needs to block this though. I would hope that it's not too too hard to add some sort of a descriptive ID or something to select compiler diagnostics.
Overall I think this seems fine to land, and honestly I think it's pretty valuable to fix this since it trips up so many newcomers.
Is a
HashSet<String>too prohibitive?
I was thinking of all the work put in to #6289, #7737, and #7838 to avoid buffering all output, and to reduce Cargo's memory footprint. That mostly only comes up in extreme cases (like using RUSTC_LOG which isn't relevant here, or use cases like #7736). I'd be comfortable using HashSet<String> if it had a cap of a few megabytes (and then just stopped adding new entries afterwards). Are you concerned about collisions?
(As an aside, I checked and the typical error message is about 300 bytes, with some outliers being ~15kb).
Nah I'm not too worried one way or another, it just seemed like the simplest thing where we wouldn't worry too much about algorithms and such. I thought the streaming and "don't buffer" support was generally for varied output of rustc/subprocesses and not for the structured output like warnings and such, I wasn't sure if rustc was likely to produce unbounded numbers of warnings as well. Being conservative and keeping a set of u64, though, seems fine.
Otherwise though I wouldn't worry too much about this in terms of hashing. This shouldn't be a hot path and if Cargo's a bit slower generating gigabytes of output that seems like it's not really the end of the world.
OK, I unmarked the draft state.
I did some basic tests against rustc's corpus of diagnostics. Using SipHash 64, out of a total of 34233 diagnostics, there were 12724 duplicate messages (usually things like "aborted due to..."), and 0 hash collisions. I'm currently not feeling motivated to try to grab a different hash algorithm, so I think using SipHash 64 for now will be fine. If someone else feels compelled to try something stronger, it's easy to swap around.
The other concerns don't seem to strong to me. If people report the new summary is confusing, we can tweak it. I used this briefly locally, and I was happy to see the extra information.
Another thought I just had thinking about this, it looks like this is all geared towards warnings but we have the same issues with compiler errors, right? (e.g. if you have a mistake in your code and type cargo test you'll get the error twice)
Is that something we can "easily" handle? If not we may not want to close the original issue since that's a case I at least pretty regularly run into myself.
It works the same for warnings and errors. I added a test demonstrating handling an error.
Ok sound good to me! I've got one minor comment which actually isn't even related to the changes you specifically made here, but otherwise this seems good to go to me.
src/cargo/core/compiler/mod.rs
Outdated
} else {
// Strip only fails if the the Writer fails, which is Cursor
// on a Vec, which should never fail.
strip_ansi_escapes::strip(&error.rendered)
strip_ansi_escapes::strip(&msg.rendered)
.map(|v| String::from_utf8(v).expect("utf8"))
.expect("strip should never fail")
};
if options.show_warnings {
alexcrichton 7 days ago
Member
I'm a little confused about this looking at this again although this isn't an issue introduced by this PR.
Despite its name it appears that this option is controlling both errors and warnings, and I was originally worried that this could sweep errors under the rug. It looks like, though, that this is only set to false when replaying back caches, which if we have a cache we're guaranteed there's no errors.
Perhaps this could instead be rename to show_diagnostics or similar?
ehuss 7 days ago
Author
Contributor
Sure! I had renamed it earlier, but changed it back when I went with a different approach. I found it confusing, too. I think it was originally show_warnings since it only applied to cache replay which doesn't record errors, so warnings was the only thing that mattered. I added a comment to try to explain what it means.
@bors: r+
Commit 5606d1b has been approved by alexcrichton
Testing commit 
Test successful - checks-actions
Approved by: alexcrichton
Pushing 54bc19e to master...
Should this be tagged relnotes? I think it's something worth highlighting as an improvement to the newcomer experience!
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK