Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Simplify lint deprecation #13180

Merged
merged 3 commits into from
Aug 5, 2024
Merged

Simplify lint deprecation #13180

merged 3 commits into from
Aug 5, 2024

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Jul 29, 2024

A couple of small changes:

  • A few deprecations were changed to renames. They all had a message similar to "this lint has been replaced by ..." which is just describing a rename.
  • The website and warning message are now the same. The website description was usually just a wordier version that contained no extra information. This can be worked around if needed, but I don't think that will happen.
  • The legacy deprecations have been removed. rustc should handle this since it already suggests adding the clippy:: for all lints (deprecated or not) when they're used without it. It wouldn't be a problem to add them back in.
  • The website no longer has a "view source" link for deprecated lints since they're no longer read from the HIR tree. I could store the line number, but the link seems totally useless for these lints.

This came up as part of separating the internal lints into their own crate. Both the metadata collector and the lint registration code needs access to the deprecated and renamed lists. This form lets all the deprecations be a separate crate.

r? @flip1995

changelog: none

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 29, 2024
@Jarcho Jarcho force-pushed the deprecated_shrink branch 2 times, most recently from 32f87e0 to 490c44a Compare July 29, 2024 17:41
flip1995
flip1995 reviewed Jul 29, 2024
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall. I still have to review the update_lints changes (I ran out of time for today).

@xFrednet can you please review the changes in index.html. I have no idea anymore what's going on there 😅

clippy_lints/src/deprecated_lints.rs Outdated
///
/// ### Deprecation reason
/// This lint has been superseded by #[must_use] in rustc.
("clippy::unsafe_vector_initialization", "the suggested alternative had substantially different behavior"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

behavior->perf characteristics to keep the old wording?

clippy_lints/src/deprecated_lints.rs Show resolved Hide resolved
@bors
Copy link
Collaborator

bors commented Jul 29, 2024

☔ The latest upstream changes (presumably #13178) made this pull request unmergeable. Please resolve the merge conflicts.

@Jarcho Jarcho force-pushed the deprecated_shrink branch 2 times, most recently from 85b8e35 to ecda54a Compare July 29, 2024 20:30
@Jarcho
Copy link
Contributor Author

Jarcho commented Jul 29, 2024

index.html has been fixed. Stupid vscode auto formatting.

@Jarcho Jarcho force-pushed the deprecated_shrink branch 3 times, most recently from 3a3d07f to 829fd0b Compare July 29, 2024 23:27
@bors
Copy link
Collaborator

bors commented Jul 30, 2024

☔ The latest upstream changes (presumably #13182) made this pull request unmergeable. Please resolve the merge conflicts.

xFrednet
xFrednet reviewed Jul 31, 2024
View reviewed changes
Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@flip1995 Sure!

The changes to metadata_collector.rs and index.html both look good to me :D

I've also checked the rendered output, and it all works as expected!

@flip1995
Copy link
Member

I just noticed that after the rebase the index.html changes are only one line. That I could've reviewed myself 😅 Sorry for the ping.

@xFrednet
Copy link
Member

No problem at all, I like to roughly know what's going on in that file anyways =^.^=

@Jarcho
Mark some lint deprecations as renames
1672d5d
@Jarcho
Store deprecated lints as an array of tuples.
2c34d58 
Remove legacy deprecations.
Remove "View Source" link for deprecated lints.
flip1995
flip1995 approved these changes Aug 5, 2024
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@flip1995
Copy link
Member

flip1995 commented Aug 5, 2024

@bors r+

(assuming CI will pass)

@bors
Copy link
Collaborator

bors commented Aug 5, 2024

📌 Commit c2186e1 has been approved by flip1995

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Aug 5, 2024

⌛ Testing commit c2186e1 with merge e17d254...

@bors
Copy link
Collaborator

bors commented Aug 5, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing e17d254 to master...

@bors bors merged commit e17d254 into rust-lang:master Aug 5, 2024
11 checks passed
@bors bors mentioned this pull request Aug 5, 2024
Merged
@y21 y21 mentioned this pull request Sep 1, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xFrednet xFrednet xFrednet left review comments

@flip1995 flip1995 flip1995 approved these changes

Assignees

@flip1995 flip1995

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Jarcho @bors @flip1995 @xFrednet @rustbot