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: don't complain about innocent newlines. #57059

Open
robpike opened this issue Dec 2, 2022 · 3 comments
Open

cmd/vet: don't complain about innocent newlines. #57059

robpike opened this issue Dec 2, 2022 · 3 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@robpike
Copy link
Contributor

robpike commented Dec 2, 2022

What version of Go are you using (go version)?

1.20

Does this issue reproduce with the latest release?

Yes

https://go.dev/play/p/_TMofzRghNL

package main

import "fmt"

const hello = `hello
`

func main() {
	fmt.Println(hello)
}

./x.go:9:2: fmt.Println arg list ends with redundant newline

I understand the desire to complain if there is an explicit newline in the text to fmt.Println, but when it's coming from a constant like this, I need to change the constant or use Print instead of Println. I keep hitting this problem with old code that has a nice header to be printed that desires a blank line. Requiring me to close the constant before the newline seems excessively fussy to me.

@ianlancetaylor ianlancetaylor added the NeedsFix The path to resolution is known, but the work has not been done. label Dec 3, 2022
@ianlancetaylor ianlancetaylor added this to the Backlog milestone Dec 3, 2022
@ianlancetaylor
Copy link
Contributor

CC @timothy-king

@rsc
Copy link
Contributor

rsc commented Dec 3, 2022

This particular message dates from the original commit of vet. (I reworded it to add the word 'redundant' later.) The extra precision of examining named constant arguments was introduced in Go 1.18.

% go1.16 vet /tmp/nl.go
% go1.17 vet /tmp/nl.go
% go1.18 vet /tmp/nl.go
# command-line-arguments
/tmp/nl.go:9:2: fmt.Println arg list ends with redundant newline
% 

This combination of factors suggests to me that we probably should leave things working as they are today:

  • The named constant propagation happened as a side effect of this change to handle literal constant expressions. That was an important precision improvement and it found real errors in code with long literal Printf formats split across multiple source lines using + of "" strings. And the longer the format the more it needs the check.
  • The expectation associated with the check has always been that if you really wanted a blank line you would write fmt.Print("abc\n\n") instead of fmt.Println("abc\n"), because the latter looks like a bug.
  • Looking at the actual values of named constants seems like exactly the thing you'd want vet to do, since seeing bugs is that much harder for people reading the code when the argument is a named constant rather than a literal.
  • In this case, just like the literal example, as written it is unclear whether the code really intends the blank line or its just a side effect of the way the code is written.
  • I don't see how to exclude this one case without losing other similar cases that probably are bugs.

Overall, it seems reasonable for vet to flag this case, which does require rewriting code that truly wants a blank line to make that intent clearer, such as by adding the blank line to the large constant literal and using fmt.Fprint.

@rsc rsc added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Dec 3, 2022
@gopherbot gopherbot removed the NeedsFix The path to resolution is known, but the work has not been done. label Dec 3, 2022
@timothy-king
Copy link
Contributor

Relevant recent discussion #49350

I don't see how to exclude this one case without losing other similar cases that probably are bugs.

We could plausibly suppress the "*ln" checks when the ast is exactly an identifier for a constant. The challenge here is it is going to be hard for users to predict when this warning applies and when it does not. Why does fmt.Println("abc\n") warn when const abc = "abc\n"; fmt.Println(abc) does not?

The other plausible distinguishing feature is that the surprises seem to occur when a `-quoted string literal is involved. We could suppress "*ln" warnings on those and constants that are set to be equal to those.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

5 participants