Minimum lint levels for C-future-compatibility issues: take two#68501
Closed
Aaron1011 wants to merge 6 commits intorust-lang:masterfrom
Closed
Minimum lint levels for C-future-compatibility issues: take two#68501Aaron1011 wants to merge 6 commits intorust-lang:masterfrom
Aaron1011 wants to merge 6 commits intorust-lang:masterfrom
Conversation
Contributor
|
r? @davidtwco (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
Contributor
|
The job 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 |
Contributor
|
Some notes:
|
Centril
reviewed
Jan 24, 2020
Comment on lines
+338
to
+352
| // If this code originates in a foreign macro, aka something that this crate | ||
| // did not itself author, then it's likely that there's nothing this crate | ||
| // can do about it. We probably want to skip the lint entirely. | ||
| if err.span.primary_spans().iter().any(|s| in_external_macro(sess, *s)) { | ||
| // Any suggestions made here are likely to be incorrect, so anything we | ||
| // emit shouldn't be automatically fixed by rustfix. | ||
| err.allow_suggestions(false); | ||
|
|
||
| // If this is a future incompatible lint it'll become a hard error, so | ||
| // we have to emit *something*. Also allow lints to whitelist themselves | ||
| // on a case-by-case basis for emission in a foreign macro. | ||
| if future_incompatible.is_none() && !lint.report_in_external_macro { | ||
| err.cancel() | ||
| } | ||
| } |
Contributor
Author
There was a problem hiding this comment.
I'm not sure what you mean.
Contributor
There was a problem hiding this comment.
Looking at the diff, it seems like this code is now duplicated from earlier in struct_span_level (take a look at "view file"), so I'm wondering if you inadvertently brought old code in with the forward port.
davidtwco
reviewed
Jan 30, 2020
Member
davidtwco
left a comment
davidtwco
left a comment
There was a problem hiding this comment.
Implementation looks good to me. I don't have an opinion on the behaviour.
|
|
||
| /// Lint level was set by an attribute. | ||
| Node(Symbol, Span, Option<Symbol> /* RFC 2383 reason */), | ||
| Node(Symbol, Option<Level>, Span, Option<Symbol> /* RFC 2383 reason */), |
Member
There was a problem hiding this comment.
Perhaps this (and the CommandLine variant) should be made into struct variants with named fields for clarity?
| // Add notes about minimum levels and what the user should do here: | ||
| err.note(&format!("the minimum lint level for `{}` is `{}`", name, min_level.as_str())) | ||
| .note(&format!("the lint level cannot be reduced to `{}`", level_str)) | ||
| .help(&format!("remove the #[{}({})] directive", level_str, name)); |
Member
There was a problem hiding this comment.
I can't see any tests that show this help message - was wondering if it could be a suggestion?
Collaborator
|
☔ The latest upstream changes (presumably #68080) made this pull request unmergeable. Please resolve the merge conflicts. |
JohnCSimon
added
S-waiting-on-author
|
Triaged |
|
Closing this after a discussion with the author :) |
Dylan-DPC-zz
added
S-inactive
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
8 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.
This is a revival of #59658.
For now, I've just rebased against
master, and tweaked the output (e.g. added in backticks) to remove unnecessary churn.It doesn't look like the discussion in #59658 arrived at a consensus as to what the exact behavior of this should be. The main options seem to be:
cap-lint): do not suppress any warning messages, and additional emit a warning whenever#[allow]is applied to the future-compat lint. This is the most verbose option, and seems quite spammy.cap-lint): emit a single message per lint type, possibly pointing to the span where it occurs.I'm interested in getting this infrastructure merged in order to support #68350 (never-type fallback lint)., I'd like the never-type fallback lint to be deny-by-default while piercing
cap-lints.