Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

log/slog: add vet checks for variadic ...any inputs #59407

Closed
AndrewHarrisSPU opened this issue Apr 3, 2023 · 8 comments
Closed

log/slog: add vet checks for variadic ...any inputs #59407

AndrewHarrisSPU opened this issue Apr 3, 2023 · 8 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) Proposal Proposal-Accepted
Milestone

Comments

@AndrewHarrisSPU
Copy link

AndrewHarrisSPU commented Apr 3, 2023

The passing of ...any input parameters to a number of functions and methods is a key feature of the slog package, with ~20 instances that convert args ...any to slices of Attrs. These argument lists ask for a particular sequencing of string keys, any values, or Attr arguments. At run time, malformed lists emit !BADKEY keys rather than anything more fatal. This is reminiscent of how fmt.Printf can emit disrupted output when formatting verbs don't match arguments. Analysis under go vet exists for this kind of fmt.Printf scenario. Similar tooling for slog could be developed.

Two rules that could be applied by analysis:

Rule 1 When arguments are given directly to a slog function accepting args ...any, malformed calls that would otherwise result in a "!BADKEY" constant are flagged. For example,

slog.Info("", "a", 1, 2, 3) // a=1 !BADKEY=2 !BADKEY=3

A strong, laudable example of prior art here is given in a package loggercheck from @timonwong, in a checker of `zapcore.Fields.

Rule 2 Disallow arguments that are splatted into a slog function accepting args ...any

This rule generates false positives and asks some code to be more verbose than it would be otherwise, but rejects any code that can't be statically determined to be well formed by the first rule. For example:

// flag this in vet checks because of args... splatting
args := []any{"a", 1, 2, 3}
slog.Info("", args...)

// could be written more verbosely, for example:
attrs := []slog.Attr{
	slog.Any("a", 1),
 	slog.Any(2, 3), // cannot use 2 (untyped int constant) as string value in argument to slog.Any
}
slog.LogAttrs(slog.LevelInfo, "", attrs...)

One question I'd like to raise with this proposal: is go vet tooling appropriate?

If so, another question is: what rules make sense? Rule 1 alone seems unlikely to generate false positives, and does a fair amount to catch malformed logging calls. Rule 2 alone would be unusual. Rule 1+2 is more adventurous than just rule 1 - it's tight enough to raise different questions, but seems to make "!BADKEY" in output highly unlikely.

Any go vet analysis is well qualified by this sentence from vet documentation:

Vet uses heuristics that do not guarantee all reports are genuine problems, but it can find errors not caught by the compilers.

@gopherbot gopherbot added this to the Proposal milestone Apr 3, 2023
@ianlancetaylor
Copy link
Contributor

CC @jba

@jba
Copy link
Contributor

jba commented Apr 4, 2023

Thanks, @AndrewHarrisSPU, for adding this proposal. I intended it to be part of the main proposal (#56345) but didn't include it.

I think Rule 1 alone is the right choice. Rule 2 would get in the way of perfectly fine code.

@rsc
Copy link
Contributor

rsc commented Apr 5, 2023

Rule 1 is the vet way. Rule 2 is not (vet eschews false positives).

@rsc
Copy link
Contributor

rsc commented Apr 6, 2023

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Apr 12, 2023

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Apr 19, 2023

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: log/slog: add vet checks for variadic ...any inputs log/slog: add vet checks for variadic ...any inputs Apr 19, 2023
@rsc rsc modified the milestones: Proposal, Backlog Apr 19, 2023
@gopherbot
Copy link

Change https://go.dev/cl/487475 mentions this issue: go/analysis/passes/slogargs: add slog analyzer

@jba
Copy link
Contributor

jba commented Jan 23, 2024

This was implemented and shipped with Go 1.21.

@jba jba closed this as completed Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) Proposal Proposal-Accepted
Projects
Status: Accepted
Development

No branches or pull requests

6 participants