feat: add fatcontext linter

[Crocmagnon]

As described in a blog post I wrote (https://gabnotes.org/fat-contexts/), we've been bitten by contexts not shadowed inside loops.
This linter addresses this issue.

Github repo: https://github.com/Crocmagnon/fatcontext

[boring-cyborg]

Hey, thank you for opening your first Pull Request !

[CLAassistant]

All committers have signed the CLA.

[ccoVeille]

Thanks for this linter. I got same issue with a very long ctx in a loop last week

[ldez]

In order for a pull request adding a linter to be reviewed, the linter and the PR must follow some requirements.

• The CLA must be signed

Pull Request Description

• It must have a link to the linter repository.
• It must provide a short description of the linter.

Linter

• It must not be a duplicate of another linter or a rule of a linter. (the team will help to verify that)
• It must have a valid license (AGPL is not allowed) and the file must contain the required information by the license, ex: author, year, etc.
• The linter repository must have a CI and tests.
• It must use go/analysis.
• It must have a valid tag, ex: v1.0.0, v0.1.0.
• It must not contain init().
• It must not contain panic().
• It must not contain log.fatal(), os.exit(), or similar.
• It must not modify the AST.
• It must not have false positives/negatives. (the team will help to verify that)
• It must have tests inside golangci-lint.

The Linter Tests Inside Golangci-lint

• They must have at least one std lib import.
• They must work with T=<name of the linter test file>.go make test_linters. (the team will help to verify that)

.golangci.next.reference.yml

• The file .golangci.next.reference.yml must be updated.
• The file .golangci.reference.yml must NOT be edited.
• The linter must be added to the list of available linters (alphabetical case-insensitive order).
  • enable and disable options
• If the linter has a configuration, the exhaustive configuration of the linter must be added (alphabetical case-insensitive order)
  • The values must be different from the default ones.
  • The default values must be defined in a comment.
  • The option must have a short description.

Others Requirements

• The files (tests and linter) inside golangci-lint must have the same name as the linter.
• The .golangci.yml of golangci-lint itself must not be edited and the linter must not be added to this file.
• The linters must be sorted in the alphabetical order (case-insensitive) in the lintersdb/builder_linter.go and .golangci.next.reference.yml.
• The load mode (WithLoadMode(...)):
  • if the linter doesn't use types: goanalysis.LoadModeSyntax
  • goanalysis.LoadModeTypesInfo required WithLoadForGoAnalysis() in the lintersdb/builder_linter.go
• The version in WithSince(...) must be the next minor version (v1.X.0) of golangci-lint.
• WithURL() must contain the URL of the repository.

Recommendations

• The file jsonschema/golangci.next.jsonschema.json should be updated.
• The file jsonschema/golangci.jsonschema.json must NOT be edited.
• The linter repository should have a readme and linting.
• The linter should be published as a binary. (useful to diagnose bug origins)

---

The golangci-lint team will edit this comment to check the boxes before and during the review.

The code review will start after the completion of those checkboxes (except for the specific items that the team will help to verify).

If the author of the PR is a member of the golangci-lint team, he should not edit this message.

This checklist does not imply that we will accept the linter.

[ccoVeille]

As described in a blog post I wrote (https://gabnotes.org/fat-contexts/), we've been bitten by contexts not shadowed inside loops. This linter addresses this issue.

Github repo: https://github.com/Crocmagnon/foreshadow

I can tell you your linter helped me to spot 4 errors in my code. Thanks

[ldez]

First, I understand the goal of this linter.

But I have a problem with the fact to "fight" against one of the classical govet rules: shadow

The shadowing is a well-known problem, it's weird to suggest shadowing variables instead of just using a dedicated one.

[Crocmagnon]

it's weird to suggest shadowing variables instead of just using a dedicated one

Contexts are often named ctx more than anything else. The first reaction when encountering the shadowing error from govet is to remove the shadowing while keeping the same variable name, leading to the kind of context misuse described in my blog post.

E.g.:

ctx := context.Background()
for /* cond */ {
    ctx := context.WithValue(ctx, "key", "val")
    // ...
}

will likely become

ctx := context.Background()
for /* cond */ {
    ctx = context.WithValue(ctx, "key", "val")
    // ...
}

or even

ctx := context.Background()
for /* cond */ {
    ctx := context.WithValue(ctx, "key", "val") //nolint
    // ...
}

where the shadowing would be more appropriate (or the definition of a new variable).

In our case, we disabled the shadowing linter on contexts specifically because developers in the team found it too noisy/annoying, not realizing we were exposing ourselves to another kind of issue.

Also, we prefer keeping the name ctx for context values because it's so ubiquitous it becomes strange to read code that uses another name + it's too easy to use an upper context, not realizing a new one has been defined with another name in a block.

[ldez]

As I said, I already understand the goal of this linter.
My remarks are about suggesting shadowing.

In our case, we disabled the shadowing linter
...
Also, we prefer keeping the name ctx

This is your story with shadowing, context, and naming, I understand.
But from the POV of golangci-lint, having two linters that fight against is not something desirable, and each user has a different story.

To be clear, I suggest not incited to shadow but to fix the problem: inside the message not talking about shadow but about nested context.
Users decide what action they wish to take.

I know that you implemented suggestedFix (not supported by golangci-lint) and it's not a problem to create a shadow as a fix because, after the fix, the users will have to "choose" between your linter and shadow reports. (but once again, suggestedFix is not supported by golangci-lint).

I also suggest renaming your linter (ex: fatcontext).
The current name is only focused on the shadowing but it's not what your linter detects.
Your linter detects invalid usage of context.
The shadowing is just one of the possible solutions.

[Crocmagnon]

To be clear, I suggest not incited to shadow but to fix the problem: inside the message not talking about shadow but about nesting context.

That's much clearer, thanks for taking the time to explain.

I changed the linter according to your suggestions, released v0.2.0 and updated the PR.

[ldez]

[ ] It must not contain panic().

You still have a usage of panic: https://github.com/Crocmagnon/fatcontext/blob/781d68514681a8a8ed2a88cb2e7e8f49e88f5711/pkg/analyzer/analyzer.go#L106

[Crocmagnon]

🤦🏻‍♂️ fixed in v0.2.2 👍🏻

EDIT: curious, how to suggest fixes with golangci-lint if SuggestedFix isn't supported?

[ldez]

curious, how to suggest fixes with golangci-lint if SuggestedFix isn't supported?

First, at some point, we will support it.

Currently, auto-fixing is limited to a few linters:

• the linters based on the "gofmt" style (not recommended)
• the linters like protogetter which created a specific "mode":

  golangci-lint/pkg/golinters/protogetter.go

  Line 28 in c037920

  cfg.Mode = protogetter.GolangciLintMode

[bombsimon]

For reference the tracking issue to support SuggestedFixes is #1779

[ldez]

@bombsimon the topic is in the top 5 of my backlog, for now, I waiting for bug fixes of v1.57 but after that, I will start again with my multiple PRs every day 😸

[Antonboom]

I feel some interception with https://github.com/kkHAIKE/contextcheck 🤔
(already in golangci-lint)

[Crocmagnon]

Tested today on our codebase at work, all reports were real cases except for one intentional (a function which returns the context it modifies). It also properly detected known cases.

[ldez]

@Antonboom contextcheck, fatcontext are complementary: contextcheck verifies the context continuity, and fatcontext verifies the nested context which doesn't impact the continuity of the context.

[Crocmagnon]

Hello!
Quick questions:

• what are the next steps for this PR?
• do you expect something from me to move this forward?
  (not impatient, just checking if I have something to do 😀)

[ldez]

LGTM

[batazor]

I create a context and want to send it to each element, I think this is not a frequent but valid case, is it not?

// start tracing
newCtx, span := otel.Tracer(fmt.Sprintf("saga: %s", s.name)).Start(ctx, fmt.Sprintf("saga: %s", s.name))
defer span.End()

for _, step := range initSteps {
	step.ctx = newCtx // nested context in loop (fatcontext)
	g.Go(step.Run)
}

[Crocmagnon]

@batazor in this case, it looks like g.Go is from errgroup. I would use WithContext instead of passing a context to a struct. Or closures.