Add redundant type annotations lint

[AlessioC31]

Hello, I'm trying to add the redundat_type_annotations lint.

It's still WIP but I'd like to start gathering some feedbacks to be sure that I'm not doing things 100% wrong :)

Right now it still misses lints like:

• let foo: u32 = 5_u32,
• let foo: String = STest2::func()
• let foo: String = self.func() (MethodCall)
• refs
• Generics

I've some problems regarding the second example above, in the init part of the Local I have:

init: Some(
                Expr {
                    hir_id: HirId(DefId(0:24 ~ playground[e1bd]::main).58),
                    kind: Call(
                        Expr {
                            hir_id: HirId(DefId(0:24 ~ playground[e1bd]::main).59),
                            kind: Path(
                                TypeRelative(
                                    Ty {
                                        hir_id: HirId(DefId(0:24 ~ playground[e1bd]::main).61),
                                        kind: Path(
                                            Resolved(
                                                None,
                                                Path {
                                                    span: src/main.rs:77:21: 77:27 (#0),
                                                    res: Def(
                                                        Struct,
                                                        DefId(0:17 ~ playground[e1bd]::STest2),
                                                    ),
                                                    segments: [
                                                        PathSegment {
                                                            ident: STest2#0,
                                                            hir_id: HirId(DefId(0:24 ~ playground[e1bd]::main).60),
                                                            res: Def(
                                                                Struct,
                                                                DefId(0:17 ~ playground[e1bd]::STest2),
                                                            ),
                                                            args: None,
                                                            infer_args: true,
                                                        },
                                                    ],
                                                },
                                            ),
                                        ),
                                        span: src/main.rs:77:21: 77:27 (#0),
                                    },
                                    PathSegment {
                                        ident: get_numb#0,
                                        hir_id: HirId(DefId(0:24 ~ playground[e1bd]::main).62),
                                        res: Err,
                                        args: None,
                                        infer_args: true,
                                    },
                                ),
                            ),
                            span: src/main.rs:77:21: 77:37 (#0),
                        },
                        [],
                    ),
                    span: src/main.rs:77:21: 77:39 (#0),
                },
            ),

And I'm not sure how to get the return type of the function STest2::func() since the resolved path DefId points to the struct itself and not the function. Do you have any idea on how I could get this information in this case?

Thanks!

changelog: changelog: [redundant_type_annotations]: New lint to warn on redundant type annotations

fixes #9155

[rustbot]

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

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[bors]

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

[xFrednet]

Welcome to Clippy. Thank you for your patience while waiting on a review!

I've left some review comments. Please don't be discouraged by them, I believe this lint can become quite complex depending on how detailed and how many edge cases we want to catch. Please reach out if you require any further assistance. :)

[AlessioC31]

Welcome to Clippy. Thank you for your patience while waiting on a review!

I've left some review comments. Please don't be discouraged by them, I believe this lint can become quite complex depending on how detailed and how many edge cases we want to catch. Please reach out if you require any further assistance. :)

Thanks for the review!

[xFrednet]

Thanks for the review!

You're welcome. Are you still working on the implementation or is the branch ready for another review? :)

[AlessioC31]

Thanks for the review!

You're welcome. Are you still working on the implementation or is the branch ready for another review? :)

I'm still working on it. Right now I've problems with Lit type of expressions (like how to map a Lit to a Ty) :)
Do you have any suggestion on where I could have a look?

Or when is a type annotation useful in case of a Lit?

pub enum LitKind {
    /// A string literal (`"foo"`). The symbol is unescaped, and so may differ
    /// from the original token's symbol.
    Str(Symbol, StrStyle),
    /// A byte string (`b"foo"`). Not stored as a symbol because it might be
    /// non-utf8, and symbols only allow utf8 strings.
    ByteStr(Lrc<[u8]>, StrStyle),
    /// A byte char (`b'f'`).
    Byte(u8),
    /// A character literal (`'a'`).
    Char(char),
    /// An integer literal (`1`).
    Int(u128, LitIntType),
    /// A float literal (`1.0`, `1f64` or `1E10f64`). The pre-suffix part is
    /// stored as a symbol rather than `f64` so that `LitKind` can impl `Eq`
    /// and `Hash`.
    Float(Symbol, LitFloatType),
    /// A boolean literal (`true`, `false`).
    Bool(bool),
    /// Placeholder for a literal that wasn't well-formed in some way.
    Err,
}

In almost all these cases we can say that the annotation is redundant, or am I missing anything?
The only exception I can think about is in case of unsuffixed numbers (Int and Float literals)

[bors]

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

[xFrednet]

I'm still working on it. Right now I've problems with Lit type of expressions (like how to map a Lit to a Ty) :) Do you have any suggestion on where I could have a look?

I don't think there is an easy way to map it. You could request the type of the expression, but that will probably not help too much.

Or when is a type annotation useful in case of a Lit?

A type annotation is useful or even required when the literal is unsuffixed. For example 8 can be several types, the compiler would then look at its usages to determine the type. If the user writes let x: u8 = 8, the type should be kept IMO. However, if the literal has a suffix 8_u8 then the type can be removed. You can take a look at LitKind::is_unsuffixed() for this one. For &str etc it should be fine to remove the type as the literal is clear.

[xFrednet]

Generally speaking, I think this is a good start, which we could actually merge as a minimum viable product. There are still several examples that could be improved. However, most of them seem a bit complicated to fix and the question is how much benefit we'll get from them. It should be fine as is, since all ToDos are false negatives.

What are your thoughts? Do you want to try perfecting it in this PR or get this merge ready and create follow-ups? (Assuming, you're still interested in the lint)

[xFrednet]

This currently doesn't trigger a lint, even if the function comment says it should. I'm not quite sure why, as String::new() etc are covered.

[AlessioC31]

Wow, I missed that, thanks!

I digged into it a bit, it turns out it is related to generics.
When I do cx.tcx.type_of(func_defid) of the function, I get EarlyBinder(fn(T) -> Self {<Self as std::convert::From<T>>::from})

If I then execute subst_identity or skip_binder on this, I get fn(T) -> Self {<Self as std::convert::From<T>>::from}

Then, when I do func_ty.fn_sig(cx.tcx).output() where func_ty is the type I wrote previously, I get Binder(Self, []), then I interpret the return type as Self instead of String.

Do you know what I could use in this case? I tried having a look at other lints but I was not able to find anything, thanks!

[AlessioC31]

Hey @xFrednet , do you have any clue on this? Thanks!

[xFrednet]

Sorry, I missed your question earlier 😅. I'm not totally sure about this one, but you might want to take a look at:

• type_dependent_def to resolve the method target
• Maybe qpath_res to resolve Self?

If both of these don't work, I'd probably ask on Zulip the others might have a good idea for this one :)

[AlessioC31]

Sorry, I missed your question earlier 😅. I'm not totally sure about this one, but you might want to take a look at:

• type_dependent_def to resolve the method target
• Maybe qpath_res to resolve Self?

If both of these don't work, I'd probably ask on Zulip the others might have a good idea for this one :)

I've tried the two methods you suggested without any luck. I've asked on Zulip, let's see.

Thanks!

[AlessioC31]

Generally speaking, I think this is a good start, which we could actually merge as a minimum viable product. There are still several examples that could be improved. However, most of them seem a bit complicated to fix and the question is how much benefit we'll get from them. It should be fine as is, since all ToDos are false negatives.

What are your thoughts? Do you want to try perfecting it in this PR or get this merge ready and create follow-ups? (Assuming, you're still interested in the lint)

Hello @xFrednet , thanks again.

I'm still interested and I plan to perfecting this PR (at least adding support for primitive types and the suggestions you mentioned in the previous reviews).
I had to pause working on this because I had a busy period, it should be better next week!

[AlessioC31]

Hey @AlessioC31, what is the current status of the PR? Are there still open TODOs, besides the rebase? :)

Hello @xFrednet , there's still the problem with the String::from (and other generics since it's the same I guess). I've asked on Zulip but I haven't received any answer, maybe I can wrap this PR, I'll add support for refs, rebase and then it's ready to being reviewed. It won't lint in case generics are used but maybe I can come back at this when I understand how to do it

[AlessioC31]

Hello @xFrednet, I think this is ready for review.

We miss a couple of things which I think they could be added in the future, like:

• Support for generics
• Refs returned from anything else than a MethodCall (maybe it's because of my little experience with rust yet, but I think this shouldn't happen often, what do you think?)
• Complex types (tuples, arrays, etc...)
• Path to something different than a primitive type. Like for now this lint supports cases like let a: u32 = u32::MAX but it doesn't support things like let a: MyStruct = Somewhere::MyStaticInstance. Do you think we need to add this?

Thanks!

[xFrednet]

Hello @xFrednet, I think this is ready for review.

We miss a couple of things which I think they could be added in the future, like:

• Support for generics

As long as we don't have false positives and only false negatives, that's totally fine :). Could you add this limitation, as well as the complex types and path one into the lint documentation? I like to use a ### Limitations section to list such things. After this PR, we should ideally also create issues for these :)

• Refs returned from anything else than a MethodCall (maybe it's because of my little experience with rust yet, but I think this shouldn't happen often, what do you think?)

We should make sure that normal function calls are also detected. For one, because they're used quite often and because it should be a simple addition. I can mark the required changes in the next review :)

• Complex types (tuples, arrays, etc...)

Complex types are sadly complex to lint. I've been contemplating, how to best handle lints for these, for a different project. I sadly haven't found a good option. This is to say, that it's fine if these are not caught, probably. I guess that Clippy has quite a few lints that can't handle these :)

• Path to something different than a primitive type. Like for now this lint supports cases like let a: u32 = u32::MAX but it doesn't support things like let a: MyStruct = Somewhere::MyStaticInstance. Do you think we need to add this?

I believe that should be fine. Such a pattern is rarely used in the projects I'm using. We can leave this as an improvement for the future :)

Thanks!

You're very welcome, thank you for working on Clippy! I'll review this PR in the upcoming week :)

[bors]

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

[xFrednet]

This version looks good to me. I would say, it only requires the limitations section and a rebase. Then we can merge it! :)

[AlessioC31]

I think I've addressed all your comments and rebased to master. Should I squash the commits?

Thanks!

[xFrednet]

I think I've addressed all your comments and rebased to master.

There still seems to be a conflict, could you resolve that one?

Should I squash the commits?

It's not mandatory, but would be nice, 1 - 5 commits would be a good balance :)

I intend to review this over the weekend :)

[AlessioC31]

I think I've addressed all your comments and rebased to master.

There still seems to be a conflict, could you resolve that one?

Should I squash the commits?

It's not mandatory, but would be nice, 1 - 5 commits would be a good balance :)

I intend to review this over the weekend :)

Ok I've rebased to master and reduced the amount of commits, thanks!

[AlessioC31]

Rebased again :D

[xFrednet]

This version looks good to me. Thank you for all the work you put into this lint! I appreciate it!

@bors r+

[bors]

📌 Commit a9b468f has been approved by xFrednet

It is now in the queue for this repository.

[bors]

⌛ Testing commit a9b468f with merge cc8ead2...

[bors]

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