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

x/playground: not showing vet warnings #58560

Closed
Deleplace opened this issue Feb 16, 2023 · 10 comments
Closed

x/playground: not showing vet warnings #58560

Deleplace opened this issue Feb 16, 2023 · 10 comments
Assignees
@findleyr
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@Deleplace
Copy link
Contributor

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

go1.20.1

(in the playground actually, not my computer)

Does this issue reproduce with the latest release?

Yes

What did you do?

In the Go playground:

  • executed program 1 which has a loop variable capture problem
  • executed program 2 which passes a Mutex by value

What did you expect to see?

Program 1:

./prog.go:11:16: loop variable i captured by func literal
5
5
5
5
5

Program 2:

./prog.go:8:16: call of work copies lock value: sync.Mutex
./prog.go:11:23: work passes lock by value: sync.Mutex
Done

What did you see instead?

Program 1:

5
5
5
5
5

Program 2:

Done

This is not expected because the playground's About explicitly says "The service receives a Go program, vets, compiles, links, and runs the program inside a sandbox, then returns the output". If the service does vet, then I'm expecting to see the vet warnings.
@gopherbot gopherbot added this to the Unreleased milestone Feb 16, 2023
@bcmills
Copy link
Contributor

bcmills commented Feb 16, 2023

(attn @toothrot @findleyr)

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 16, 2023
@findleyr
Copy link
Contributor

Thanks for the report. This appears to be related to the new frontend, since the warning does appear on the old frontend:
https://gotipplay.golang.org/p/g3iO751-3NW

@Deleplace
Copy link
Contributor Author

The vet error message is not reaching the browser anymore, as the JSON key "VetErrors" is missing from the "compile" HTTP response:

image

The old JSON on the left is the response of a POST request to https://gotipplay.golang.org/compile

The new JSON on the right is the response of a POST request to https://go.dev/_/compile?backend=

@Deleplace
Copy link
Contributor Author

In both cases (old and new), the request to the endpoint compile has in its payload withVet: true. This seems to point at the server behaving differently.

@findleyr
Copy link
Contributor

Thanks @Deleplace, due to your investigation, I found the bug:
https://cs.opensource.google/go/x/website/+/master:internal/play/proxy.go;l=66;drc=4a88b4c11220875d54c754de17547b467e875186

The compile proxy is not forwarding the withVet field. Should be an easy fix.

@findleyr findleyr self-assigned this Mar 22, 2023
@findleyr
Copy link
Contributor

It also looks like there were styles in the stylesheet that were not being applied, due to selectors changing in the new playground. I fixed that as well (assuming that was unintentional):

image

@gopherbot
Copy link

Change https://go.dev/cl/478576 mentions this issue: internal/play: fix support for vet errors in playground output

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Mar 29, 2023
@gopherbot
Copy link

Change https://go.dev/cl/480236 mentions this issue: cmd/golangorg: update testdata/live.txt to expect the VetErrors field

gopherbot pushed a commit to golang/website that referenced this issue Mar 29, 2023
@findleyr
cmd/golangorg: update testdata/live.txt to expect the VetErrors field
2bd866e 
CL 478576 restored support for go vet errors to go.dev/play. However, I
didn't realize that cmd/golangorg/testdata contained tests that aren't
run by trybots. Since these tests were not updated, the deployment
failed.

Update live.txt to expect the VetErrors field. Tested locally by
uncommenting the skip in server_test.go.

For golang/go#58560

Change-Id: I2757ef569907a730f9cccccd0f15e7a7b1bb9975
Reviewed-on: https://go-review.googlesource.com/c/website/+/480236
Reviewed-by: Jamal Carvalho <jamal@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@Deleplace
Copy link
Contributor Author

\o/ fantastic, thank you Robert

@findleyr
Copy link
Contributor

Thank you @Deleplace for reporting this, helping to root cause, and for providing feedback on the colors!

@golang golang locked and limited conversation to collaborators Mar 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@findleyr findleyr

Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@dmitshur @bcmills @gopherbot @Deleplace @findleyr