False Positive

[dprotaso]

This was just flagged recently - but oddly it wasn't flagged before (maybe because we're moving to go1.23)

https://github.com/knative/pkg/blob/4ba3f1b39dbf3df9ac80de63421aafe372cb7742/webhook/testing/factory.go#L99

I think it's a false positive because it's not in a loop

 Running [/home/runner/golangci-lint-1.62.2-linux-amd64/golangci-lint run  --go=1.23] in [/home/runner/work/pkg/pkg] ...
  Error: webhook/testing/factory.go:99:3: nested context in function literal (fatcontext)
  		r.Ctx = ctx
  		^

[Crocmagnon]

Hello!
Thanks for reporting this :)
It's not in a loop but in a function literal (see the linter msg).
However, given the context of the code, I also feel like this is a false positive.

[Crocmagnon]

@dprotaso Could you please confirm which version of golangci-lint you're using?

[dprotaso]

our CI was using golangci-lint-1.62.2-linux-amd64

[Crocmagnon]

Hm interestingly this doesn't trigger:

func something() func() {
	return func() {
		r := struct {
			Ctx context.Context
		}{}
		ctx := r.Ctx
		ctx = context.WithValue(ctx, "key", "val")
		r.Ctx = ctx
	}
}

But this does:

type R struct {
	Ctx context.Context
}

func something() func(*R) {
	return func(r *R) {
		ctx := r.Ctx
		ctx = context.WithValue(ctx, "key", "val")
		r.Ctx = ctx // triggered on this line
	}
}

This is indeed a false positive IMO, an extended case of this test:

fatcontext/testdata/src/example.go

Lines 221 to 226 in 52f7fb5

	func okMiddleware2(ctx context.Context) func(ctx context.Context) error {
	return func(ctx context.Context) error {
	ctx = wrapContext(ctx)
	return doSomethingWithCtx(ctx)
	}
	}

We have a test case, now we need a fix :)

Do you want to work on this?

[Crocmagnon]

On second thought I’ll have to experiment a bit more to check whether that’s really a false positive. I’ll post back here once I’m done.

[dprotaso]

I’ll have to experiment a bit more to check whether that’s really a false positive

I guess if the function was called twice then you'd add the same values into the values linked list. I don't believe context.WithValue performs any deduping.

It seems like a gray area in my mind.

[dprotaso]

I'm ok with leaving this as is and I can add a nolint directive in my code

[Crocmagnon]

There is indeed no deduplication, that’s the whole point of this linter 🙂
I wrote about it in more details here: https://gabnotes.org/fat-contexts/

[dprotaso]

I guess though if the following isn't flagged then I wouldn't expect our code to trigger it

func Blah(r *R) {
  ctx := r.Ctx
  ctx = context.WithValue(ctx, "key", "val")
  r.Ctx = ctx // triggered on this line
}

[dprotaso]

A similar false positive is here

https://github.com/knative/serving/blob/c2f2b8b4581620ee3f5f9bbe53d97e035cbd1c94/pkg/apis/serving/metadata_validation_test.go#L309

	for _, tc := range cases {
		t.Run(tc.name, func(t *testing.T) {
			if tc.ctx == nil {
				tc.ctx = context.Background()
			}
			err := ValidateContainerConcurrency(tc.ctx, tc.containerConcurrency)
			if got, want := err.Error(), tc.expectErr.Error(); got != want {
				t.Errorf("\nGot:  %q\nwant: %q", got, want)
			}
		})
	}

[Crocmagnon]

Here's the "signature" of OP's code (simplified, marked as "experiment") compared to a known "fat" and a known "thin".

See this commit for reproducible results: Crocmagnon/fat-contexts-article-companion@7c777c5

So from my POV, it's not a false positive.

I guess though if the following isn't flagged then I wouldn't expect our code to trigger it

That's a false negative IMO, we should probably add this to the linter. I agree that the current behavior is not consistent.

Now for the knative/serving, we can probably make a few exceptions for standard known functions that return empty contexts, such as as context.Background and context.TODO.

TODO:

• Detect fat contexts in struct pointer in function arguments (see feat: detect potential issues with struct pointers #36)
• Ignore context.Background and context.TODO (see feat: ignore context.TODO and context.Background #35)

Feedback welcome on both PRs

[dprotaso]

So from my POV, it's not a false positive.

It depends on semantics - for example this is test setup code so we wouldn't be calling it twice. It's really application specific so I don't think a linter can have an opinion.

We have a lot of patterns where we have a function that decorates a context. Calling that function multiple times would make it fat but we don't expect consumers to do that ¯_(ツ)_/¯

[Crocmagnon]

Completed with #36, the linter now better detects struct pointer assignations and they're hidden behind a feature flag.

Follow golangci/golangci-lint#5334 for integration in golangci-lint.