Skip to content

cmd/vet: printf check misses strings that are + together #30436

New issue

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

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

Already on GitHub? # to your account

Closed
imirkin opened this issue Feb 27, 2019 · 6 comments
Closed

cmd/vet: printf check misses strings that are + together #30436

imirkin opened this issue Feb 27, 2019 · 6 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@imirkin
Copy link

imirkin commented Feb 27, 2019

With this example put into https://play.golang.org/ (and go1.11), there is no warning. Joining the two strings into 1 results in a warning.

package main

import (
	"fmt"
)

func main() {
	fmt.Println("Hello, playground %d" + "", 5)
}

Some people split long strings, which is where I encountered it.

@bcmills bcmills changed the title vet: printf check misses strings that are + together cmd/vet: printf check misses strings that are + together Feb 27, 2019
@bcmills
Copy link
Contributor

bcmills commented Feb 27, 2019

CC @alandonovan @josharian @mvdan @ianthehat for cmd/vet

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 27, 2019
@alandonovan
Copy link
Contributor

alandonovan commented Feb 27, 2019

Thanks, this is indeed a bug. The checkPrint function in golang.org/x/tools/go/analysis/passes/printf/printf.go looks for ast.BasicLit, which is a relic of the older implementation which could not assume well-typed syntax trees. Now it's better to look for expressions that are string constants, independent of their syntax, as we do when checking Printf (not Println as in your example).

@ALTree ALTree added this to the Unplanned milestone Sep 27, 2019
AlexanderYastrebov added a commit to AlexanderYastrebov/tools that referenced this issue Oct 19, 2021
AlexanderYastrebov added a commit to AlexanderYastrebov/tools that referenced this issue Oct 19, 2021
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/356830 mentions this issue: x/tools: print check misses concatenated strings

AlexanderYastrebov added a commit to AlexanderYastrebov/tools that referenced this issue Nov 2, 2021
@dmitshur dmitshur modified the milestones: Unplanned, Go1.18 Nov 3, 2021
@dmitshur dmitshur added Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Nov 3, 2021
@dmitshur
Copy link
Contributor

dmitshur commented Nov 3, 2021

This change is finding 3 instances in the Go tree itself:

# cmd/cover
cmd/cover/cover.go:43:2: fmt.Fprintln arg list ends with redundant newline
# cmd/vendor/github.com/google/pprof/internal/report
cmd/vendor/github.com/google/pprof/internal/report/source.go:877:2: fmt.Fprintln arg list ends with redundant newline
# cmd/trace
cmd/trace/main.go:69:3: fmt.Fprintln arg list ends with redundant newline

Filed #49322 for resolving them.

zpavlinovic added a commit to zpavlinovic/pprof that referenced this issue Nov 3, 2021
This would otherwise be caught with the improved vet printf
checker (golang/go#30436).
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/361263 mentions this issue: src/cmd/cover: use fmt.Print for newline-ending fixed string

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/361265 mentions this issue: src/cmd/trace: use fmt.Print for newline-ending fixed string

gopherbot pushed a commit that referenced this issue Nov 4, 2021
This redundancy is now caught by the improved printf vet checker
(#30436).

Updates #49322

Change-Id: Id450247adc6fa28a9244c019be3c1b52c2d17f49
Reviewed-on: https://go-review.googlesource.com/c/go/+/361263
Run-TryBot: Zvonimir Pavlinovic <zpavlinovic@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Zvonimir Pavlinovic <zpavlinovic@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
zpavlinovic added a commit to zpavlinovic/pprof that referenced this issue Nov 4, 2021
This would otherwise be caught with the improved vet printf
checker (golang/go#30436).
aalexand pushed a commit to google/pprof that referenced this issue Nov 4, 2021
* Replace fmt.Println with fmt.Printf for newline-ending string.

This would otherwise be caught with the improved vet printf
checker (golang/go#30436).

* Replace fmt.Println with fmt.Printf for newline-ending string.

This would otherwise be caught with the improved vet printf
checker (golang/go#30436).

* Avoid adding a redundant newline when printing weblistPageClosing.
@golang golang locked and limited conversation to collaborators Nov 3, 2022
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants