Uh oh!
There was an error while loading. Please reload this page.
lint reasons (RFC 2383, part 1) - #54683
Conversation
rust-highfive
commented
Sep 30, 2018
(rust_highfive has picked a reviewer for you, use r? to override) |
There was a problem hiding this comment.
IIRC, you can just use item.ident == "reason".
There was a problem hiding this comment.
Could you add a couple more cases:#[warn(lint1, reason = "zzz", reason = "yyy")] - duplicated reason (probably an error).#[warn(lint1, reason = "zzz", lint2)] - reason is not the last (an error? just to be conservative)
There was a problem hiding this comment.
reason is not the last (an error? just to be conservative)
Sure. I trust this is compliant with the spirit of the RFC? @Centril@myrrlyn
This is also nice from an implementer's perspective—in the first revision of this PR, I felt bad about doing a entire extra iteration over the meta items just to pick up the reason before entering the loop that processes the lint names themselves, but with this restriction, we can just peek at the end.
There was a problem hiding this comment.
No code is good, code is evil.
There was a problem hiding this comment.
I don't think this case will ever be hit realistically, so I see this as an immediate technical debt.
TimNN
commented
Oct 9, 2018
Ping from triage @zackmdavis. It looks like some changes have been requested to your PR. Is this PR blocked on #54926? |
zackmdavis
commented
Oct 9, 2018
@TimNN No, I just suffer from a psychological defect in which composing shiny new PRs is more fun than addressing reviews of outstanding PRs (and the microincentives of which tasks are more fun play a disproportionate role in determining what humans will actually accomplish in general, not just as an open-source volunteer); give me a few more days |
bors
commented
Oct 11, 2018
☔ The latest upstream changes (presumably #54969) made this pull request unmergeable. Please resolve the merge conflicts. |
e5ec30d to
f575218Comparezackmdavis
commented
Oct 12, 2018
(My local build is mysteriously failing right now.) |
rust-highfive
commented
Oct 12, 2018
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 |
bors
commented
Oct 14, 2018
☔ The latest upstream changes (presumably #55015) made this pull request unmergeable. Please resolve the merge conflicts. |
zackmdavis
commented
Oct 15, 2018
"Mysterious", bah! (This was actually a really idiotic bug where the reslicing that I meant to do when we found the |
f575218 to
0ddf461Comparezackmdavis
commented
Oct 15, 2018
@petrochenkov updated 🏁 |
| } else { | ||
| reason = Some(rationale); | ||
| } | ||
| let tail_li = &metas[metas.len()-1]; |
There was a problem hiding this comment.
This will panic on #[allow()], no?
There was a problem hiding this comment.
Should this trigger unused_attributes? (As recently discussed for empty derives and inappropriate #[must_use])
There was a problem hiding this comment.
Feels reasonable to me to trigger unused_attributes for empty allow; esp. if we do it for empty derives.
| #![warn(keyword_idents, reason = "root in rubble", macro_use_extern_crate)] | ||
| //~^ ERROR malformed lint attribute | ||
| //~| HELP reason in lint attribute must come last | ||
| #![warn(missing_copy_implementations, reason)] |
There was a problem hiding this comment.
What happens with #[allow(reason = "foo")]?
I.e. no lints to allow, only the reason.
There was a problem hiding this comment.
Yeah, let's make this an error (it would have been a no-op if you hadn't pointed this out).
There was a problem hiding this comment.
... well, actually, I guess I could see a case that, if empty lint attributes (#[allow()] as discussed above) aren't an error (the stable behavior), then we should also allow reason-only lint attributes for consistency/uniformity.
(Presumably the only sane reason for empty lint attributes to exist is for the sake of the base case of some recursive macro, and if we care about that, then we should also care about a macro that's otherwise identical but also includes a reason.)
0ddf461 to
fecc001Comparezackmdavis
commented
Oct 16, 2018
I took a shot at implementing unused-attributes linting for empty and reason-only lint attributes, but I didn't understand some of the behavior I was seeing (the first empty lint attribute in a program was getting linted twice for some reason), so I've elected to defer that, filing an issue (#55112) and leaving FIXME comments. (I could have hacked around the duplication with one-time-diagnostics, but that would be bad; if you want to write correct software and you don't understand something, don't guess.) In this revision, we don't panic on ... I could be persuaded to go with my first instinct and make @petrochenkov what do you think?? |
bors
commented
Oct 18, 2018
💔 Test failed - status-appveyor |
kennytm
commented
Oct 18, 2018
Failed a |
bors
commented
Oct 25, 2018
☔ The latest upstream changes (presumably #54658) made this pull request unmergeable. Please resolve the merge conflicts. |
This is just for the `reason =` name-value meta-item; the `#[expect(lint)]` attribute also described in the RFC is a problem for another day. The place where we were directly calling `emit()` on a match block (whose arms returned a mutable reference to a diagnostic-builder) was admittedly cute, but no longer plausibly natural after adding the if-let to the end of the `LintSource::Node` arm. This regards rust-lang#54503.
We take stability seriously, so we shy away from making even seemingly "trivial" features insta-stable.
Vadim Petrochenkov suggested this in review ("an error? just to be
conservative"), and it turns out to be convenient from the
implementer's perspective: in the initial proposed implementation (or
`HEAD~2`, as some might prefer to call it), we were doing an entire
whole iteration over the meta items just to find the reason (before
iterating over them to set the actual lint levels). This way, we can
just peek at the end rather than adding that extra loop (or
restructuring the existing code). The RFC doesn't seem to take a
position on this, and there's some precedent for restricting things to
be at the end of a sequence (we only allow `..` at the end of a struct
pattern, even if it would be possible to let it appear anywhere in the
sequence).We avoid an ICE by checking for an empty meta-item list before we index into the meta-items, and leave commentary about where we'd like to issue unused-attributes lints in the future. Note that empty lint attributes are already accepted by the stable compiler; generalizing this to weird reason-only lint attributes seems like the conservative/consilient generalization.
zackmdavis
commented
Oct 27, 2018
@kennytm I can't reproduce this (Ubuntu 16.04). (At least not consistently—I remember seeing it once while trying to reproduce the other week, while also struggling with a flaky build, but it's passing for me now, as illustrated by output below.) Please advise? |
fecc001 to
f66ea66Comparekennytm
commented
Oct 27, 2018
@zackmdavis maybe try to test on windows? |
zackmdavis
commented
Oct 27, 2018
... I don't have a Windows machine handy. I would be pretty surprised if there was really a Windows-specific bug to find here (what Windows-specific API could this patch be using??), as opposed to something about the cargo fix test being spuriously nondeterministic (which would be bad, but not something we can fix in this pull request). Is it OK if we try one more time? @bors r=petrochenkov (feel free to r- if you disagree with my judgement) |
bors
commented
Oct 27, 2018
📌 Commit f66ea66 has been approved by |
…r=petrochenkov lint reasons (RFC 2883, part 1) This implements the `reason =` functionality described in [the RFC](https://github.com/rust-lang/rfcs/blob/master/text/2383-lint-reasons.md) under a `lint_reasons` feature gate. 
bors
commented
Oct 28, 2018
lint reasons (RFC 2883, part 1) This implements the `reason =` functionality described in [the RFC](https://github.com/rust-lang/rfcs/blob/master/text/2383-lint-reasons.md) under a `lint_reasons` feature gate. 
bors
commented
Oct 28, 2018
☀️ Test successful - status-appveyor, status-travis |
8573
commented
Jun 7, 2022
Could the RFC number in the ticket title be corrected for ease of searching? Also, is it right that this remains labelled as "S-waiting-on-bors"? |
bjorn3
commented
Jun 7, 2022
Edited the title. As for S-waiting-on-bors, it seems bors keeps it on every merged PR. |
This implements the
reason =functionality described in the RFC under alint_reasonsfeature gate.