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.

Sign up for GitHub

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

Jump to bottom

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

Closed
marwan-at-work opened this issue Dec 11, 2023 · 9 comments
Closed

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

marwan-at-work opened this issue Dec 11, 2023 · 9 comments
Labels
Proposal
Milestone
Proposal

Comments

@marwan-at-work
Copy link
Contributor

marwan-at-work commented Dec 11, 2023 •
edited

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 :)
@mauri870 mauri870 changed the title [Proposal] Add a function to detect formatting directives proposal: fmt: add a function to detect formatting directives in string Dec 12, 2023
@mauri870 mauri870 added this to the Proposal milestone Dec 12, 2023
@timothy-king
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

jba commented Dec 18, 2023

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
Copy link
Member

adonovan commented Dec 22, 2023 •
edited

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
Copy link
Contributor Author

@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
Copy link
Member

adonovan commented Feb 2, 2024

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
Copy link
Contributor

rsc commented Mar 13, 2024

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

@rsc
Copy link
Contributor

rsc commented Mar 15, 2024

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

@rsc
Copy link
Contributor

rsc commented Mar 27, 2024

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

@rsc rsc closed this as completed Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Proposal
Projects
Proposals
Status: Declined
Milestone
Proposal
Development

No branches or pull requests
8 participants
@dominikh @rsc @timothy-king @adonovan @gopherbot @mauri870 @marwan-at-work @jba