feat: New lints deprecated_attributes_without_(since|note)

[hashcatHitman]

View all comments

Adds two new lints:

• deprecated_attributes_without_since: Requires filling out the since field when using the deprecated attribute. This is pedantic, making it allow by default.
• deprecated_attributes_without_note: Requires filling out the note field when using the deprecated attribute. This is restriction, making it allow by default.

Why `pedantic` and `restriction`?

When allow_attributes_without_reason was originally added, it was put into the restriction category due to the impact it would have on existing projects. (See: #8504).

So taking that into consideration:

• deprecated_attributes_without_since is pedantic because it is the easier of the two to satisfy (despite appearing to be the more prevalent issue in the wild at the moment). When deprecating an item, just include the version of your crate you are deprecating it in, or if you don't know what version that will be yet, "TBD" is also accepted by the since field. When trying to fix this lint retroactively, any project that still has a git history should be able to find the correct value for this field fairly easily using git tag --contains, as was done in chore: add missing since versions to #[deprecated] attributes apache/datafusion#24544.
• deprecated_attributes_without_note is restriction because it requires more involvement when fixing. The note field can be used for any information, but it is usually used to inform crate users why an item was deprecated and what they should use instead. Knowing "why" in the moment isn't too difficult, but finding an alternative may be. When trying to fix this lint retroactively, it can be extremely difficult, if not impossible, to determine why a given item was deprecated. The deprecated attribute has always had the ability to document the reason; if a project has been declining to do so, it's entirely possible they haven't been doing a very good job of documenting the reason anywhere at all. Who knows why foo() was deprecated 3 years ago? Not me. You can't really fill this field out "incorrectly" since it can be used to store any arbitrary message, but I think we'd generally prefer that people explicitly opt-in to this lint and choose to actually include useful information. The other lint is technically opt-in too, but enabling pedantic as a group is not nearly as uncommon as enabling restriction as a group, which we very emphatically tell users not to do. The other lint is also stricter about it's value, which lends itself well to being easier to deal with correctly. There is even an existing correctness lint (clippy::deprecated_semver) which checks that the value in the since field is either "TBD" or a valid semantic version.

Personally, I think I'd rather both be pedantic, since I don't think deprecated is even used that much to begin with (certainly not as much as allow was). But I suspect the categories chosen here will ultimately be preferred. If I'm wrong, I expect it to come up in the non-community review and/or final comment period.

I think the naming conventions might technically prefer names like:

• deprecated_attributes_without_since_fields
• deprecated_attributes_without_notes or deprecated_attributes_without_note_fields

But I went against my understanding of the naming conventions because of the very similarly named allow_attributes_without_reason. If this seems wrong, just let me know which names are preferred.

Fixes #17595

changelog: [deprecated_attributes_without_since]: New pedantic lint enforcing use of the since field on deprecated attributes
changelog: [deprecated_attributes_without_note]: New restriction lint enforcing use of the note field on deprecated attributes

[rustbot]

Thanks for the pull request. A reviewer will take a look after it receives 2 community reviews.

In the meantime, we would highly appreciate if you could try to review any of PRs waiting on community reviews.

[github-actions]

Lintcheck changes for 41f2591

Lint	Added	Removed	Changed
clippy::deprecated_attributes_without_note	5	0	0
clippy::deprecated_attributes_without_since	63	0	0

This comment will be updated if you push new changes

[CommanderStorm]

community review: I am not quite happy with how this is tested. Seems rather weird and sloppily done, so please rework that part.

The actual lint is fairly simple, but I have some questions there too.

View changes since this review

[CommanderStorm]

Community review: Lgtm

Two things that I am iffy about:

• not sure if the "to the end" in the diagnostic can not just be removed. To me it just makes the disgnostid longer, but 🤷‍♂️
• I am surprised to see the no-rustfix. I though hasplaceholders cannot auto apply, so --fix does not apply this.. you sure this is nessary?

View changes since this review

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[hashcatHitman]

• not sure if the "to the end" in the diagnostic can not just be removed. To me it just makes the disgnostid longer, but 🤷‍♂️

Yeah, I think that's a fair point. I've dropped that part, it's more concise now.

• I am surprised to see the no-rustfix. I though hasplaceholders cannot auto apply, so --fix does not apply this.. you sure this is nessary?

I was surprised to need it too, but yes, it certainly seems necessary. Without it, running cargo uibless would generate .fixed files and fail with messages like these:

error: test got exit status: 1, but expected 0
 --> tests/ui/deprecated_attributes_without_note.fixed:2:9
  |
2 | #![deny(clippy::deprecated_attributes_without_note)]
  |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ after rustfix is applied, all errors should be gone, but weren't
  |

Anyway, thanks for your feedback and help! I think these lints are in a much better condition now.