New lint [`legacy_numeric_constants`] #10997

Centri3 · 2023-06-20T21:39:56Z

~~I don't like one specific part of its implementation, pretty obvious what it is :) If somebody has a better way to do it, please tell me!~~ Thanks Alex :)
(Still had fun on this though :D)

Closes #10995

changelog: New lint [legacy_numeric_constants]

rustbot · 2023-06-20T21:40:01Z

r? @xFrednet

(rustbot has picked a reviewer for you, use r? to override)

clippy_lints/src/legacy_integral_constants.rs

m-ou-se · 2023-06-21T10:53:44Z

What does this new lint do for this snippet?

use std::u64;

fn main() {
    let _ = u64::MAX;
}

It should suggest simply removing the use std::u64; line and not comment on the u64::MAX expression, because that will still work when u64 resolves to the type instead of the module.

Centri3 · 2023-06-21T17:31:13Z

What does this new lint do for this snippet?
use std::u64;

fn main() {
    let _ = u64::MAX;
}
It should suggest simply removing the use std::u64; line and not comment on the u64::MAX expression, because that will still work when u64 resolves to the type instead of the module.

I think it should just not lint. Ideally it would remove the use. Getting the use of u64 would be a bit tricky, IIUC.

Centri3 · 2023-06-21T21:44:31Z

I've altered its suggestion on such a case; it now suggests std::primitive::<etc> instead, with a note stating that removing the use statement will make the above unnecessary

tests/ui/legacy_integral_constants.stderr

pitaj · 2023-06-22T03:50:11Z

It's kinda weird to call this legacy_integral_constants when it also concerns the floating-point types, which are not integers. Maybe legacy_numeric_constants instead?

pitaj · 2023-06-22T04:09:46Z

I've altered its suggestion on such a case; it now suggests std::primitive::<etc> instead, with a note stating that removing the use statement will make the above unnecessary

Not really a huge fan of this, since std::primitive::i32::MAX is IMO worse. I think it's fine to not provide a machine-applicable suggestion in this case, and instead just span_help to remove use std::<number>;.

pitaj · 2023-06-22T04:15:27Z

Thought of another corner case that might require handling separately:

// replacing this with just `use u64::MAX` results in an error which ironically suggests `std::u64::MAX`
use std::u64::MAX;

fn main() {
    let _ = MAX;
}

Centri3 · 2023-06-22T04:22:36Z

I've altered its suggestion on such a case; it now suggests std::primitive::<etc> instead, with a note stating that removing the use statement will make the above unnecessary

Not really a huge fan of this, since std::primitive::i32::MAX is IMO worse. I think it's fine to not provide a machine-applicable suggestion in this case, and instead just span_help to remove use std::<number>;.

As said on zulip, this is tricky. Likely not very plausible. It's either std::primitive or no suggestion, really. (Or, of course, finding each use statement as said on zulip as well but this will be A: ugly and B: have an ugly suggestion that likely won't even work)

tests/ui/legacy_integral_constants.fixed

pitaj · 2023-06-22T04:39:37Z

Ah right. If getting the span for span_help will be an enormous pain, then just providing a dumb help message or note is fine. Something as generic as "remove the use std::f32 import to use the associated constant instead" would be fine with me.

pitaj · 2023-06-22T05:14:24Z

Actually, is it possible to make the lint trigger on the actual imports like use std::i32; directly, and ignore cases where the imports are just used like i32::MAX? Would the suggestion to remove the import be easier to implement in that case?

Might want to exclude floats from machine applicable in case someone does something like

use std::f32;

fn main() {
    let _ = f32::consts::E;
}

Centri3 · 2023-06-22T05:17:55Z

What about std::i32::MAX, then? If there's no use statement then it won't lint it. You could lint expressions only if it's an absolute path but that feels hacky imo and would likely lead to an awful suggestion

blyxyas · 2023-06-22T07:52:18Z

You could lint expressions only if it's an absolute path but that feels hacky

Maybe this solution is worse, but you could implement a check_item for ItemKind::Use and store spans to these uses. It's a solution I've used a few times and it shouldn't be a performance hit when ignoring allowed lints is implemented.

But that's just a thought.

Centri3 · 2023-06-22T08:33:59Z

That's what I was thinking of, but I was worried of cases like

use std::f64 as _;
// or
pub mod a {
    pub use std::f64;
}

fn a() {
    self::a::f64::MAX;
    use std::f64;
    f64::MAX; // will it always lint the above `use` instead of the `as _` one?
    // etc...

    // as alexendoo also noted, it'll likely be terribly difficult to create an accurate suggestion, so
    // it might also not be worth the effort at all, like how would:
    use std::{f64, f32::{MAX, self}, borrow::Cow};
    // work?
}

There are many edgecases to be worried of here, and to ensure that the removed use would be the one it originated from would likely require reimplementing resolving, which isn't exactly ideal (we could have a single once note that suggests removing them all, or a hidden suggestion, but I don't like either of those ideas all too much)

blyxyas · 2023-06-22T11:36:26Z

// as alexendoo also noted, it'll likely be terribly difficult to create an accurate suggestion, so
// it might also not be worth the effort at all, like how would:
use std::{f64, f32::{MAX, self}, borrow::Cow};
// work?

The rust-analyzer team may have some insights here, when using an unknown object, RA may suggest to import it, and the use item will be formatted AFAIK.

Alexendoo · 2023-06-22T12:14:15Z

What about std::i32::MAX, then? If there's no use statement then it won't lint it. You could lint expressions only if it's an absolute path but that feels hacky imo and would likely lead to an awful suggestion

You can use the current logic where it checks if the first segment is e.g. u8 to determine if it needs to add std::primitive, but instead just return early in that case. The user removing the use std::u8 will then make it use the associated constant

The rust-analyzer team may have some insights here, when using an unknown object, RA may suggest to import it, and the use item will be formatted AFAIK.

The part that makes it difficult is that all the required information to create a good suggestion isn't available in the HIR, it lowers nested use items into multiple single ones

pitaj · 2023-06-22T13:47:37Z

What about std::i32::MAX, then? If there's no use statement then it won't lint it. You could lint expressions only if it's an absolute path but that feels hacky imo and would likely lead to an awful suggestion

Just to be clear, my thought was to keep linting expressions like you already are and only skip ones which look like associated constant usage i32::MAX. Anything else like std::i32::MAX or MAX would still get linted directly with a suggestion.

Again, if providing a machine applicable suggestion is very difficult even doing it this way, I'm okay with just a span_help or unspanned help or note instead.

Lint use std::<number>; statements
- Okay if not auto-fixable
- Exclude floats
Lint std::<number>::<CONST> expressions
- If the expression looks exactly like it would when using the associated constant (the only difference is the import), provide a note about removing the import ("remove the import of std::i32 as identified above") or just skip the lint entirely.

Thanks for working on this! I really appreciate it.

tests/ui/legacy_integral_constants.fixed

Centri3 · 2023-06-22T22:13:56Z

Just to be clear, my thought was to keep linting expressions like you already are and only skip ones which look like associated constant usage i32::MAX. Anything else like std::i32::MAX or MAX would still get linted directly with a suggestion.

Yeah that sounds fine by me

Centri3 · 2023-06-23T00:04:07Z

I've made it have a note on every applicable suggestion that some use statements may be the culprit, but it doesn't suggest removing them automatically.

Centri3 · 2023-06-23T00:05:07Z

clippy_lints/src/legacy_numeric_constants.rs

+                        }
+                    }
+                },
+                UseKind::Glob => glob_use_stmts.push(item.span),


Maybe we can use names_imported_by_glob_use to filter some of these out? Might not be worth the effort, though

pitaj · 2023-06-27T17:00:13Z

I'm happy with this now. The only thing left is that FIXME. Not sure what the issue is there.

xFrednet

I like the lint idea, but I'm not too sure about the way it's implemented. I've left some notes. (I've read the discussion a few days ago, so sorry if some of these have already been discussed 😅 )

xFrednet · 2023-06-30T14:25:51Z

clippy_lints/src/legacy_numeric_constants.rs

+        if !msrv.meets(msrvs::STD_INTEGRAL_CONSTANTS) || in_external_macro(cx.sess(), expr.span) {
+            return;
+        }


Can you add the lint to the msrv config list and run metadata-collection?

xFrednet · 2023-07-03T14:46:16Z

clippy_lints/src/legacy_numeric_constants.rs

+            glob_use_stmts,
+        } = self;
+
+        if !item.span.is_dummy() && let ItemKind::Use(path, kind) = item.kind {


I think this should also check for item.span.from_expansion() or in_external_macro respectively

clippy_lints/src/legacy_numeric_constants.rs

xFrednet · 2023-07-03T15:04:35Z

clippy_lints/src/legacy_numeric_constants.rs

+                "usage of a legacy numeric constant"
+            };
+
+            span_lint_and_then(cx, LEGACY_NUMERIC_CONSTANTS, span, msg, |diag| {


Clippy's span_lint_* functions emit the lint, at the node, that the cx has last passed to the LateLintPass. The emission location is important for lint level tracking and lint expectations. If you emit a lint for a different node, please use span_lint_hir or span_lint_hir_and_then respectively. They allow you to specify what node should be used for lint level evaluation. Could you also add a test, that #[allow] works?

FYI @blyxyas

xFrednet · 2023-07-03T15:09:47Z

tests/ui/legacy_numeric_constants.rs

+fn msrv_too_low() {
+    // FIXME: Why does this lint??
+    std::u32::MAX;
+}


This properly also has a similar reason like span_lint_hir. The msrv is tracked by extract_msrv_attr!(LateContext); which does it for every node, but you lint once at the end. Either, you have to do the tracking in the visitor or change the logic to lint in the lint pass

Yeah it should probably do it like that instead (the latter)

xFrednet · 2023-07-03T15:15:31Z

clippy_lints/src/legacy_numeric_constants.rs

+struct V<'a, 'tcx> {
+    cx: &'a LateContext<'tcx>,
+    msrv: &'a Msrv,
+    use_stmts: &'a FxHashMap<Symbol, Vec<Span>>,
+    glob_use_stmts: &'a Vec<Span>,
+}


I'm not sure, if visiting everything again with a Visitor is the best thing to do. I'm guessing, you're doing it this way, to catch cases where the use std::primitive::u32 is after an item/body?

I can't remember if this has been discussed as part of the previous conversion. But I would guess that just linting on use std::primitive::u32 should be fine. There might be some uncool cases like use std::primitive::u32::MAX but even then, I'd say it's enough to lint that one and state that all uses of MAX have to be replaced with u32::MAX. Walking all items again will probably have a performance impact and is weird when it comes to msrv tracking and lint emission.

Yeah we need to do 2 passes to have that work unfortunately, we could lint the use specifically if it exists but that might be a bit uglier than just linting the usage

I believe V is used in check_crate_post as I was a bit worried it wouldn't have the list of every Use by the time it finds an applicable Expr, but maybe that's not the case

We could perhaps do a visit in check_crate like in check_crate_post then use check_expr like normal, now that we'd have the list of every Use

Linting the use seems fine to me since that's the part we want them to remove

But we can't really know whether they're using it for std::f32::MAX or std::f32::consts. Something like

use std::f32; use f32::consts;

Would be a FP (but not something people use regardless)

I think this would be an acceptable FP, I can't imagine people really using this. :)

I think this is a good idea and should also simplify the implementation :)

I don't particularly agree, it's inconsistent with the behavior of deprecated which lints on the usage itself which is generally a lot clearer. In the case of std::<int> though we should also lint the use itself as that's also quasi-deprecated but regardless the usage itself should be linted

Seeing usage of legacy numeric constants on use std::u32::MAX will likely confuse the programmer if removing that suddenly causes compilation to fail, and they may just silence the lint as a result (adding a single allow is easier than manually changing every MAX to u32::MAX)

This would also make it non-MachineApplicable in the case of std::f32::MAX and friends, which I don't think is worth the increased simplicity...

I think you may have misunderstood the behavior I proposed. I've edited it to hopefully clear things up.

I don't particularly agree, it's inconsistent with the behavior of deprecated which lints on the usage itself which is generally a lot clearer.

But the pattern i32::MAX is not the problem, it's actually the prior use std::i32; that's the problem because it was made redundant.

Seeing usage of legacy numeric constants on use std::u32::MAX will likely confuse the programmer if removing that suddenly causes compilation to fail, and they may just silence the lint as a result (adding a single allow is easier than manually changing every MAX to u32::MAX)

Under the behavior I suggested, use std::u32::MAX; would NOT trigger the lint. use std::u32; would.

This would also make it non-MachineApplicable in the case of std::f32::MAX and friends, which I don't think is worth the increased simplicity...

The behavior for std::f32::MAX would not change at all under my proposal.

I see.
But I still don't think we should go with this. In the case of f32::MAX or f64::MAX it'll result in a much less helpful message, just "remove the import" which is ambiguous; which import? It looks identical to f32::MAX, just saying there's some import for std::f32 somewhere isn't all too helpful.

What this simplifies is removing the use collection; however, the rest of it is only made more complicated. If the "remove the import" portion is improved, nothing has been simplified.

But the pattern i32::MAX is not the problem, it's actually the prior use std::i32; that's the problem because it was made redundant.

And in the cases where they're identical, It should still lint the usage but not give a suggestion, only listing the potentially bad imports. Then, since std::i32 is quasi-deprecated, that can be linted as well (we could also omit the bad imports in this case, since it'll be linted). In the case of f32::MAX it'd always show the bad imports.

But I still don't think we should go with this. In the case of f32::MAX or f64::MAX it'll result in a much less helpful message, just "remove the import" which is ambiguous; which import? It looks identical to f32::MAX, just saying there's some import for std::f32 somewhere isn't all too helpful.

I think you're overstating how hard it is to find and remove an import. And in the case of floats, it will commonly require more work anyways to resolve the ::consts issue. Maybe we just shouldn't ever lint f32::MAX.

What this simplifies is removing the use collection; however, the rest of it is only made more complicated. If the "remove the import" portion is improved, nothing has been simplified.

And in the cases where they're identical, It should still lint the usage but not give a suggestion, only listing the potentially bad imports. Then, since std::i32 is quasi-deprecated, that can be linted as well (we could also omit the bad imports in this case, since it'll be linted). In the case of f32::MAX it'd always show the bad imports.

In my opinion, linting i32::MAX is purely noise. There's nothing to change with that code. All that needs to be done is remove the redundant import. With current behavior, stderr is longer and more repetitive.

It feels like throwing a warning when someone uses .try_into() because they don't need to explicitly import the TryInto trait, since it's in the prelude.

Here's how I see it. The current behavior is slower, more complicated, and more repetitive for a slightly more useful warning message. I don't think the tradeoffs are worth it.

That said, I'll be happy with either, and I appreciate your work on this.

Urgau · 2023-07-03T16:31:05Z

Have you considered simply using diagnostics items instead of manually trying to match them ?

Centri3 · 2023-07-03T16:42:41Z

They aren't diagnostic items, so we can't unfortunately.

Urgau · 2023-07-03T16:51:17Z

They could be added. I'm sure rust-lang/rust wouldn't mind them, in particular for a lint.

Centri3 · 2023-07-03T16:54:38Z

That would require waiting for the next sync in 2 weeks. Blocking this for 2 weeks due to a minor nit isn't really the best imo. It's possible, but it would have to be done this way for a while.

There's also >30 of them, adding that many pre-interned symbols for a single lint to use might not be desirable for the compiler to do tbh. Maybe if other lints use them, but not just one that's intending to port people away (and will likely be deprecated if they're ever deprecated themselves)

xFrednet · 2023-07-05T19:38:40Z

They could be added. I'm sure rust-lang/rust wouldn't mind them, in particular for a lint.

They wouldn't mind at all. The diagnostic items are exactly intended for this kind of work. Just the sync wait time can be annoying from the Clippy side. I don't think it's a necessity, but definitely a nice idea :)

Centri3 · 2023-08-04T23:33:17Z

Well, I completely forgot about this PR 😅 I should have this back going again within a few days

bors · 2023-08-11T15:48:10Z

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

pitaj · 2023-08-12T19:32:18Z

tests/ui/legacy_numeric_constants.stderr

+LL |     use std::u32;
+   |         ^^^^^^^^
+   |
+   = help: use the associated constants on `u32` instead at their usage


Can we just recommend removing the import altogether? Maybe something like

help: remove this import note: then `u32::<CONST>` will resolve to the respective associated constant

We can't because somebody could do something stupid like use crate::u32::MAX after it, which would be incorrect. It would be nice if we could detect that though

It's definitely pedantic, but I think clippy should only suggest that if it'll actually be correct (at least for the note; the help seems ok)

Also, we should only show the note if it isn't aliased, like use std::u32 as u32_consts

What about this then?

help: remove this import and use the associated constants on `u32` instead note: then `u32::<CONST>` will resolve to the respective associated constant

pitaj · 2023-11-12T22:33:18Z

Hey @Centri3 any updates on this? If you're busy I may be able to finish it off.

xFrednet · 2023-11-13T12:31:29Z

@Centri3 has been busy lately, @pitaj If you're interested in this PR, you could probably pick it up and complete the last steps :)

xFrednet · 2024-02-12T21:04:18Z

I'm closing this PR due to inactivity. The implementation is an excellent start, if anyone is interested in picking this PR up, go ahead. If you have questions, you're welcome to ask them here as well.

@Centri3 Thank you for working on this. If you want to continue your work, you can reopen this PR or open a new one :D

new lint `legacy_numeric_constants` 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 ```rust 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](#12312)

rustbot assigned xFrednet Jun 20, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 20, 2023

Alexendoo reviewed Jun 20, 2023

View reviewed changes

clippy_lints/src/legacy_integral_constants.rs Outdated Show resolved Hide resolved

Centri3 force-pushed the std_integral_consts branch from 43d84ed to da08b7f Compare June 20, 2023 23:00

Centri3 force-pushed the std_integral_consts branch from da08b7f to 18ded67 Compare June 21, 2023 21:44

Centri3 force-pushed the std_integral_consts branch from 18ded67 to 30d5586 Compare June 22, 2023 02:56

pitaj reviewed Jun 22, 2023

View reviewed changes

tests/ui/legacy_integral_constants.stderr Outdated Show resolved Hide resolved

pitaj suggested changes Jun 22, 2023

View reviewed changes

tests/ui/legacy_integral_constants.fixed Outdated Show resolved Hide resolved

pitaj suggested changes Jun 22, 2023

View reviewed changes

tests/ui/legacy_integral_constants.fixed Outdated Show resolved Hide resolved

Centri3 commented Jun 23, 2023

View reviewed changes

Centri3 changed the title ~~New lint [legacy_integral_constants]~~ New lint [legacy_numeric_constants] Jun 23, 2023

Centri3 force-pushed the std_integral_consts branch 3 times, most recently from 7170eb4 to d94f41a Compare June 27, 2023 21:17

xFrednet reviewed Jul 3, 2023

View reviewed changes

Centri3 and others added 4 commits August 8, 2023 15:34

new lint legacy_integral_constants

1612dd8

now mentions use statements to remove

d5c15a4

Split tests into two files

d516d56

Refine stderr

1ad0d75

Centri3 force-pushed the std_integral_consts branch from d94f41a to 1ad0d75 Compare August 8, 2023 22:01

Urgau mentioned this pull request Aug 11, 2023

Activate rustc deprecated_in_future lint by default in clippy #11323

Open

pitaj reviewed Aug 12, 2023

View reviewed changes

xFrednet assigned blyxyas and xFrednet and unassigned xFrednet and blyxyas Nov 13, 2023

xFrednet added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Feb 12, 2024

xFrednet closed this Feb 12, 2024

pitaj mentioned this pull request Feb 18, 2024

new lint legacy_numeric_constants #12312

Merged

New lint [legacy_numeric_constants] #10997

New lint [legacy_numeric_constants] #10997

Conversation

Centri3 commented Jun 20, 2023 • edited

rustbot commented Jun 20, 2023

m-ou-se commented Jun 21, 2023 • edited

Centri3 commented Jun 21, 2023

Centri3 commented Jun 21, 2023

pitaj commented Jun 22, 2023

pitaj commented Jun 22, 2023 • edited

pitaj commented Jun 22, 2023 • edited

Centri3 commented Jun 22, 2023

pitaj commented Jun 22, 2023 • edited

pitaj commented Jun 22, 2023 • edited

Centri3 commented Jun 22, 2023

blyxyas commented Jun 22, 2023

Centri3 commented Jun 22, 2023 • edited

blyxyas commented Jun 22, 2023

Alexendoo commented Jun 22, 2023

pitaj commented Jun 22, 2023 • edited

Centri3 commented Jun 22, 2023

Centri3 commented Jun 23, 2023

Choose a reason for hiding this comment

pitaj commented Jun 27, 2023

xFrednet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Centri3 Jul 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Centri3 Jul 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitaj Jul 6, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitaj Jul 6, 2023 • edited

Choose a reason for hiding this comment

Urgau commented Jul 3, 2023

Centri3 commented Jul 3, 2023

Urgau commented Jul 3, 2023

Centri3 commented Jul 3, 2023 • edited

xFrednet commented Jul 5, 2023

Centri3 commented Aug 4, 2023

bors commented Aug 11, 2023

pitaj Aug 12, 2023 • edited

Choose a reason for hiding this comment

Centri3 Aug 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitaj Aug 12, 2023 • edited

Choose a reason for hiding this comment

pitaj commented Nov 12, 2023

xFrednet commented Nov 13, 2023 • edited

xFrednet commented Feb 12, 2024

New lint [`legacy_numeric_constants`] #10997

New lint [`legacy_numeric_constants`] #10997

Centri3 commented Jun 20, 2023 •

edited

m-ou-se commented Jun 21, 2023 •

edited

pitaj commented Jun 22, 2023 •

edited

pitaj commented Jun 22, 2023 •

edited

pitaj commented Jun 22, 2023 •

edited

pitaj commented Jun 22, 2023 •

edited

Centri3 commented Jun 22, 2023 •

edited

pitaj commented Jun 22, 2023 •

edited

Centri3 Jul 3, 2023 •

edited

Centri3 Jul 3, 2023 •

edited

pitaj Jul 6, 2023 •

edited

pitaj Jul 6, 2023 •

edited

Centri3 commented Jul 3, 2023 •

edited

pitaj Aug 12, 2023 •

edited

Centri3 Aug 12, 2023 •

edited

pitaj Aug 12, 2023 •

edited

xFrednet commented Nov 13, 2023 •

edited