Go: x/playground: do the "vet" check along with running a program

[ysmolsky]

"vet" is implemented as a separate button. Consider if we can remove that button and do the "vet" check whenever a user pushes the "run" button.

Comments from CL 100776:

Bryan Mills
I wonder whether this should even be a separate button. Could we Vet unconditionally whenever we Run?

We already have a bit of a visual distinction between “errors” and “output”, so it probably wouldn't be terribly confusing to put the errors before the output if we have both, and that would make the warnings that much more discoverable.

Nathan Youngman
I think that's a good point.

Also, with the Go Playground supporting tests, and tests in Go 1.10 now running a subset of vet, it seems like this could just be automatic whether running tests or code. Though it may be important to clearly indicate what is an error from the compiler vs. an error from vet for people new to Go.

https://go-review.googlesource.com/c/playground/+/98155

Andrew Bonventre
This would have ramifications for every call to /compile, which is an endpoint that anyone can use right now.

Yury Smolsky
That would mean changing the spec of /compile endpoint. That endpoint is not supposed to return errors along with output. All clients assume that if there is an error, then output should not be considered. So this could break clients. IMHO, this feature requires new version of /compile along with new protocol.

Nathan Youngman
There is more than one way to do it. An alternative is for the front-end to call /compile and /vet from the same button click.

https://go-review.googlesource.com/c/playground/+/100776/5/edit.html#115

[opennota]

No way. Refusing to run perfectly correct programs isn't what one expects of a playground, for Zeus's sake.

fmt.Println("Is it?\n") // vet: redundant newline

[ysmolsky]

It is not supposed that "vet" errors will prevent from executing the program. Errors from vet must be displayed along with output from the program. However how to present it clearly, I don't know.

[rsc]

Back end protocol aside (that can be worked out either way), it does seem nice to just show the vet output alongside the normal output, instead of having yet another button. Unlike go test, if vet fails, the program should still run. (This is the playground.)

[nathany]

Now that the playground supports go test (#6511) is there any interplay to consider between go test running vet and running vet standalone? Or would the vet part of go test just be disabled/ignored in the playground in favour of the same vet as non-test code?

Regarding back end, what makes most sense to me at the moment is separate vet and compile endpoints that the UI can use from the same button click. What happens when the compile fails with errors though? Does vet only run after the compile succeeds and the output (begins) to display? (Keeping in mind time.Sleep works in the playground).

[ysmolsky]

To clarify go test function of Playground. Right now it does not run vet while testing. So we assume that we have to run go vet for the code containing tests too. And errors returned from go vet should not break an execution of a program even for tests. It differs from the behaviour of a real 'go test', but this will provide some consistency for the playground users.

EDIT: removed spammy examples from here.

[ysmolsky]

I have made the output of stderr to look like an error and put vet errors before the output. It is not indicated that first error is from vet. Should we?

Running an example without output compiles it but does not executes. Produces this error:

[gopherbot]

Change https://golang.org/cl/107455 mentions this issue: godoc: display "go vet" errors before the output of a program

[gopherbot]

Change https://golang.org/cl/100776 mentions this issue: playground: add vet

[bcmills]

All of these formats seem to rely on differences in color, which might not work so well for someone with red-green colorblindness, although I suppose that's also an existing issue.

To distinguish vet output from user code, what about putting the vet output at the end of the affected line instead of after the program?

That might look like:

That way the warning isn't buried at the end of the output, but also doesn't bury the output. On the other hand, line-wrapping makes things a bit dicey for small windows, long code lines, and/or long error lines.

[jimmyfrasche]

Red/black can look a lot like black/black or black/nearly-black for various forms of color blindness and mostly for red-green color blindness which is the most prevalent form.

Another way to distinguish the vet output is to leave it where it is but draw a box around it labeled something like "vet output".

The red issue also applies to stdout vs stderr. Maybe adding unhighlightable {out, err} labels where line numbers would go in addition to the red? They could be made smallish since they'd essentially be icons. The lack of either on vet warnings would probably be enough to make it clear where things start and stop along with a little more spacing between the vet output and the program output.

[ysmolsky]

More screenshots for my proposed solution/design above: https://imgur.com/a/Jn3qx.

Thanks for the feedback. I will meditate on it.

Feel free to add more. For some reason I am not satisfied with my proposed design anymore. It has apparent flaws. 7 years of pure programming have put too much rust on my design/layout skills.

[ysmolsky]

I don't want to add graphics for this feature. I really like simple or minimal design as it is right now. Also I want vet errors to be on the top, that they urge a user to fix the code and get rid of them. They must be annoying in some sense.

Is current design friendly to color blind people?

I propose this simple layout:

If go vet returned no errors, then first 3 lines are not displayed.

[jimmyfrasche]

The explicit delimiter works. It's not friendly, per se—but it's not antagonistic.

[ysmolsky]

One more observation:

package main

func main() {
    qwe
}

produces

prog.go:4:2: undefined: qwe

Program exited.

But in fact the program was not running at all. Error came from the go build. So the output could be:

prog.go:4:2: undefined: qwe

Go build exited.

[jimmyfrasche]

"Go build failed." might be clearer. It always exits.

[bcmills]

We should ensure that vet results show up in the Tour, too. That might save some folks a lot of time while they're learning.

[ysmolsky]

I agree. Tour is a different thing and does things differently. It uses golang.org/compile. Since we add new endpoint for vetting, we do not break Tour by any means. So that feature is still possible and should be tackled as a separate change.

I have updated https://go-review.googlesource.com/c/tools/+/107455 with the proposal I made above. I think this is the best design retaining simplicity I can think of. Examples are here.

[ysmolsky]

@andybons, please take a look at https://go-review.googlesource.com/c/tools/+/107455.