Only enforce context-only when a context.Context is within scope

[ktarplee]

#13 added context-only but it is not very useful when in some functions you do not have a context passed in. I either need to add no lint to all the call sites where there is no context or I need to disable the linter. The linter is useful so I would prefer that the linter only enforce context-only when a variable of type context.Context is passed into the function or better yet within the scope when the log method is called. If not context.Context is present then it is not a lint issue if we call log.Info().

[tmzane]

Hello,

context-only is optional and disabled by default. The idea was to enforce it only for slog handlers that make use of context.Context. In your case, I suggest not to enable it in the first place.

[ktarplee]

Hello,

context-only is optional and disabled by default. The idea was to enforce it only for slog handlers that make use of context.Context. In your case, I suggest not to enable it in the first place.

My point is that there are parts of code where I use contexts (because I am doing network IO) and other parts of my code where I am not using contexts. In both places I am logging. In one it is with InfoContext() and the other Info(), respectively. So, yes, I can turn it off for my entire project but then the majority of the code that uses contexts would not benefit from the linter reminding me to use InfoContext().

[tmzane]

I get it now. We definitely won't change the default behavior of context-only, but we could implement your idea as context-only: scope or something like that.

[mattdowdell]

@ktarplee How would you differentiate between 'context shouldn't be in scope' and 'context isn't in scope but should be'?

[ktarplee]

@ktarplee How would you differentiate between 'context shouldn't be in scope' and 'context isn't in scope but should be'?

There is no way to easily determine if the context should be in scope. Bringing in context often requires a function signature change and is impractical or undesired for some functions. All I am asking here is that if there is a variable of type context.Context in the lexical scope (excluding global or package scopes or object fields) then the rule is enabled. If not then the linter is disabled for that scope.

[mattdowdell]

@ktarplee How would you differentiate between 'context shouldn't be in scope' and 'context isn't in scope but should be'?

There is no way to easily determine if the context should be in scope. Bringing in context often requires a function signature change and is impractical or undesired for some functions. All I am asking here is that if there is a variable of type context.Context in the lexical scope (excluding global or package scopes or object fields) then the rule is enabled. If not then the linter is disabled for that scope.

I understand the ask, and initially it seems reasonable. My concern is that this might could create awkward to track technical debt. For example, a function is created without a context argument, either by accident or as a reaction to the linter and the proposed exclusion. But a context should be present as it contains metadata used by the logger to create fields at runtime.

Perhaps it's just a matter of personal preference and this idea just 'isn't for me'. But I wonder if the exceptions to the rule are rare enough that a nolint directive would be sufficient.

[tmzane]

@ktarplee I implemented a prototype in #33.

It detects function declarations with a context.Context as the first parameter in the scope of a logger call:

func foo(ctx context.Context) {
    slog.Info("msg") // <- InfoContext
}

func bar() {
    slog.Info("msg") // <- OK
}

I don't think it should detect local context.Context variables, e.g. ctx, cancel := context.WithTimeout(...), because there would be false positives. It would also be kinda hard to implement.

Could you try it on your codebase and tell me if it works for you?

Here's how to install:

go install go-simpler.org/sloglint/cmd/sloglint@context-only-scope
~/go/bin/sloglint -context-only=scope ./...

[ktarplee]

@tmzane It works well! Thank you.

[kellen-miller]

This would be a great feature to add, was just about to open a request for this.

[ktarplee]

Do you plan on merging this feature?

[tmzane]

Working on it 😎

[tmzane]

Should be available in the next golangci-lint release 🎉