False positive for contexts stored in structs

[MichaelUrman]

Love the concept, but I got stung by a false positive involving a struct that contains a context. (Yeah, yeah, I know that's not preferred.) I can repro by adding either of the following to the test:

    var c = struct { Ctx context.Context }{ctx}
    for i := 0; i < 10; i++ {
        c := c
        c.Ctx = context.WithValue(c.Ctx, "key", i) // here
        c.Ctx = context.WithValue(c.Ctx, "other", "val")
    }

    for i := 0; i < 10; i++ {
        var c = struct { Ctx context.Context }{ctx}
        c.Ctx = context.WithValue(c.Ctx, "key", i) // here
        c.Ctx = context.WithValue(c.Ctx, "other", "val")
    }

My actual case is more like the first, but the second is more clearly a false positive. It looks like this comes down to not recognizing this as a case that breaks the chain, continuing due to t.String() != "context.Context" instead of breaking due to assignStmt.Tok == token.DEFINE. But solving this seems like a bear, especially as a syntactically very similar case should likely still be reported:

    var c = &struct { Ctx context.Context }{ctx}
    for i := 0; i < 10; i++ {
        c := c
        c.Ctx = context.WithValue(c.Ctx, "key", i) // want "nested context in loop"
        c.Ctx = context.WithValue(c.Ctx, "other", "val")
    }

I can't look today but let me know if you'd want me to attempt a solution. My guess is that pass.TypesInfo's Defs and Uses could disambiguate these, at least if filtered by appropriate levels of indirection in their containers.

[Crocmagnon]

Hello @MichaelUrman 👋🏻

Thank you for your report and coming up with test cases. I don’t have much free time now to come up with a solution myself but I’d be glad to review a PR, should you send one 🙂

I’d like to keep the implementation simple though, so I’m not sure if this is a use case I want to support if it makes things way more complex ; especially since there are “official” guidelines against it (https://go.dev/blog/context-and-structs).

I don’t have much experience with linters, so I admit that I don’t see a simple implementation from the top of my head. Again: happy to review a PR 🙂

[MichaelUrman]

I found enough time to give this a shot. The added complexity seems within reason to me, so I opened a PR. I leave the assessment of the additional complexity and the choice to you. (I won't be offended either way!)

[Crocmagnon]

Thank you for your contribution and your time! I released v0.3.0 which contains your fix and some dependency udpates.

[tmc]

func WithContext(ctx context.Context) SqliteChatMessageHistoryOption {
    return func(m *SqliteChatMessageHistory) {
        m.Ctx = ctx
    }
}

this apparently is picked up by this linter so this may still be an issue

[Crocmagnon]

Looks like it’s working as expected: https://github.com/Crocmagnon/fatcontext/pull/9/files#diff-6cae0614464d2fe2003259c4f94f3e5d25c1c661c1c3e4a0dd28e27e93df93d0R58