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

Jump to bottom

x/tools/go/packages: LoadSyntax and LoadSyntaxAll are now the same #31148

Closed
seebs opened this issue Mar 29, 2019 · 5 comments
Closed

x/tools/go/packages: LoadSyntax and LoadSyntaxAll are now the same #31148

seebs opened this issue Mar 29, 2019 · 5 comments
Labels
FrozenDueToAge
Milestone
Unreleased

Comments

@seebs
Copy link
Contributor

seebs commented Mar 29, 2019

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

N/A; this applies to commit dbeab5a, I think.

Does this issue reproduce with the latest release?

Not exactly.

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

N/A.

What did you do?

Ran go get -u on something with a go.mod file referring to an older version of x/tools.

What did you expect to see?

Probably the build succeeding anyway.

What did you see instead?

../../../../pkg/mod/github.com/golangci/golangci-lint@v1.15.0/pkg/lint/load.go:171:2: duplicate case "golang.org/x/tools/go/packages".LoadAllSyntax (value 991) in switch
        previous case at /home/seebs/src/go/pkg/mod/golang.org/x/tools@v0.0.0-20190329151228-23e29df326fe/go/packages/packages.go:98

So the issue is, something elsewhere has a switch to show which of the Load constants something is, and this switch breaks because the patch made LoadSyntaxAll and LoadSyntax be the same value. This... may actually be fine. I don't know whether it's intentional or not, I don't entirely think that the external package (golangci-lint) should be that aware of the internals, but I'm also of the opinion that this feels sort of like a breaking change that probably shouldn't be a breaking change -- if nothing else, it seems like there should be a distinction between these, since there used to be a documented distinction.

This is easily avoided by not running go get -u, but I figured I should note that the change appears to have broken at least one thing somewhere.
@gopherbot gopherbot added this to the Unreleased milestone Mar 29, 2019
@torbjornvatn
Copy link

I ran into this today as well, makes my CI/CD pipeline fail.

My issue is also related to golangci-lint

@matloob
Copy link
Contributor

matloob commented Apr 2, 2019

This is now fixed in golangci-lint: golangci/golangci-lint@de1d1ad.

I'm going to close this issue for now, but if another case like this pops up, we can think about putting in a "dummy" bit that does nothing to distinguish LoadSyntax and LoadAllSyntax

@matloob matloob closed this as completed Apr 2, 2019
@matloob
Copy link
Contributor

matloob commented Apr 2, 2019

(Please reopen if that doesn't seem like a good solution)

@mec07 mec07 mentioned this issue May 23, 2019
Open
@thepudds
Copy link
Contributor

@matloob as far as I can tell, this is still causing problems roughly 2 months later.

go get -u github.com/golangci/golangci-lint/cmd/golangci-lint
...
# github.com/golangci/golangci-lint/pkg/lint
...\go\pkg\mod\github.com\golangci\golangci-lint@v1.16.0\pkg\lint\load.go:171:2: duplicate case "golang.org/x/tools/go/packages".LoadAllSyntax (value 991) in switch
        previous case at ...\go\pkg\mod\golang.org\x\tools@v0.0.0-20190525145741-7be61e1b0e51\go\packages\packages.go:98

You had written in golangci/golangci-lint@de1d1ad#commitcomment-33008947:

I made the change that broke this (sorry!)

LoadAllSyntax and LoadSyntax are now the equal in x/tools/go/packages, and the Go compiler will object to two identical switch cases, so that change breaks the compilation of this code.

It sounded like you had an idea for fixing this within x/tools/go/packages:

we can think about putting in a "dummy" bit that does nothing to distinguish LoadSyntax and LoadAllSyntax

Could that change happen?

If not, could this bug be re-opened, at least until there is a golangci-lint release that solves this, and could you ping the golangci-lint people to encourage them to issue a release that solves this?

Also, I have only briefly tried to tease apart the history here; apologies if I am not fully following the history.

CC @dmitshur

@dmitshur
Copy link
Contributor

dmitshur commented May 27, 2019 •
edited

as far as I can tell, this is still causing problems roughly 2 months later.

Did the commit golangci/golangci-lint@de1d1ad not fix the issue, or is the problem that there was no new release tag made that includes that commit?

Checked myself (golangci/golangci-lint@v1.16.0...master), the problem is that there's no new release tag.

This seems like an issue that needs to be solved in the golangci-lint project, and not x/tools. Making changes to x/tools won't help if the golangci-lint doesn't update to a newer version and issue a new release. Please let me know if you think there is something else that can be done here.

gdbelvin added a commit to google/trillian that referenced this issue May 29, 2019
@gdbelvin
GO111MODULE=on go get github.com/golangci/golangci-lint/cmd/golangci-…
8757bd1 
…lint@HEAD

@Head is required to include a fix related to
golang/go#31148
@frrist frrist mentioned this issue Jul 25, 2019
Merged
ddworken added a commit to keybase/bot-sshca that referenced this issue Aug 23, 2019
@ddworken
Update golangci-lint version to fix golang/go#31148
a6d0b58
@golang golang locked and limited conversation to collaborators May 26, 2020
@rsc rsc unassigned matloob Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
6 participants
@torbjornvatn @seebs @dmitshur @gopherbot @matloob @thepudds