Repository navigation
Lint #[deprecated] on use items, where it has no effect - #165842
johncarmack1984 wants to merge 1 commit into
Conversation
rustbot
added
A-allocators
|
|
rustbot
added
S-waiting-on-review
|
Thanks for the pull request, and welcome! The Rust Project has assigned @WaffleLapkin (or someone else) to review your changes, you should hear from them (or someone else) within the next two weeks. Please see the contribution instructions and our LLM policy for more information. |
This comment has been minimized.
This comment has been minimized.
Deprecation is checked on the item a path resolves to, so a `#[deprecated]` attribute on a `use` item is accepted and never consulted at use sites. Deprecating a re-export that way warns nobody. Add `ineffective_deprecated_reexports`, a warn-by-default lint emitted from the stability checker for every `use` item carrying `#[deprecated]`. Its note says this is a long-standing bug and links the tracking issue; its help suggests deprecating the item itself, or a `#[deprecated]` type alias or wrapper function where one applies. core (`alloc::LayoutErr`) and std (`collections::Bound`) each carry one of these attributes. Both get an item-level `#[expect]`, which std's deny-warnings build turns into an error once the lint stops firing there, so the annotations go away with the fix for deprecating re-exports. The run-rustfix import test carries its expectation on the item too: a crate-level one is unfulfilled in the fixed output, which removes the annotated `use`.
ee5fc9e to
0c50040
Compare
|
What exactly is the LLM involvement here? What does "I wrote this lint" entail? |
Hi, first of all, all user-facing components including PR description should be written manually from start to finish; otherwise, this violates our policy. Second, you should have found a pre-arranged LLM reviewer in #llm-reviews before opening a PR. More you can read in our LLM policy. |
| find_attr!(self.tcx, item.owner_id.def_id, Deprecated { span, .. } => *span) | ||
| { | ||
| self.tcx.emit_node_span_lint( | ||
| INEFFECTIVE_DEPRECATED_REEXPORTS, |
There was a problem hiding this comment.
This is better implemented by making this line here Warn instead of Allow, rather than here.
That would not make it its own lint, and rather a variant of UNUSED_ATTRIBUTES, which all other invalid targets also are.
There was a problem hiding this comment.
I tried this on the PR head, and found check_target routes a misplaced deprecated to useless_deprecated unless the target is an expression, closure, statement, arm or macro call, so Warn(Target::Use) gives this, deny-by-default:
error: the `deprecated` attribute cannot be used on use statements
= help: the `deprecated` attribute can be applied to associated consts, [...] and type aliases
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: `#[deny(useless_deprecated)]` on by default
core stops building at LayoutErr, and every crate with one of these fails the same way. It's the same error #142731's review asked not to have. Given the attribute is wanted (it just doesn't work yet), with this change the text would say the opposite of where #30827 would take it.
Adding Target::Use to that exclusion list gets unused_attributes instead, but the message still says "cannot be used" and "being phased out". Changing those would mean a deprecated-on-use special case inside the generic target checker.
A lint of its own is what #161178 did for #[unstable] on re-exports, so I went the same way to follow the precedent I could find for this kind of case. If you would rather I write it as a special case in the target checker, I'll move it there.
I believe this part violates the LLM policy as well. |
JonathanBrouwer
added
the
llm-assisted
It means only that; I sat down in front of my code editor in mid-October and thought through the problem one character at a time until it was solved.
The PR description and docs are my own writing. The LLM was used for correctness checks when drafting, but the prose is my own. I address this on the Zulip thread, and I apologize that it makes things hard for reviewers, but when trying to be thorough and clear, I actually do sound a lot like an LLM. And I miss being able to use em-dashes and the word “delve”. Would be happy to give you a call on zoom/teams/FaceTime if that sounds hard to believe. I get very excited and agreeable and try to sound like everyone’s favorite Reddit commenter from 2009. It only became a problem in 2023.
That… is fair. Oversight on my part. I can do that now.
The LLM was used when choosing between approaches (and I know this sounds ridiculous but it’s what I did to stay in accordance with the policy): I would have the tool create implementations in a folder I refused to look at, based on instructions I gave it, and have it run tests & benchmarks to ensure given approaches would not create regressions, unexpected drawbacks, etc.. If a given approach did not have inherent flaws under this system, I would then go about hand-typing a naive implementation from the prompt I had previously given the LLM. The submitted code in this PR was hand-typed by a human without significant portions being written by an LLM. |
To clarify the policy, I don't see you've missed anything here. As far as I can tell with your latest explanations here (thank you for those), you did not use LLMs in a way that would run into the need for pre-arranging reviewers. |
|
Could you please rewrite the disclosure in the PR description to reflect the actual workflow? As it stands, some points contradict each other and are too ambiguous. |
Done. I believe the new version removes the ambiguities and resolves the contradictions they created, open to feedback if it still seems otherwise. |
#[deprecated]on auseitem is accepted and then ignored: deprecation is checked on the resolved item, not on the imports a path went through, so deprecating a re-export that way warns nobody (#30827).This adds
ineffective_deprecated_reexports, a warn-by-default lint that says so at the attribute, notes that it's a long-standing bug, and suggests what does work: deprecating the item itself, or a#[deprecated]type alias or wrapper function where one applies.Choices made:
useless_deprecatedis deny-by-default, and the review on Add lint rule for#[deprecated]on re-exports (rebase) #142731 asked for a warning instead of an error.rustc_passes, next toineffective_unstable_reexports(lint ineffective #[unstable] annotations on re-exports #161178).use Alias::Variant, a function needs a wrapper, and a module or macro re-export has no equivalent.alloc::LayoutErr) and std (collections::Bound) each carry one of these attributes. Both get an item-level#[expect], which std's deny-warnings build turns into an error once the lint stops firing there; core gets a FIXME, std already had one. The run-rustfix test intests/ui/importscarries its expectation on the item: a crate-level one is unfulfilled in the fixed output, which deletes the annotateduse.The fix for #30827 should delete this lint.
Supersedes #142731 (I asked GrigorenkoPV there on Oct 22 whether they planned to return to it and haven't heard back). Closes #142436.
r? @WaffleLapkin
LLM disclosure: I wrote this lint. Claude Code (Fable 5.1, then Opus 5.5, both max effort) were used to evaluate design options from the issue's history for inherent virtues or flaws, after which I drafted the implementation, docs and tests, while Claude ran the test suites. Codex reviewed. Before posting, I pared and re-reviewed every line myself.