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: cmd/vet: warn about // go:... #50898

Closed
odeke-em opened this issue Jan 28, 2022 · 12 comments
Closed

proposal: cmd/vet: warn about // go:... #50898

odeke-em opened this issue Jan 28, 2022 · 12 comments
Labels
FrozenDueToAge Proposal
Projects
Proposals (old)
Milestone
Proposal

Comments

@odeke-em
Copy link
Member

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

$ go version
go version devel go1.18-35b0db7607 Fri Jan 21 21:58:14 2022 +0000 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

Irrelevant

What did you do?

From some internal code at Orijtech Inc that caused unexpected problems after runtime and deployment; it used an embed pragma statement to embed a file but unfortunately we had a space so // go:embed file.txt instead of //go:embed file.txt, the code compiled alright and was deployed but caused a bit of a scare until a colleague noticed the bug.

package main

import _ "embed"

// go:embed hello.txt
var file string

func main() { println(file) }

What did you expect to see?

I expected an error reported by a static analyzer or that it fails to compile this code. We have advanced tooling but unfortunately we are still letting such bugs go through. I really think that the compiler should do more to help out users flag such problems. I worked on an initiative such as in CL https://go-review.googlesource.com/c/go/+/34562/ which @randall77 alternated and nailed in https://go-review.googlesource.com/c/go/+/45010 to reject unknown pragmas in the standard library. I expected either a static analyzer report something such as

/home/emmanuel/Desktop/openSrc/bugs/golang/embed/main.go:5:1: pragmas should not have leading whitespace
Got:   "// go:embed hello.txt"
Want: "//go:embed hello.txt"

We should try as much to make our users very successful

What did you see instead?

$ go run main.go

To solve this problem I wrote a static analyzer internally that I'd be delighted to open source or even write a pass for, for x/go/analysis or to contribute a compiler patch.
@gopherbot gopherbot added this to the Proposal milestone Jan 28, 2022
@seankhliao
Copy link
Member

dup of #43698 ?

@gopherbot
Copy link

Change https://golang.org/cl/381914 mentions this issue: x/tools/go/analysis/passes: add pragmyzer to flag invalid pragmas

@odeke-em
Copy link
Member Author

I've mailed out a new static analyzer "pragmyzer" in CL https://go-review.googlesource.com/c/tools/+/381914 which fixes the problem and is simple.

Thank you for pointing that out @seankhliao, indeed in a limited case that that issue only requests for cmd/vet! However, I still think perhaps we should consider having the compiler actually fail to compile hence I shall keep this one open for proposal review.

@mvdan
Copy link
Member

mvdan commented Jan 29, 2022

My take is that a vet check is enough, as long as it runs as part of go test. Historically that combination has worked well, as developers run their tests regularly, and especially for error conditions that don't really fit a compiler's role.

@seankhliao seankhliao changed the title proposal: cmd/compile, cmd/go, x/go/analysis: Go pragmas with a space silently compile and cause unexpected problems at runtime e.g "// go:embed file.txt" should fail and report need for "//g:embed file.txt" proposal: cmd/compile: error out on `// go:..." Jan 29, 2022
@seankhliao seankhliao changed the title proposal: cmd/compile: error out on `// go:..." proposal: cmd/compile: error out on // go:... Jan 29, 2022
@zpavlinovic
Copy link
Contributor

That being said, perhaps the most straightforward way is indeed to take the logic from "pragmyzer" and push it into buildtag analyzer?

@ianlancetaylor ianlancetaylor changed the title proposal: cmd/compile: error out on // go:... proposal: cmd/vet: warn about on // go:... Feb 16, 2022
@ianlancetaylor
Copy link
Contributor

I changed this to be a cmd/vet proposal.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Feb 16, 2022
@rsc rsc changed the title proposal: cmd/vet: warn about on // go:... proposal: cmd/vet: warn about // go:... Feb 16, 2022
@rsc
Copy link
Contributor

rsc commented Feb 16, 2022

There is already a buildtag vet check. That should be extended if needed.
It's a little tricky, because what if that's in a doc comment?
But we should probably do it.
The check should probably look for "// go:".
The buildtag vet check is still fine though; we do not need a separate analyzer.

@rsc rsc moved this from Incoming to Active in Proposals (old) Feb 16, 2022
@rsc
Copy link
Contributor

rsc commented Feb 16, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@seankhliao
Copy link
Member

This was already accepted for cmd/vet in #43698

@mvdan
Copy link
Member

mvdan commented Feb 17, 2022

Also worth noting that I already suggested using a compiler/build error in #43698 (comment), and it seems like the idea was already discarded in favor of vet. So I agree with @seankhliao that this seems like a dupe; it's far too easy to forget that a proposal has already been filed, even when you've participated in the previous one! :)

@rsc
Copy link
Contributor

rsc commented Feb 23, 2022

Thanks for digging that up!

@rsc rsc moved this from Active to Declined in Proposals (old) Feb 23, 2022
@rsc
Copy link
Contributor

rsc commented Feb 23, 2022

This proposal is a duplicate of a previously discussed proposal, as noted above,
and there is no significant new information to justify reopening the discussion.
The issue has therefore been declined as a duplicate.
— rsc for the proposal review group

@rsc rsc closed this as completed Feb 23, 2022
@golang golang locked and limited conversation to collaborators Feb 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal
Projects
No open projects
Proposals (old)
Declined
Milestone
Proposal
Development

No branches or pull requests
7 participants
@rsc @zpavlinovic @ianlancetaylor @mvdan @odeke-em @gopherbot @seankhliao