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

cmd/vet: reevaluate printf check #49350

Open
hyangah opened this issue Nov 4, 2021 · 28 comments
Open

cmd/vet: reevaluate printf check #49350

hyangah opened this issue Nov 4, 2021 · 28 comments

Comments

@hyangah
Copy link
Contributor

@hyangah hyangah commented Nov 4, 2021

Recent work on #30436 improved the accuracy of the printf check greatly, and it started to uncover style issues previously undetected. For example, it detected problems like #49322, fmt.Println arg list ends with redundant new line.

Even though I agree that this is a useful information in general, I want us to think if this type of check is severe enough to justify breaking existing tests when users upgrade go to 1.18.

From @dmitshur's comment #49322 (comment)

It looks like it was added in the "initial commit" for vet in 2010, CL 3522041. Back then, it worked on *ast.BasicLit only. It's possible #30436 is a bigger improvement for other print checks like "possible formatting directive", and maybe it scales less well for the newline check.

Unlike in 2010, now vet is part of go test. Improved accuracy is great, but it looks to me some of the checks included in the initial commit may be more suitable for linters or gopls.

@hyangah hyangah changed the title vet: reevaluate printf check cmd/vet: reevaluate printf check Nov 4, 2021
@thanm
Copy link
Contributor

@thanm thanm commented Nov 4, 2021

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Nov 4, 2021

I believe we should include it in the beta and see.
Improved precision in vet checks only breaks 'go test', not 'go build', which is exactly how it was designed to work.

Loading

@timothy-king
Copy link
Contributor

@timothy-king timothy-king commented Nov 4, 2021

IMO here are the 3 relevant cases of fmt.Println arg list ends with redundant new line to consider arising from #49322 :

  1. fmt.Println("<loremipsum>\n")
  2. const x = "<loremipsum>\n"; fmt.Println(x)
  3. const p = "ipsum>\n"; fmt.Println("<lorem"+p)

[edited: the examples which previously used variables. I was wrong that these were involved.]

Thanks to @dmitshur's detective work it is clear case 1 is as old as govet. 2 should also have previously reported. #49322 was caused by changing 3 to being reported.

Going through vet's inclusion criteria. IIUC the reason for flagging any of these is that ending with "\n" in a "ln" function is considered to be a correctness problem as this is likely not what the programmer intended to output. So correctness box ticked. As yesterday indicates the Frequency box is clearly ticked. So we are really going to be debating vet's Precision criteria. Is each case getting the false positive to false negative ratio right for the programmer's intention of including the extra "\n" within Printlns?

Given the age of cases 1 and 2, those are probably are getting it right. (Right?) Evidence for case 3 would be helpful.

I am also inclined to wait and see in the beta.

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Nov 4, 2021

Can you please clarify what has changed in Go 1.18?

It seems like case (1) should have been reported before. Why was it not?

Cases (2) and (3) seem like maybe an overreach:
were we doing anything involving variable assignment propagation before Go 1.18?

What about fmt.Println("<lorem" +"ipsum>\n")?
My undersatnding was that this was the case we were trying to fix, not (2) or (3).

Loading

@timothy-king
Copy link
Contributor

@timothy-king timothy-king commented Nov 4, 2021

@rsc My examples were bad. We are still not going through variables. Just "const x =". I've edited them. The only case I believe has changed in 1.18 was case 3.

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Nov 4, 2021

It sounds like you are confirming that the only thing that changed in vet is handling of +'ed string constants.
I believe we should ship that. I've definitely had real Printf bugs in multiline formats in cmd/go.
Generally speaking, it is OK to add new checks to vet even though they break 'go test'.
(If we couldn't, we'd never be able to add a new check to vet ever again!)

Loading

@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Nov 5, 2021

I tested the vet change on all golang.org/x repos and it found issues in x/tools and x/exp, the rest were clean:

# golang.org/x/tools/cmd/cover
cmd/cover/cover.go:45:2: fmt.Fprintln arg list ends with redundant newline
# golang.org/x/tools/cmd/eg
cmd/eg/eg.go:62:3: fmt.Fprint call has possible formatting directive %s
# golang.org/x/tools/cmd/callgraph
cmd/callgraph/main.go:169:3: fmt.Fprintln arg list ends with redundant newline
# golang.org/x/tools/cmd/gomvpkg
cmd/gomvpkg/main.go:86:3: fmt.Println arg list ends with redundant newline
# golang.org/x/tools/cmd/gorename
cmd/gorename/main.go:49:3: fmt.Println arg list ends with redundant newline
# golang.org/x/tools/cmd/gotype
cmd/gotype/gotype.go:170:2: fmt.Fprintln arg list ends with redundant newline
# golang.org/x/tools/cmd/guru
cmd/guru/main.go:108:2: fmt.Fprintln arg list ends with redundant newline
# golang.org/x/exp/vulndb/govulncheck
govulncheck/main.go:84:24: fmt.Fprintln arg list ends with redundant newline
govulncheck/main.go:88:3: fmt.Fprintln arg list ends with redundant newline

(Source: https://storage.googleapis.com/go-build-log/624eb6f7/linux-amd64_f52d45a1.log and https://storage.googleapis.com/go-build-log/624eb6f7/linux-amd64_de3063d6.log from CL 361094 slowbots.)

Looking through the findings, they're almost all instances of CLI commands that print a usage string that happens to end with a newline, and rely on fmt.Fprintln to add a second newline. (Like cmd/cover and cmd/trace from #49322.)

The one "fmt.Fprint call has possible formatting directive %s" finding from x/tools/cmd/eg happens because its -help output implementation uses fmt.Fprint(os.Stderr, eg.Help) to print a help string, and that string is a constant that contains "%s":

const Help = `
This tool implements example-based refactoring of expressions.

[...]

The expression statement form is useful when the expression has no
result, for example:

 	func before(msg string) { log.Fatalf("%s", msg) }
 	func after(msg string)  { log.Fatal(msg) }

[...]
`

If we vendor the latest x/tools into gotip now so this is included in beta 1, it'll create red rows for x/tools and x/exp on build.golang.org, unless they're updated first. Please let me know how you'd like to proceed after considering this new information.

Loading

@bcmills
Copy link
Member

@bcmills bcmills commented Nov 5, 2021

If we vendor the latest x/tools into gotip now so this is included in beta 1, it'll create red rows for x/tools and x/exp on build.golang.org, unless they're updated first.

That would mask any other regressions in those repos in the interim — it doesn't seem viable to me unless the fix CLs are already ready to merge, and at that point we may as well just merge them before the vendor update and avoid the breakage in the first place.

Loading

@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Nov 5, 2021

I agree. If vet owners suggest to include this change in beta 1, then the findings in x/tools and x/vet need to be resolved first.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 5, 2021

Change https://golang.org/cl/361716 mentions this issue: cmd/cover: avoid printing a redundant newline

Loading

@zpavlinovic
Copy link
Contributor

@zpavlinovic zpavlinovic commented Nov 5, 2021

I can send the changes to the detected issues. The eg example I believe will need a change to the checker, let me see what I can do about it.

Loading

@zpavlinovic
Copy link
Contributor

@zpavlinovic zpavlinovic commented Nov 5, 2021

Regarding "fmt.Fprint call has possible formatting directive %s" finding from x/tools/cmd/eg, the tool is actually doing what it is supposed to do AFAIK. I can see several options for mitigating this:

  1. Change the help string
  2. Make some sort of a (generalized) exception for the help string
  3. Revert the introduced change but only for the has possible formatting directive case.

My hunch is telling me 3). Thoughts?

Loading

@bcmills
Copy link
Member

@bcmills bcmills commented Nov 5, 2021

I would also lean toward (3).

We could also do (1) for the short term to get the vendor update unblocked, followed by (3) as a followup before the final 1.18 release.

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Nov 5, 2021

We need to fix our own code. Let's do that.
(The big const in the eg help text can easily change to a var.)

I've commented twice in favor of keeping the change and explained why.
@bcmills and @zpavlinovic you say you are in favor of reverting but did not explain why.
Why should we revert this?
(And if we should revert it, are we saying we have to stop adding any new checks to vet?)

Loading

@zpavlinovic
Copy link
Contributor

@zpavlinovic zpavlinovic commented Nov 5, 2021

My suggestion was not reverting the whole change, but only the part for formatting directive case. I see nothing wrong with the help string in the eg package, so reporting that looks like an FP to me (which is in fact blocking things). The reverting would happen until we figure out a better solution.

Loading

@bcmills
Copy link
Member

@bcmills bcmills commented Nov 5, 2021

Looking back at #30436, the has possible formatting directive check is exactly the original bug report.

So, let's keep it and go with just option (1). (Honestly, for these uses, os.Stdout.WriteString is probably even clearer still — these days fmt.Print and fmt.Println don't really add any value for single strings.)

Loading

@zpavlinovic
Copy link
Contributor

@zpavlinovic zpavlinovic commented Nov 5, 2021

Looking back at #30436, the has possible formatting directive check is exactly the original bug report.

So, let's keep it and go with just option (1). (Honestly, for these uses, os.Stdout.WriteString is probably even clearer still — these days fmt.Print and fmt.Println don't really add any value for single strings.)

I like this since, instead of changing the string, we would just change the printing mechanism.

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Nov 5, 2021

FWIW, I would rather use s/const/var/ to make clear that it's a good workaround for a spurious message.
One of the requirements for a vet check is that there be a clear, easy way to disable a false positive.
Making the argument not a constant is that way, for this check.
In contrast, changing fmt.Print to os.Stdout.WriteString is not a general solution.

Loading

@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Nov 5, 2021

For non-os.File's, io.WriteString can be used instead. Is there a reason modifying a const into var would be better, given it'd allow other code to unintentionally modify something that was meant to be a constant?

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Nov 5, 2021

No one is going to unintentionally modify main.Help.

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Nov 5, 2021

Another possibility is

help := Help // hide %s from vet
fmt.Print(help)

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 5, 2021

Change https://golang.org/cl/361854 mentions this issue: vulndb/govulncheck: avoid printing redundant newlines

Loading

gopherbot pushed a commit to golang/exp that referenced this issue Nov 5, 2021
For golang/go#49350

Change-Id: I6d3c50f56a61e666235e6d2e0ba3231768575218
Reviewed-on: https://go-review.googlesource.com/c/exp/+/361854
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>
Reviewed-by: Roland Shoemaker <roland@golang.org>
@zpavlinovic
Copy link
Contributor

@zpavlinovic zpavlinovic commented Nov 5, 2021

Another possibility is

help := Help // hide %s from vet fmt.Print(help)

Also quiet neat. If there aren't any objections, I will send this fix then.

Loading

@timothy-king
Copy link
Contributor

@timothy-king timothy-king commented Nov 5, 2021

Revert the introduced change but only for the has possible formatting directive case.

Bit late to the party, but here are my 2 cents. I don't think we should be trying to adjust the checker to "fix forward" on features like false positives suppression at the moment. I'd prefer a revert of the original change to printf over this option.

These 2 cents might not matter as we seem to be either fixing the help strings or s/const/var/ at the moment.

Loading

gopherbot pushed a commit to golang/tools that referenced this issue Nov 8, 2021
For golang/go#49350

Change-Id: Ic566112ff8a8707998b461d2b99fca852f2972f8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/361716
Run-TryBot: Zvonimir Pavlinovic <zpavlinovic@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Zvonimir Pavlinovic <zpavlinovic@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@guodongli-google
Copy link

@guodongli-google guodongli-google commented Nov 9, 2021

I am yet to be convinced that fmt.Println arg list ends with redundant new line is always a bug. If the constant string is short then it should be a TP where the programmer adds an extra "\n" that is unnecessary:

fmt.Println("<loremipsum>\n")

If the string is long and dynamically constructed, and is to be displayed in multiple lines, then using fmt.Println ending with a new line may actually save one line of code fmt.Println().

One case is that if the string already contains "\n" in the middle, then it is very likely that it is a multi-line string, and the vet checker should suppress the finding:

  const x = "<loremipsum>\n<loremipsum>\n"; fmt.Println(x)   // should not report 

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 9, 2021

Change https://golang.org/cl/361094 mentions this issue: all: update vendored golang.org/x/tools for Go 1.18 release

Loading

gopherbot pushed a commit that referenced this issue Nov 9, 2021
The Go 1.18 code freeze has recently started. This is a time to update
all golang.org/x/... module versions that contribute packages to the
std and cmd modules in the standard library to latest master versions.

This CL updates only the tools module, keeping mod unchanged because
its lastest commit isn't ready to be vendored yet.

For #36905.
Updates #49350.

Change-Id: Ib39713d28a55fc9ec79058aab9919eba912def5f
Reviewed-on: https://go-review.googlesource.com/c/go/+/361094
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
@bcmills
Copy link
Member

@bcmills bcmills commented Nov 12, 2021

@guodongli-google

If the string is long and dynamically constructed, and is to be displayed in multiple lines,

The existing vet warning only applies to arguments that are constants. If the string is dynamically-constructed (or even just declared as a var), it will not trigger the check.

Loading

@guodongli-google
Copy link

@guodongli-google guodongli-google commented Nov 15, 2021

@guodongli-google

If the string is long and dynamically constructed, and is to be displayed in multiple lines,

The existing vet warning only applies to arguments that are constants. If the string is dynamically-constructed (or even just declared as a var), it will not trigger the check.

Right. I meant something like:

  const x1 = "<loremipsum>\n"
  fmt.Println(x1 + x1)   // should not report 

Such cases should be rare though.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
9 participants