Avoid repetition on “use of unstable library feature 'rustc_private'” #45540
Conversation
…vate'” error is very repetitive
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
kennytm
added
the
S-waiting-on-review
|
|
kennytm
added
S-waiting-on-author
|
Yes, I've seen it now, sorry for that; I didn't have time to look at it/ fix it yet. |
…s long as the same error message has not been used before (i.e. use the None as the span key, for errors that occur within macros)
src/librustc/middle/stability.rs
Outdated
|
|
||
| let msp: MultiSpan = span.into(); | ||
| let cm = &self.sess.parse_sess.codemap(); | ||
| let span_key = |
There was a problem hiding this comment.
You can probably fit the following line on this one.
src/librustc/middle/stability.rs
Outdated
| let cm = &self.sess.parse_sess.codemap(); | ||
| let span_key = | ||
| msp.primary_span().and_then(|sp:Span| | ||
| if sp != DUMMY_SP { |
There was a problem hiding this comment.
Clean up the formatting for this entire block to avoid as having as much left padding.
src/librustc/middle/stability.rs
Outdated
| } | ||
| ); | ||
|
|
||
| let tuple = (None, span_key, msg.clone()); |
There was a problem hiding this comment.
Try to give tuple a more descriptive name. You can also get around that by just inserting the tuple inline ;)
src/librustc/middle/stability.rs
Outdated
| msp.primary_span().and_then(|sp:Span| | ||
| if sp != DUMMY_SP { | ||
| let fname = cm.lookup_char_pos(sp.lo()).file.as_ref().name.clone(); | ||
| if fname.starts_with("<") && fname.ends_with(" macros>") { |
There was a problem hiding this comment.
This piece of code could be better as a method of cm.lookup_char_pos(sp.lo()).file.
There was a problem hiding this comment.
Not sure what you meant. Hopefully the way it's written now is better (well, it is better, I don't exactly know why I felt I needed to make it so complicated... )
src/librustc/session/mod.rs
Outdated
| @@ -78,7 +78,7 @@ pub struct Session { | |||
| /// Set of (LintId, Option<Span>, message) tuples tracking lint | |||
| /// (sub)diagnostics that have been set once, but should not be set again, | |||
| /// in order to avoid redundantly verbose output (Issue #24690). | |||
| pub one_time_diagnostics: RefCell<FxHashSet<(lint::LintId, Option<Span>, String)>>, | |||
| pub one_time_diagnostics: RefCell<FxHashSet<(Option<lint::LintId>, Option<Span>, String)>>, | |||
There was a problem hiding this comment.
Normally, instead of making the first tuple field an option, I would suggest making the FxHashSet accept a new struct with three fields diagnostic_id, span and message, with diagnostic_id being a new enum that takes either a LintId or a GateIssue. This way we can keep expanding one_time_diagnostics without the risk of having multiple different diagnostics with the same span message and no lint_id, but being different errors, from being incorrectly stopped, but because of the message being there, what you're doing is correct and unlikely to be made incorrect with a subsequent change.
estebank
changed the title
src/librustc/session/mod.rs
Outdated
| @@ -361,7 +368,7 @@ impl Session { | |||
| }, | |||
| _ => { | |||
| let lint_id = lint::LintId::of(lint); | |||
| let id_span_message = (lint_id, span, message.to_owned()); | |||
| let id_span_message = (DiagnosticMessageId::LintId(lint_id), span, message.to_owned()); | |||
There was a problem hiding this comment.
Please shorten this line to ≤100 chars.
[00:02:54] tidy error: /checkout/src/librustc/session/mod.rs:371: line longer than 100 chars
|
@bors r=estebank |
|
📌 Commit bb0049b has been approved by |
Avoid repetition on “use of unstable library feature 'rustc_private'” This PR fixes the error by only emitting it when the span contains a real file (is not inside a macro) - and making sure it's emitted only once per span. The first check was needed because spans-within-macros seem to differ a lot and "fixing" them to the real location is not trivial (and the method that does this is private to another module). It also feels like there always will be an error on import, with the real file name, so not sure there's a point to re-emit the same error at macro use. Fix #44953.
kennytm
added
S-waiting-on-bors
|
☀️ Test successful - status-appveyor, status-travis |
|
@virgil-palanciuc, may be worth adding a test? |
It tested rust-lang#44953. `log` macros in newer versions are no longer recursive, so these duplicated error messages (about unstable feature uses) previously occurring at each level of recursion are no longer possible, even with the fix by rust-lang#45540. Furthermore this test breaks when multiple versions of `log` are in the sysroot (`log 0.3.9` depends on`log 0.4.1`)
This PR fixes the error by only emitting it when the span contains a real file (is not inside a macro) - and making sure it's emitted only once per span.
The first check was needed because spans-within-macros seem to differ a lot and "fixing" them to the real location is not trivial (and the method that does this is private to another module). It also feels like there always will be an error on import, with the real file name, so not sure there's a point to re-emit the same error at macro use.
Fix #44953.