WIP: use_self checking lifetimes & generics

[MasonRemaley]

Work in progress. Fixing a false positive in the use_self lint caused by type parameters not being checked, and also allowing the lint to check types with lifetimes.

fixes #1993

[MasonRemaley]

I'm a little stuck here--I started out by comparing the lifetimes and generics that are present in the item's path, but then I realized that they won't be in the path if they're inferred.

It seems like this information should probably be cached somewhere in LateContext, so I've been looking through the docs but haven't found a way to get the inferred parameters given a NodeId. Am I misunderstanding how things are set up, or am I just missing the function that does this?

[oli-obk]

Am I misunderstanding how things are set up, or am I just missing the function that does this?

What's the exact issue with the current implementation? Can you give an example where the lint fails?

[MasonRemaley]

Yup--I'm trying to fix the false positive given in #1993 caused by not verifying that the type parameters match self before warning (playground link), and also the fact that it currently bails if the type has any lifetimes.

[oli-obk]

Right, but you fixed some of these. I meant what failures are left after your changes?

[MasonRemaley]

Ah my bad misunderstood/didn't recognize your username. I've only changed the lifetimes side of things, I realized the inference issue when I started trying to work out how to do generics but it also affects lifetimes:

mod lifetimes {
    struct Foo<'a>{foo_str: &'a str}

    impl<'a> Foo<'a> {
        // Correctly gives no warnings here
        fn differnt_lifetimes() -> Foo<'static> {
            Foo { foo_str: "foo"}
        }

        // Correctly warns on both the declared return type and the struct literal
        fn explicit_lifetimes(&self) -> Foo<'a> {
            Foo::<'a> { foo_str: self.foo_str }
        }

        // Correctly warns on the declared return type, but not the literal even though it
        // should because I don't know how to get the inferred lifetimes
        fn inferred_lifetimes(&self) -> Foo<'a> {
            Foo { foo_str: self.foo_str }
        }
    }
}

So basically to fix this I just need to figure out how to get the list of lifetimes and generics after type inference, but I'm not sure how to do that.

[oli-obk]

So basically to fix this I just need to figure out how to get the list of lifetimes and generics after type inference, but I'm not sure how to do that.

Hmm... I think that's pretty hard to do in a general way. But we can special case struct constructors, by implementing visit_expr, checking for ExprStruct and obtaining the type of the expression. Then you can check whether the lifetime and generics of the expression match those of the impl block.

[MasonRemaley]

Makes sense, I'll try that out.

[MasonRemaley]

Sorry for the delay on this, been busy with other things--how do I actually obtain the type of the ExprStruct?

I'm testing against the same code as before, for the struct literal in the explicit_lifetimes function I end up visiting this expression:

Expr_::ExprStruct(Resolved(None, path(Foo<'a>)), [...], None)

But when I visit struct literal in the inferred_lifetimes function I get it without lifetimes since it's not written out explicitly, so it's doesn't seem to be as easy as just grabbing the info from the ast:

Expr_::ExprStruct(Resolved(None, path(Foo)), [...], None)

[oli-obk]

That's expected. You can use cx.tcx.tables.expr_type(expr.id) to obtain the real type and print that one's sty field. Maybe we can see the lifetime in there.

[MasonRemaley]

Ah okay, checked that out it does in fact give us the inferred parameters!

Here's what I have so far. It will print out the type for every struct literal that doesn't have Self as the last item in its path:

fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
    if let Expr_::ExprStruct(ref path, _, _) = expr.node {
        let segment = match *path {
            QPath::Resolved(_, ref path) => path.segments
                                                .last()
                                                .expect(SEGMENTS_MSG),
            QPath::TypeRelative(_, ref segment) => segment,
        };
        
        if segment.name != SelfType.name() {
            let ty = cx.tables.expr_ty(expr);
            println!("{:?}: {:?}", path, ty);
        }
    }
}

The next step is to determine if the type I've obtained here is actually equivalent to Self, and warn if it is. To do this, I need to actually know the type of Self.

Previously was this was accomplished by triggering a separate visitor when the late lint pass entered an impl block, and saving the self type as a field on that visitor. (Side note: I think this isn't exactly correct--you can nest structs and impls inside functions, so really the visitor needs to implement the visit_item function as well and only call walk_item if the item isn't an impl block, unless I'm misunderstanding something I haven't actually tested this yet.)

I'm not sure that this strategy works anymore anyway though. It seems that I actually need the instance of LateContext that gets passed into check_expr--if I try to obtain the expressions type using a different instance of late context (such as the one passed into check_item and then into the visitor) it just returns None.

Given this I'm not sure how to obtain the type of Self. The LateLintPass doesn't have callbacks on exiting an impl block or use the walk_* pattern AFAICT so I can't just like maintain a stack of Self types cause I wouldn't know when to pop from it. Is there an API I can call somewhere that just tells me what the current Self type is/gives me the correct LateContext for a given id or something? Or an alternative to LateLintPass that works more like Visitor does?

[oli-obk]

You can implement visit_expr on the visitor instead of check_expr on the lintcontext.

I'd ignore nested functions for now.

The none issue can be worked around by implementing visit_body. Look for other visit_body impla in clippy to see what you need to change in your visitor to make it work.

[oli-obk]

ping @MasonRemaley

[MasonRemaley]

Whoops, I've been busy and this fell off my radar--I'll mess with this some more this weekend and see if I can get something working.

[MasonRemaley]

Okay, haven't pushed yet as I have to clean up a few things but I've made some progress. I reimplemented the snippet from above as visit_expr and now have:

• A TyS for the struct literal I'm trying to compare to Self
• A Path representing Self, as well as a P<Ty> representing Self

Now I need to figure out how to compare these two things so I can print the lint message if they match up, or if that doesn't make sense I need to figure out how to get Self as a TyS to begin with. I'm not really sure where to go from here though--does it make sense to try to convert a Path or a P<Ty> to TyS?

---

Maybe a more general question, but are there docs/blog posts/such I could read anywhere that explain what some of these different structures in rustc are? Right now my strategy for figuring out if I can get one thing given another is to click through as many levels of the generated docs as I can, command+fing each page for the type I want--if I knew what they actually represented I'd know what actually makes sense and wouldn't have to brute force search everything.

[oli-obk]

So the TyS is the actual inner representation of a type in the compiler.

The P<Ty> is just a syntactical representation and it gets resolved in the resolve step in the compiler.

does it make sense to try to convert a Path or a P to TyS?

I don't think so. You need context for that conversion (e.g. the entirety of the resolve step).

I don't fully understand the situation, can you push the code so I can have a look?

are there docs/blog posts/such I could read anywhere that explain what some of these different structures in rustc are?

Unfortunately the most you get are the module docs in which the types, that you want to know stuff about, reside.

[MasonRemaley]

Ah thanks for the explanation about P<Ty> vs TyS, that makes sense. So probably I need to find some other way to get the type of Self.

I just pushed my work in progress. If you run it as is the visit_expr implementation will print the information I was able to get on each struct literal found in a method body as well as the info I was able to get on the Self for that scope. Here's a gist of the example code I've been feeding it to make sure it prints the inferred lifetimes and such.

(Uh also sorry for the weird commit history..I just reverted the changes from earlier before doing this because it was easier than dealing with the merge conflicts while I work out this detail, plan was to put them back in before committing because I didn't realize I was gonna get stuck here.)

[TimDiekmann]

@MasonRemaley Any news on this?

[flip1995]

Ping from triage @MasonRemaley: It looks like this PR hasn't received any updates in a while, so I'm closing it per our new guidelines. Thank you for your contributions and please feel free to re-open in the future.

[gnzlbg]

I think this would have solved #3859, i'm hitting it a lot, forcing me to disable the use_self lint globally all the time. Alternatively, we could disable the use_self lint by default (not use it on pedantic, etc.) until a fix for this lands.

[HKalbasi]

finished in #6179
@rustbot label -S-inactive-closed