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

cmd/go, x/tools/gopls: clarify error messages about std modules (which have no '.') #65301

Open
meling opened this issue Jan 26, 2024 · 6 comments · May be fixed by golang/tools#471
Open

cmd/go, x/tools/gopls: clarify error messages about std modules (which have no '.') #65301

meling opened this issue Jan 26, 2024 · 6 comments · May be fixed by golang/tools#471
Labels
gopls Issues related to the Go language server, gopls.
Milestone

Comments

@meling
Copy link

meling commented Jan 26, 2024

Go version

go version devel go1.22-33d4a51 darwin/arm64

Output of go env in your module/workspace:

Not relevant. It isn't a bug per se, but a request for better error messages.

What did you do?

go.mod:

module notstd

go 1.22

server/server.go:

package main

import "notstd/proto"

func Insert(req *proto.InsertRequest) (*proto.InsertResponse, error) {
	return &proto.InsertResponse{Success: true}, nil
}

There are no Go files in the proto folder (yet):

├── go.mod
├── proto
│   └── kv.proto
└── server
    └── server.go

What did you see happen?

$ go run ./server/
server/server.go:4:2: package notstd/proto is not in std (/Users/meling/sdk/gotip/src/notstd/proto)
$ gopls check server/server.go
/Users/meling/tmp/notstd/server/server.go:4:2-16: could not import notstd/proto (no required module provides package "notstd/proto")

When opening the quick fix suggestions in VSCode, gopls or the Go extension suggests to go get package notstd/proto, which is nonsensical.

What did you expect to see?

I would expect both the compiler and gopls to report that there are no Go files in the proto folder within the same module, since the import is explicitly trying to import its own package.

With the simple example it might seem trivial to figure out that you've forgotten to compile the proto file to populate the proto package. But in larger projects it may be easy to forget, and students don't necessarily understand what the problem is, if they are expected to compile the proto file themselves. Moreover, the error messages are not helpful in this case.

Edit: Added the kv.proto file to match the actual use case that triggered this and simplified the what to expect description.

@gopherbot gopherbot added the gopls Issues related to the Go language server, gopls. label Jan 26, 2024
@seankhliao
Copy link
Member

I think cmd/go may be working as intended, we've previously stated that all names without a dot in the first component are considered reserved for the standard library.

@findleyr
Copy link
Contributor

When opening the quick fix suggestions in VSCode, gopls or the Go extension suggests to go get package notstd/proto, which is nonsensical.

It is nonsensical to suggest go get for a package without a dot, as they are guaranteed not to be go gettable. But to clarify, it's not entirely nonsensical to go get a package that is prefixed by the module path, since the package could be in a nested module. With that said, that is not going to be the case 99+% of the time, and we could do a better job of diagnosing the actual problem in this case.

@adonovan adonovan changed the title cmd/go, x/tools/gopls: make compile error more helpful cmd/go, x/tools/gopls: clarify error messages about std modules (which have no '.') Feb 1, 2024
@adonovan
Copy link
Member

adonovan commented Feb 1, 2024

Perhaps when the go command fails it could additionally report whether the go.mod file contains "module foo" where foo contains no '.' and is not "std"? @bcmills @matloob

Also, perhaps gopls could have a checker to report module declarations of that form so that one learns the '.' rule as soon as you make the mistake. (Update: @findleyr tells me that there are unfortunately quite a few modules out there that use such names: they work fine as a main module, even though they aren't gettable. Perhaps a hint augmenting the gopls error message would be better.)

@adonovan adonovan added this to the gopls/v0.16.0 milestone Feb 1, 2024
@bcmills
Copy link
Member

bcmills commented Feb 1, 2024

That reminds me, I need to clean up https://go.dev/cl/359594 and get it merged. 😩

@meling
Copy link
Author

meling commented Feb 2, 2024

Let me add a little more context for our use case... I really like that I can do things like the following for our courses, where dat520 is the course code:

module dat520

In fact, the whole point is that the main repository should not be go gettable as it is private and contains more code than we share with students. We copy part of the main repository to another per-year repository, e.g., dat520-2024 (also private) from which students pull into their own private repositories. It is nice that the import paths become short. But more important nobody is confused thinking they would need to go get some repository, which wouldn't be accessible to them, and we don't have to rewrite the imports to match the student's repository path.

I really hope that we can keep using our current approach, and that the "module names without dots are reserved" idea will never be enforced. I would much rather have to deal with a less-than-ideal error message (it is easy to detect now that we know about it), than to loose the ability to use a module name like dat520. We already went through a bad couple of years of transitioning from GOPATH to modules.

@adonovan
Copy link
Member

adonovan commented Feb 2, 2024

Thanks for the context. I agree, this usage seems quite reasonable and in any case we can't disallow previously legal behavior. As I noted in my post script, perhaps the best we can do is add an additional hint when the go command exits with certain errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gopls Issues related to the Go language server, gopls.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants