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.

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

new lint legacy_numeric_constants #12312

Merged
merged 2 commits into from Mar 30, 2024
Merged

Conversation

pitaj
Copy link
Contributor

@pitaj pitaj commented Feb 18, 2024

Rework of #10997

  • uses diagnostic items
  • does not lint imports of the float modules (use std::f32)
  • does not lint usage of float constants that look like f32::MIN

I chose to make the float changes because the following pattern is actually pretty useful

use std::f32;
let omega = freq * 2 * f32::consts::PI;

and the float modules are not TBD-deprecated like the integer modules.

Closes #10995


changelog: New lint [legacy_numeric_constants]
#12312

@rustbot
Copy link
Collaborator

rustbot commented Feb 18, 2024

r? @dswij

rustbot has assigned @dswij.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 18, 2024
@pitaj
Copy link
Contributor Author

pitaj commented Feb 18, 2024

r? xFrednet since they reviewed the last one. Also, CC @Centri3

@bors
Copy link
Collaborator

bors commented Feb 19, 2024

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

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 19, 2024
…nstants, r=Nilstrieb

Add diagnostic items for legacy numeric constants

For rust-lang/rust-clippy#12312
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 19, 2024
…nstants, r=Nilstrieb

Add diagnostic items for legacy numeric constants

For rust-lang/rust-clippy#12312
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 19, 2024
Rollup merge of rust-lang#121272 - pitaj:diag_items-legacy_numeric_constants, r=Nilstrieb

Add diagnostic items for legacy numeric constants

For rust-lang/rust-clippy#12312
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Feb 21, 2024
…nstants, r=Nilstrieb

diagnostic items for legacy numeric modules

For rust-lang/rust-clippy#12312

Missed these in rust-lang#121272

r? `@Nilstrieb`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 21, 2024
Rollup merge of rust-lang#121361 - pitaj:diag_items-legacy_numeric_constants, r=Nilstrieb

diagnostic items for legacy numeric modules

For rust-lang/rust-clippy#12312

Missed these in rust-lang#121272

r? `@Nilstrieb`
@xFrednet
Copy link
Member

Hey, thank you for picking this up. This is on my todo list for next week, you should have a review by Friday, the latest. :)

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 27, 2024
…tants, r=Nilstrieb

syms for legacy numeric constants diag items

Missed these in rust-lang#121272 and rust-lang#121361, woops.

For rust-lang/rust-clippy#12312

r? `@Nilstrieb`
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Feb 28, 2024
…Nilstrieb

syms for legacy numeric constants diag items

Missed these in #121272 and #121361, woops.

For rust-lang/rust-clippy#12312

r? `@Nilstrieb`
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.

The implementation looks good to me. I have two suggestions, where I would like to hear your thoughts about it. Once they've been discussed, this should be good to go.

And also thank you for your patience! The next review will be quicker :D

tests/ui/legacy_numeric_constants.stderr Outdated Show resolved Hide resolved
tests/ui/legacy_numeric_constants_unfixable.stderr Outdated Show resolved Hide resolved
@pitaj
Copy link
Contributor Author

pitaj commented Mar 4, 2024

Thanks for taking a look. It seems like the next toolchain update should be on the 7th, which should include all the necessary diagnostic items.

I've made the changes you suggested, but wait to merge until I modify this to use the diag items.

@pitaj
Copy link
Contributor Author

pitaj commented Mar 10, 2024

Welp, in working on the implementation I discovered that the diag items were still incomplete. So I guess we'll wait another two weeks 🙃

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 10, 2024
…nstants, r=Nilstrieb

Fix legacy numeric constant diag items

- missed syms for usize/isize
- missed diag items on unsigned integers

For rust-lang/rust-clippy#12312

r? `@Nilstrieb`

Follow-up to rust-lang#121272, rust-lang#121361, rust-lang#121667
This should be the last one 🤞 Sorry!
jhpratt added a commit to jhpratt/rust that referenced this pull request Mar 10, 2024
…nstants, r=Nilstrieb

Fix legacy numeric constant diag items

- missed syms for usize/isize
- missed diag items on unsigned integers

For rust-lang/rust-clippy#12312

r? ``@Nilstrieb``

Follow-up to rust-lang#121272, rust-lang#121361, rust-lang#121667
This should be the last one 🤞 Sorry!
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 10, 2024
…nstants, r=Nilstrieb

Fix legacy numeric constant diag items

- missed syms for usize/isize
- missed diag items on unsigned integers

For rust-lang/rust-clippy#12312

r? ```@Nilstrieb```

Follow-up to rust-lang#121272, rust-lang#121361, rust-lang#121667
This should be the last one 🤞 Sorry!
@xFrednet
Copy link
Member

Okay, good to know :D I'll mark this as waiting for you. Once the next update is ready, you can push the update, and it should show up in my inbox again. Otherwise, you can also always ping me :D

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 10, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 10, 2024
Rollup merge of rust-lang#122271 - pitaj:diag_items-legacy_numeric_constants, r=Nilstrieb

Fix legacy numeric constant diag items

- missed syms for usize/isize
- missed diag items on unsigned integers

For rust-lang/rust-clippy#12312

r? ```@Nilstrieb```

Follow-up to rust-lang#121272, rust-lang#121361, rust-lang#121667
This should be the last one 🤞 Sorry!
@bors
Copy link
Collaborator

bors commented Mar 20, 2024

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

@pitaj pitaj force-pushed the legacy_numeric_constants branch 2 times, most recently from c9b5198 to 73ad96f Compare March 21, 2024 23:05
@pitaj
Copy link
Contributor Author

pitaj commented Mar 21, 2024

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Mar 21, 2024
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.

This version looks good to me. I love how almost 1/3 of the implementation lines are just diagnostic symbols :D

I have a tiny nit in the tests, and then we can merge this. You can r=me after the next update.

@bors delegate+

tests/ui/legacy_numeric_constants.rs Outdated Show resolved Hide resolved
tests/ui/legacy_numeric_constants_unfixable.rs Outdated Show resolved Hide resolved
@xFrednet
Copy link
Member

Looks like bors commands still don't work in reviews... Let's try this again:

@bors delegate+

@bors
Copy link
Collaborator

bors commented Mar 24, 2024

✌️ @pitaj, you can now approve this pull request!

If @xFrednet told you to "r=me" after making some further change, please make that change, then do @bors r=xFrednet

@xFrednet
Copy link
Member

xFrednet commented Mar 24, 2024

Actually, let's not r+ it yet. I just remembered that @Manishearth suggested in the last meeting, that we discuss/notify others about new lints first to see if someone has comments on it. So let's keep this open for a week, to see if others have any thoughts on it. I'll write something for Zulip :)

Zulip: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/New.20.60legacy_numeric_constants.60.20lint.20rust-clippy.2312312/near/429186612

@xFrednet xFrednet added S-final-comment-period Status: final comment period it will be merged unless new objections are raised (~1 week) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 24, 2024
@xFrednet
Copy link
Member

It has been a week and all comments have been positive, so I'm merging this now. Thank you for the new lint :D

@bors r+

@rustbot label -S-final-comment-period

@bors
Copy link
Collaborator

bors commented Mar 30, 2024

📌 Commit f2e91ab has been approved by xFrednet

It is now in the queue for this repository.

@rustbot rustbot removed the S-final-comment-period Status: final comment period it will be merged unless new objections are raised (~1 week) label Mar 30, 2024
@bors
Copy link
Collaborator

bors commented Mar 30, 2024

⌛ Testing commit f2e91ab with merge cebf879...

@bors
Copy link
Collaborator

bors commented Mar 30, 2024

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

@bors bors merged commit cebf879 into rust-lang:master Mar 30, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warn against using legacy std integral constants
9 participants