New lint: default_numeric_fallback #6662

Y-Nak · 2021-02-02T05:20:24Z

fixes #6064
r? @flip1995

As we discussed in here and here, I start implementing this lint from the strictest version.
In this PR, I'll allow the below two cases to pass the lint to reduce FPs.

Appearances of unsuffixed numeric literals in Local if Local has a type annotation, for example:

// Good.
let x: i32 = 1;

// Also good.
let x: (i32, i32) = if cond {
   (1, 2)
} else {
   (2, 3)
};

Appearances of unsuffixed numeric literals in args of Call or MethodCall if corresponding arguments of their signature have concrete types, for example:

fn foo_mono(x: i32) -> i32 {
    x
}

fn foo_poly<T>(t: T) -> t {
    t
}

// Good.
let x = foo_mono(13);

// Still bad.
let x: i32 = foo_poly(13);

changelog: Added restriction lint: default_numeric_fallback

rust-highfive · 2021-02-02T05:20:27Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @flip1995 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Y-Nak · 2021-02-03T15:32:56Z

@flip1995 I finished the first implementation, though CI failed ~~(maybe) due to updates of the toolchain~~, I completely misunderstood how Clippy runs tests, almost fixed now.
As we discussed, hir_ty_to_ty causes ICR if passed type includes hir::TyKind::Infer because it doesn't assume the use from inside of function bodies. I'll try to resolve this problem tomorrow.

Y-Nak · 2021-02-04T08:41:04Z

I added some tests mainly related to hir::Tykind::Infer and fixed some bugs.
Now all tests have been passed! Could you review this PR, please?

Y-Nak · 2021-02-04T14:20:19Z

Changes: Rewrote enclosing_body_owner_opt to correctly determine if a target hir_id is included in a body and renamed it to enclosing_body_opt.

flip1995

Let me preface this with: I hate giving reviews like this, when so much work was put into this.

So I don't think we can walk every type we find just to check if somewhere in the type some type has to be inferred. This is just way too overkill for this lint.

I also don't quite understand why this lint has to be so complicated. If I had to implement this, I would just implement it on a big match on the ExprKind inside the check_expr function. To avoid linting let statements where types are defined, you can just save the HitId of the local.init expression and then skip this expression in check_expr.

Looking at the tests, the only thing you want to lint are int or float literals. And the only case you don't want to lint is if you are inside a let binding where a type definition is in the let binding. You can achieve this with:

fn check_stmt(&mut self, cx: &LateContext<'_>, stmt: &Stmt<'_>) {
    if_chain! {
        if let StmtKind::Local(local) = stmt.kind;
        if local.ty.is_some();
        if let Some(init) = local.init;
        then {
            self.skip_lint.insert(init.hir_id);
        }
    }
}

fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) {
    let parent_iter = cx.tcx.hir().parent_iter(expr.hir_id);
    if_chain! {
        if let ExprKind::Lit(lit) = expr.kind;
        if matches!(lit.node, LitKind::Int(_, LitIntType::Unsuffixed) | LitKind::Float(_, LitFloatType::Unsuffixed));
        if let Some(ty) = cx.typeck_results().expr_ty(expr);
        if matches!(ty.kind, ty::Int(IntTy::I32) | ty::Float(FloatTy::F64));
        if !parent_iter.any(|(hir_id, _)| self.skip_lint.contains(hir_id));
        then {
            span_lint_and_sugg(..);
        }
    }
}

I didn't test this code, but I'm fairly confident that it should work like this or similar.

flip1995 · 2021-02-06T12:59:32Z

tests/ui/default_numeric_fallback.stderr

+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:39:16
+   |
+LL |     let x: _ = 13;


I wouldn't lint this, because specifying the type _ really means "I don't care about this type".

flip1995 · 2021-02-06T12:59:40Z

tests/ui/default_numeric_fallback.stderr

+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:40:22
+   |
+LL |     let x: [_; 3] = [1, 2, 3];


flip1995 · 2021-02-06T13:00:23Z

tests/ui/default_numeric_fallback.stderr

+error: default numeric fallback might occur
+  --> $DIR/default_numeric_fallback.rs:41:27
+   |
+LL |     let x: (_, i32) = (1, 2);


This is a FP, because the type of the second tuple item has been specified.

Y-Nak · 2021-02-06T14:00:13Z

Let me explain.
Firstly, I thought we had agreed on how this lint should be used. First of all, I thought we both agreed on how the lint was to be used, in that it was to be used in situations where a very high level of reliability is required at the type level, and the appearance of FP was inevitable. Isn't that why we chose the category restriction?
The reason this lint is so long is not to detect the possibility of a Default numeric fallback, but to try to reduce FP as much as possible while not generating FN.
I suspect your implementation will produce FN.
For example, wouldn't it result in FN in the following case?

fn foo<T>(t:T) -> Foo {}
let x: Foo = foo(13);

Y-Nak · 2021-02-06T14:17:19Z

Also this case would cause FP in your implementation.

fn foo() -> i32 {
    13
}

Y-Nak · 2021-02-06T14:32:31Z

Just in case: I don't care at all even if I need to remove all the lines I wrote, I'm sorry if I took up your time though, as for me I was happy to learn a lot through this implementation.

Y-Nak · 2021-02-06T15:01:16Z

Anyway I'll rewrite this tomorrow. Thanks for reviewing.

Y-Nak · 2021-02-07T09:24:32Z

Changes: Simplify DefaultNumericFallback as @flip1995 suggested.

In fact, I can't yet be convinced that this implementation is right.
Consider the next scenario.

A user wrote a code like below.

fn foo(x: i32) -> i32 {}
fn main() {
    // FP!
    let x = foo(1);
}

The current implementation would lint this although that's FP, so the user added a type annotation to avoid FP.

fn foo(x: i32) -> i32 {}
fn main() {
    let x: _ = foo(1);
}

After months or so, the user noticed that they have to rewrite it so that foo should also receive u32 or other integer types because it turns out in some part of their code other integer types should be used with foo for some reasons.
So they rewrote it like below.

fn foo<T: Integer>(x: T) -> i32 {}
fn main() {
    // FN! Default numeric fallback has occured.
    let x: _ = foo(1);
}

This seems a disaster for me.
I don't understand why this kind of FPs/FNs should be acceptable.

The "complex" version can avoid this kind of disaster, and we can reduce the complexity in exchange for some increase of FPs or by allowing some FNs.
But I fully respect your works and experiences in this region, so I'm willing to accept changes if you still think this version is preferable to the first version.

Y-Nak · 2021-02-07T13:00:37Z

As a reference, I simplified the "complex" version and reduced FPs by allowing FNs.
default_numeric_fallback.rs
tests/ui/default_numeric_fallback.rs
tests/ui/default_numeric_fallback.stderr

flip1995 · 2021-02-14T14:20:05Z

Isn't that why we chose the category restriction?

No, the restriction group is the correct group for this lint, because fallback to the default int/float type is a Rust concept that does not really cause problems. Having to specify the type anyway would restrict the language, so that you can't just write x = 42 anymore without specifying the type. If we want no FNs without caring about FPs, we could just lint every literal that is Unsuffixed

For example, wouldn't it result in FN in the following case?
fn foo<T>(t:T) -> Foo {}
let x: Foo = foo(13);

Yes, I guess it would. But I think this is a really constructed example, because you wouldn't specify a type here. Even if you change it to

fn foo<T>(t:T) -> Foo<T> {}
let x: Foo<_> = foo(13);

I don't think this would be a FN, because you specified T to be _ and therefore said "It doesn't matter".

Also this case would cause FP in your implementation.
fn foo() -> i32 {
    13
}

Why is that a FP? The type is not immediately visible for the 13, but defined via the return value. If I would care about int/float types that much, I would also like to make this clear in return positions.

I now start to think that our interpretations of what the lint should do is different. You say, that only places where actual fallback occurs should be linted. I think all places where no type is specified immediately in the same statement should be linted.

so the user added a type annotation to avoid FP.

Adding _ as a type is a solution to this lint, that could lead to the disaster you described. But we don't and shouldn't suggest that as the fix. Rather we're suggesting to add i32/f64 to the literal as the fix, which is resistent to the disaster you described.

In the end all your work you put into this helped to produce the ICE reproducer, no one else managed to come up with. So your work was definitely not in vain.

flip1995 · 2021-02-14T14:23:04Z

tests/ui/default_numeric_fallback.rs

-fn ret_i31() -> i32 {
-    23
-}


I think you should keep all the tests, so if someone else wants to enhance/modify the lint there are already tests in place, that can be used for reference. Also then I will see, how my suggested changes changed the behavior of the lint.

Y-Nak · 2021-02-15T05:22:38Z

Yes, I guess it would. But I think this is a really constructed example, because you wouldn't specify a type here. Even if you change it to

Consider this case.

fn foo<T, U> foo(t: T) -> U {
...
}
let x: MyType = foo(1);

In this case, T and U are irrelevant, so I still think this is FN.

And I think there is another problem in the current implementation. If an init expression is ExprKind::Block and the Local has type annotation, the current implementation doesn't lint anything in the block
Example:

fn main() {
    let x: MyType = {
        // FN. 
        let y = 13;

        ...
    };
}

Y-Nak · 2021-02-15T05:40:35Z

Changes: In order to make the comparison easier, I pushed another version that addresses the problems I pointed out. This implementation is almost the same as this.
I suppose that this implementation somewhat meets the requirements of both of us.
I don't stick to the implementation, so please feel free to reject it.

flip1995

And I think there is another problem in the current implementation. If an init expression is ExprKind::Block and the Local has type annotation, the current implementation doesn't lint anything in the block

Yeah your right, that is really too restrictive.

The problem I have with making this lint too complex is that complexity makes bugs more likely and also makes it harder to fix bugs. And IMO this is a kind of lint, that should not require this complexity, when choosing the right heuristics.

The current implementation idea LGTM. But I don't think you need an extra visitor, since LateLintPass already is a visitor. You can just push the arg types in check_expr and then pop them in check_expr_post. The arguments get visited after the {Method}Call anyway. That way you also get rid of the restriction, that it only works if the call is on a Path.

Also you have to be careful with calling fn_sig on Closures. This will ICE

Please also add back all the tests you already had and the examples you gave in the comments of the PR with a comment each, why it should (not) lint.

flip1995 · 2021-02-15T08:43:57Z

clippy_lints/src/default_numeric_fallback.rs

+        let ty_bound = self.ty_bounds.last().unwrap();
        if_chain! {


Suggested change

let ty_bound = self.ty_bounds.last().unwrap();

if_chain! {

if_chain! {

if let Some(ty_bound) = self.ty_bounds.last();

Y-Nak · 2021-02-15T13:13:52Z

The problem I have with making this lint too complex is that complexity makes bugs more likely and also makes it harder to fix bugs.

Yeah, I completely agree.

But I don't think you need an extra visitor, since LateLintPass already is a visitor.

I agree on this in almost all cases. But in this case, I think we can't use it as a visitor because there is no check_stmt_post or check_local_post in LateLintPass.

Also you have to be careful with calling fn_sig on Closures. This will ICE

Thanks for pointing it out! I didn't realize it.

Please also add back all the tests you already had and the examples you gave in the comments of the PR with a comment each, why it should (not) lint.

Ok, I'll add them later.

flip1995 · 2021-02-15T14:01:16Z

But in this case, I think we can't use it as a visitor because there is no check_stmt_post or check_local_post in LateLintPass.

Ah right. Please add something like "This lint can only be allowed on function levels and above to the Known Problems section then?

Y-Nak · 2021-02-15T14:39:57Z

Changes:

Add fn_sig_opt to get fn signature from HirId
Add more tests and edit known problems.

Y-Nak · 2021-02-16T02:23:33Z

Changes: Handle struct constructor case.
I suppose that struct ctor should be handled in the same way as Call or MethodCall, but not sure.

flip1995

This looks really good to me now.

I think linting in return places is good, because normally the return place is not as close to the fn sig as in the tests, so specifying the type more locally makes sense to me. Not regarding correctness, but readability.

One small change left to make.

clippy_lints/src/default_numeric_fallback.rs

Y-Nak · 2021-02-16T09:09:08Z

Changes: Use span_lint_and_sugg instead of span_lint_and_help.

flip1995 · 2021-02-16T09:58:44Z

Perfect! I think we found a good balance for this lint. Thanks for all your work!

@bors r+

Changes: Use span_lint_and_sugg instead of span_lint_and_help.

I really like those comments summarizing your latest changes, btw 👍

bors · 2021-02-16T09:58:46Z

📌 Commit 9b0c1eb has been approved by flip1995

bors · 2021-02-16T09:58:54Z

⌛ Testing commit 9b0c1eb with merge e2753f9...

bors · 2021-02-16T10:11:29Z

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

Y-Nak · 2021-02-16T10:22:44Z

@flip1995 Thanks for your patience in the long discussion. I really appreciate your help!

rust-highfive assigned flip1995 Feb 2, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 2, 2021

Y-Nak marked this pull request as draft February 2, 2021 05:20

Y-Nak force-pushed the default-numeric-fallback branch 3 times, most recently from 547bc1b to c350f89 Compare February 3, 2021 15:08

Y-Nak marked this pull request as ready for review February 3, 2021 15:10

Y-Nak marked this pull request as draft February 4, 2021 06:18

New lint: default_numeric_fallback

e32e4de

Y-Nak force-pushed the default-numeric-fallback branch from c350f89 to e32e4de Compare February 4, 2021 08:29

Y-Nak marked this pull request as ready for review February 4, 2021 08:40

Fix enclosing_body_owner_opt and rename it to enclosing_body_opt

8b8b0a9

flip1995 reviewed Feb 6, 2021

View reviewed changes

Simplify DefaultNumericFallback

22d4483

flip1995 reviewed Feb 14, 2021

View reviewed changes

Add some restrictions to default_numeric_fallback to avoid FNs

93796b2

Y-Nak force-pushed the default-numeric-fallback branch from e8a9cbe to 93796b2 Compare February 15, 2021 05:45

flip1995 reviewed Feb 15, 2021

View reviewed changes

Y-Nak added 2 commits February 15, 2021 23:29

Add fn_sig_opt to get fn signature from HirId

0198ac7

Add more tests for default_numeric_fallback

fb91c76

Handle struct ctor case

1f8ee3c

flip1995 approved these changes Feb 16, 2021

View reviewed changes

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

Change to span_lint_and_sugg from span_lint_and_help

9b0c1eb

bors merged commit e2753f9 into rust-lang:master Feb 16, 2021

Y-Nak deleted the default-numeric-fallback branch February 17, 2021 04:54

taiki-e mentioned this pull request Jun 1, 2021

default_numeric_fallback should not warn code generated by external macros #7304

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New lint: default_numeric_fallback #6662

New lint: default_numeric_fallback #6662

Y-Nak commented Feb 2, 2021 •

edited

Loading

rust-highfive commented Feb 2, 2021

Y-Nak commented Feb 3, 2021 •

edited

Loading

Y-Nak commented Feb 4, 2021 •

edited

Loading

Y-Nak commented Feb 4, 2021

flip1995 left a comment

flip1995 Feb 6, 2021

flip1995 Feb 6, 2021

flip1995 Feb 6, 2021

Y-Nak commented Feb 6, 2021 •

edited

Loading

Y-Nak commented Feb 6, 2021 •

edited

Loading

Y-Nak commented Feb 6, 2021 •

edited

Loading

Y-Nak commented Feb 6, 2021

Y-Nak commented Feb 7, 2021 •

edited

Loading

Y-Nak commented Feb 7, 2021

flip1995 commented Feb 14, 2021

flip1995 Feb 14, 2021

Y-Nak commented Feb 15, 2021

Y-Nak commented Feb 15, 2021

flip1995 left a comment •

edited

Loading

flip1995 Feb 15, 2021

Y-Nak commented Feb 15, 2021

flip1995 commented Feb 15, 2021

Y-Nak commented Feb 15, 2021 •

edited

Loading

Y-Nak commented Feb 16, 2021

flip1995 left a comment

Y-Nak commented Feb 16, 2021

flip1995 commented Feb 16, 2021

bors commented Feb 16, 2021

bors commented Feb 16, 2021

bors commented Feb 16, 2021

Y-Nak commented Feb 16, 2021

New lint: default_numeric_fallback #6662

New lint: default_numeric_fallback #6662

Conversation

Y-Nak commented Feb 2, 2021 • edited Loading

rust-highfive commented Feb 2, 2021

Y-Nak commented Feb 3, 2021 • edited Loading

Y-Nak commented Feb 4, 2021 • edited Loading

Y-Nak commented Feb 4, 2021

flip1995 left a comment

Choose a reason for hiding this comment

flip1995 Feb 6, 2021

Choose a reason for hiding this comment

flip1995 Feb 6, 2021

Choose a reason for hiding this comment

flip1995 Feb 6, 2021

Choose a reason for hiding this comment

Y-Nak commented Feb 6, 2021 • edited Loading

Y-Nak commented Feb 6, 2021 • edited Loading

Y-Nak commented Feb 6, 2021 • edited Loading

Y-Nak commented Feb 6, 2021

Y-Nak commented Feb 7, 2021 • edited Loading

Y-Nak commented Feb 7, 2021

flip1995 commented Feb 14, 2021

flip1995 Feb 14, 2021

Choose a reason for hiding this comment

Y-Nak commented Feb 15, 2021

Y-Nak commented Feb 15, 2021

flip1995 left a comment • edited Loading

Choose a reason for hiding this comment

flip1995 Feb 15, 2021

Choose a reason for hiding this comment

Y-Nak commented Feb 15, 2021

flip1995 commented Feb 15, 2021

Y-Nak commented Feb 15, 2021 • edited Loading

Y-Nak commented Feb 16, 2021

flip1995 left a comment

Choose a reason for hiding this comment

Y-Nak commented Feb 16, 2021

flip1995 commented Feb 16, 2021

bors commented Feb 16, 2021

bors commented Feb 16, 2021

bors commented Feb 16, 2021

Y-Nak commented Feb 16, 2021

Y-Nak commented Feb 2, 2021 •

edited

Loading

Y-Nak commented Feb 3, 2021 •

edited

Loading

Y-Nak commented Feb 4, 2021 •

edited

Loading

Y-Nak commented Feb 6, 2021 •

edited

Loading

Y-Nak commented Feb 6, 2021 •

edited

Loading

Y-Nak commented Feb 6, 2021 •

edited

Loading

Y-Nak commented Feb 7, 2021 •

edited

Loading

flip1995 left a comment •

edited

Loading

Y-Nak commented Feb 15, 2021 •

edited

Loading