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

x/tools/go/analysis/singlechecker: don't use os.Exit in Main #30219

Closed
mvdan opened this issue Feb 13, 2019 · 10 comments

Comments

@mvdan
Copy link
Member

commented Feb 13, 2019

Right now, the func is used like:

func main() { singlechecker.Main(foo.Analyzer) }

I propose to instead have it return the exit code integer, changing the usage to:

func main() { os.Exit(singlechecker.Main(foo.Analyzer)) }

The reasoning is that calling os.Exit directly in libraries should generally be avoided, even in those APIs which are meant to be used in func main directly. For example, see https://golang.org/pkg/testing/#M.Run and https://godoc.org/golang.org/x/tools/go/packages#PrintErrors.

In particular, using os.Exit means that it's impossible for the developer to run any code after the checker has run. The only option would be to stick the code at the very end of the checker's Run function, but that's not the same. And it would likely pollute the non-main package with code from the main package.

My use case is that I want to write a coverage profile to disk after executing main function. This happens for many main packages, so the tooling does not care about go/analysis or singlechecker. So the tool requires that users write their main function as follows, to be able to call main1 and do the extra work afterward:

func main() { os.Exit(main1()) }

func main1() int {
     // actual implementation
}

I think changing the signature is reasonable, and now is the right time to make the change. There are very few users of go/analysis outside of x/tools, and the API is still experimental.

My biggest worry is that all main packages doing func main() { singlechecker.Main(foo.Analyzer) } would suddenly stop properly exiting with non-zero status codes when an error happens. However, we can communicate this API change properly via the golang-tools group and Hangouts calls, and like I said above, I presume almost noone has shipped code using singlechecker yet.

/cc @alandonovan @ianthehat @matloob @rogpeppe

@mvdan mvdan added the NeedsDecision label Feb 13, 2019
@gopherbot gopherbot added this to the Unreleased milestone Feb 13, 2019
@alandonovan

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

I'm reluctant to make this change. The whole point of the {single,multi}checker.Main functions is to do the entire job of the command-line tool. If part of that job is to support coverage, then unitchecker should write the coverage report. Why should some command-line tools get that feature and not others?

It would also be a pernicious API change: it breaks people's programs without breaking the build.

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

If part of that job is to support coverage, then unitchecker should write the coverage report. Why should some command-line tools get that feature and not others?

Perhaps I gave the wrong impression in my original post. I don't intend to use this to add extra features or work to my built tool. The use case is @rogpeppe's https://godoc.org/github.com/rogpeppe/go-internal/testscript, which is a port of cmd/go/script_test.go to be portable to other tools.

To be able to add one's main function as a command in the script, a func() int is supplied. The tests work if the function exits directly, but then extra testscript features like test coverage don't work. In this scenario, adding a coverage flag to singlechecker or unitchecker wouldn't be the same, as then testscript would have to add special treatment of analyzers.

And of course, I could be filing the same issue over and over again for other testing features that I'll probably need down the line, like cpu and memory profiles. I just need to be able to run my analyzer within a test, without it insisting on exiting the entire program.

You could argue that testscript requiring main-like funcs that return exit codes is asking too much, but in practice, practically all libraries out there can be tuned to not exit directly, such as flag.ContinueOnError or testing itself, which I linked to earlier. singlechecker is the only outlier I've found so far.

It would also be a pernicious API change: it breaks people's programs without breaking the build.

Yes, agreed. This is why I was hoping that very few developers would have started using the API already, meaning it would be easier to coordinate a breaking change.

Another option would be to add parallel functions like RunMain, which would be like Main but returning an integer. Then you'd have something like func Main(t T) { os.Exit(RunMain(t)) } to not break existing programs.

@rogpeppe

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

FWIW I have a convention of using Main1 for main functions that return an exit code rather than exiting. shrug

In general I think that any Go package entry point should avoid calling os.Exit, even if it's intended to be a top level entry. There are always going to be use cases that one might not have thought about, and calling os.Exit shuts them all down hard.

The caller is free to run code before calling Main, so why not afterwards too?

@adonovan

This comment has been minimized.

Copy link

commented Feb 14, 2019

The os.Exit at the end of singlechecker/multichecker is far from the only point at which it might terminate the process. These packages were designed for one job alone, and that is to make it trivial to implement a standalone tool that wraps one or many analyzers. As such, these packages are entitled to all the usual rights of a main package.

If you need an importable library with no global effects (on os.Args, flags.CommandLine, os.Exit, signals, whatever) much more invasive changes are required. We could certainly make them, but it is not as simple is not calling os.Exit, and if we're going to make them, they should be exposed using a different API than a main function.

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

To clarify, we don't need the API to have zero side effects. These testscript main-like commands are run in their own separate process, so it's fine to use globals and such. But exiting is different, as it makes certain features like coverage and cpu profiles impossible.

In other words, it's fine if singlechecker has the usual rights of a main package. Like Roger said, we just need to be able to run code after Main has finished.

Are there any other places besides singlechecker and unitchecker where os.Exit might be called? As far as I can see, there's only a handful of pretty simple direct calls to os.Exit. I presume there will be others like flag's default to exit on errors, but that can be changed with a few lines of code as well.

@adonovan

This comment has been minimized.

Copy link

commented Feb 14, 2019

Every log.Fatal is an os.Exit, remember. I suppose in your scenario, for coverage, you only care about exits after the main work is done, not those that occur during setup.

This is not the first time we've encountered the problem of trying to measure coverage of a standalone (non-test) executable, and it seems we're fixing the symptom but not the cause. What if there were a "cover" package in the standard library that exposed a function to flush out the coverage report, if applicable? Applications could call it prior to their final exit. It's still imperfect because you have to remember to call flush before each exit, but at least the API would then resemble the existing ones for cpu/memory/trace.

@rogpeppe

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

What if there were a "cover" package in the standard library that exposed a function to flush out the coverage report,

That would be great! I had to go to some rather unsavoury lengths to do that in the testscript package and I'd love to be able to remove those hacks.

The companion to that would be for a running test binary to be able to request that additional coverage information be added. I did succeed in doing that but I definitely invoked Hyrum as I did so.

I've been meaning to raise an issue, but I didn't think it would be accepted so haven't done so yet. This conversation is a good reference point for it.

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Mar 21, 2019

Happy to close this issue if the "cover" package and API gets its own issue :)

@matloob

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

Enjoy: #31007

It's pretty bare, but I'm sure we can edit the issue into shape

@mvdan

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2019

Thanks @matloob!

@mvdan mvdan closed this Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.