proposal: fmt: add a function to detect formatting directives in string

[marwan-at-work]

There are some tools out there that need/should check if a string has fmt.Printf formatting directives.

Therefore, it would be great if we had a function like the following

// HasFormattingDirective returns whether s has a fmt.Printf formatting directive
// "Hello %s" => true
// "Hello there" => false
func HasFormattingDirective(s string) bool

Motivation:

The new log/slog function signature is exactly the same as fmt's Printf function signature: func (string, ...any) but the expected arguments are not the same.

Therefore, it's easy to make a mistake for users migrating from log.Printf to slog by accidentally passing formatting directives.

logger.Info("username %s has %d errors", "John", 5)
Where the output ends up being {"message": "username %s has %d erros", "John": 5} instead of {"message": "username John has 5 errors"}

There's already a slog analysis package but it does not catch the above case.

While there's a printf analysis package that catches exactly this case when you pass formatting directives to non-formatting functions like fmt.Println("Hello %s", "John") // fmt.Println has possible Printf formatting directive. The analysis code for catching this error is here

Instead of copying the internal logic over, it might be nice to just create a std lib function (or under x/tools) that catches this case as I imagine there are other use cases out there where this function is needed.

Otherwise, this proposal can just be to update the slog Analysis package to catch the above case :)

[timothy-king]

There's already a slog analysis package but it does not catch the above case.

IIUC all that needs to happen is to for x/tools/go/analysis/passes/slog to warn when formatting directives are in the first argument and the first argument is a are constant strings. This leaves an escape hatch by using a variable. We could also not warn within ``` enclosed strings.

IIalsoUC this is not strictly a correctness criteria for log/slog so it will have false positives. OTOH this seems like it will prevent more bugs than it would cause pain. This is somewhat borderline for vet. Data could help, but it may be difficult to classify what was 'in developer's hearts' when they wrote the code in this case.

@jba Any thoughts on classifying formatting directives as a bug in log/slog?

@dominikh staticcheck candidate?

There are some tools out there that need/should check if a string has fmt.Printf formatting directives.

Could you maybe flesh this out a bit more? Right now it seems like this is to code analysis tools. And there is one additional analysis that has been identified as maybe benefiting, slog. So cloning the logic as needed seems okay. Are there other benefits for exposing this API?

[dominikh]

IIalsoUC this is not strictly a correctness criteria for log/slog so it will have false positives. OTOH this seems like it will prevent more bugs than it would cause pain. This is somewhat borderline for vet. Data could help, but it may be difficult to classify what was 'in developer's hearts' when they wrote the code in this case.

Would this case be so different from the "possible formatting directive" that gets flagged for the fmt.Println case? Both can have false positives in theory and don't seem to in practice.

[jba]

The slog pass is run during go test so it must have very few false positives. I think that checking for formatting directives in the first arg probably meets that bar, but it's worth noting.

[adonovan]

I think the predicate you are looking for is simply fmt.Sprintf(s) != s. There are three cases to consider:

• A string that does not contain "%" will format as itself.
• A string that contains "%%" will format as something other than itself, because "%%" becomes "%".
• A string that contains "%d" or any other format spec will format as something other than itself, because it lacks an operand and thus renders as "%!(NOVERB)" or "%!d(MISSING)" or similar, but never "%%".

All of these rules compose under concatenation.

[marwan-at-work]

@adonovan nice to hear from you and thanks for the response :)

That's definitely a clever way (and missed on my part) to be able to tell if the string has a formatting directive.

Question: should the printf analysis implementation be updated to do this as an example? https://github.com/golang/tools/blob/06407013ff725a89b552c86d78d310b93f80aa57/go/analysis/passes/printf/printf.go#L1052

Thanks!

[adonovan]

That's definitely a clever way (and missed on my part) to be able to tell if the string has a formatting directive.

Question: should the printf analysis implementation be updated to do this as an example? https://github.com/golang/tools/blob/06407013ff725a89b552c86d78d310b93f80aa57/go/analysis/passes/printf/printf.go#L1052

I think the answer has to be no, because the comment next to the regexp says that it intentionally excludes the "% " case so that (for example) "x % y" doesn't trigger a diagnostic. That function is using a heuristic to answer the question "does this look like it was intended for printf?", not "might this be a valid format string?"

See https://go.dev/play/p/dpWRELHLES0 for an example that defeats the heuristic.

@adonovan nice to hear from you and thanks for the response :)

Sorry for the delay. We should definitely grab that (much belated) beer we discussed at GothamGo.

[rsc]

Since we have an answer - fmt.Sprintf(s) != s - this can probably be declined.

[rsc]

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

[rsc]

No change in consensus, so declined.
— rsc for the proposal review group