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

cmd/vet: reject flag.Parse during func init #33190

Open
rsc opened this issue Jul 19, 2019 · 4 comments
Open

cmd/vet: reject flag.Parse during func init #33190

rsc opened this issue Jul 19, 2019 · 4 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) early-in-cycle A change that should be done early in the 3 month dev cycle. help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@rsc
Copy link
Contributor

rsc commented Jul 19, 2019

In #31859, @cespare suggested rejecting flag.Parse during func init, which is always incorrect (other packages not yet initialized may want to define flags).

We could add a special runtime hook of some kind to allow flag to see whether main.main has started, but that would be unfortunate.
There also may be lots of code in the wild that does parse flags during init and kind of works out OK, and if it's working well enough we don't want to break it unnecessarily.

A vet check, on by default during go test, seems like the perfect compromise to me.
@rsc rsc added this to the Go1.14 milestone Jul 19, 2019
@rsc rsc added early-in-cycle A change that should be done early in the 3 month dev cycle. NeedsFix The path to resolution is known, but the work has not been done. labels Jul 19, 2019
@rsc rsc mentioned this issue Jul 19, 2019
Closed
@gbbr
Copy link
Member

gbbr commented Sep 21, 2019 •
edited

A vet check would be very helpful, if it’s accomplishable, as the current output is confusing and misleading. Example:

$ go test ./pkg/trace/api
flag provided but not defined: -test.testlogfile
Usage of /var/folders/02/23s7r95n6t1739kdh0zc3trr0000gn/T/go-build397748628/b001/api.test:
  ...

I'm honestly a bit shocked that such a change went through. This (IMHO) is worse than the original problem being fixed.

@gbbr gbbr mentioned this issue Sep 23, 2019
Merged
gbbr added a commit to DataDog/datadog-agent that referenced this issue Sep 23, 2019
@gbbr
pkg/trace/flags: move flag.Parse to main.main
6e6261f 
Due to a recent change in go1.13 (golang/go#21051), parsing of
flags inside `init` functions causes tests to fail. This change moves
`flag.Parse` inside `main.main`.

For more information see golang/go#33190
gbbr added a commit to DataDog/datadog-agent that referenced this issue Sep 23, 2019
@gbbr
pkg/trace/flags: move flag.Parse to main.main (#4190)
1f53c50 
Due to a recent change in go1.13 (golang/go#21051), parsing of
flags inside `init` functions causes tests to fail. This change moves
`flag.Parse` inside `main.main`.

For more information see golang/go#33190
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
jybp added a commit to jybp/twitch-downloader that referenced this issue Dec 18, 2019
@jybp
Fix parse flags in tests golang/go#33190
2f9186f
@jespino
Copy link

jespino commented Feb 1, 2020

I can work on this

@jespino jespino mentioned this issue Feb 9, 2020
Open
@gopherbot
Copy link

Change https://golang.org/cl/218757 mentions this issue: go/analysis/passes/initflagparse: add check for avoid flag.Parse at init

@seankhliao seankhliao mentioned this issue May 15, 2020
Closed
@ALTree ALTree mentioned this issue Sep 21, 2020
Closed
@Deleplace
Copy link
Contributor

Reviving this issue: the suggested fix would be very useful IMO (I was going to open an identical issue)

Additionally, the documentation of flag.Parse itself could warn more explicitly about the init pitfall:

Parse parses the command-line flags from os.Args[1:]. Must be called after all flags are defined and before flags are accessed by the program.

When following this guidance, calling flag.Parse() at the end of init looks like a right thing to do, but it's not.

@adonovan adonovan added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Apr 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) early-in-cycle A change that should be done early in the 3 month dev cycle. help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
7 participants
@rsc @jespino @ianlancetaylor @adonovan @gbbr @gopherbot @Deleplace