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

Jump to bottom

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

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
Go1.18

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 •
edited

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
x/tools: print check misses concatenated strings
cde5df5 
Fixes golang/go#30436
@AlexanderYastrebov AlexanderYastrebov mentioned this issue Oct 19, 2021
Closed
AlexanderYastrebov added a commit to AlexanderYastrebov/tools that referenced this issue Oct 19, 2021
@AlexanderYastrebov
x/tools: print check misses concatenated strings
3c93a47 
Fixes golang/go#30436
@gopherbot
Copy link

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
@AlexanderYastrebov
x/tools: print check misses concatenated strings
3ee8e18 
Fixes golang/go#30436
@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 dmitshur mentioned this issue Nov 3, 2021
Closed
@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
@zpavlinovic
Replace fmt.Println with fmt.Printf for newline-ending string.
0f04ff8 
This would otherwise be caught with the improved vet printf
checker (golang/go#30436).
@zpavlinovic zpavlinovic mentioned this issue Nov 3, 2021
Merged
@gopherbot
Copy link

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

@gopherbot
Copy link

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
@zpavlinovic
cmd/cover: use fmt.Print for newline-ending fixed string
9cf6711 
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
@zpavlinovic
Replace fmt.Println with fmt.Printf for newline-ending string.
d4db37f 
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
@zpavlinovic
Avoid adding a redundant newline when printing weblistPageClosing (#665)
f987b9c 
* 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.
@hyangah hyangah mentioned this issue Nov 4, 2021
Closed
@golang golang locked and limited conversation to collaborators Nov 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
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
Milestone
Go1.18
Development

Successfully merging a pull request may close this issue.

x/tools: print check misses concatenated strings AlexanderYastrebov/tools
6 participants
@imirkin @dmitshur @ALTree @bcmills @alandonovan @gopherbot