Skip to content

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

Closed
@AndrewHarrisSPU

Description

@AndrewHarrisSPU

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.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions