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/gopls: doesn't report errors for disallowed uses of internal packages #35937

Closed
stamblerre opened this issue Dec 2, 2019 · 7 comments
Closed

Comments

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Dec 2, 2019

Forked from microsoft/vscode-go#2926.

Repro:
Add the following to golang.org/x/tools/blog/blog.go:

import "golang.org/x/tools/go/internal/gcimporter"

func _() {
	gcimporter.BExportData(nil, nil)
}

This code will not compile, but we don't report any errors for it.

@brainflake
Copy link

@brainflake brainflake commented Feb 7, 2020

Hi @stamblerre - has anyone started this? Thought I could give it a whirl.

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Feb 7, 2020

No I don't think anyone has! Please go ahead.

The code you'll want to look at is here. We'll probably need to detect if the package is an allowed internal package and then add an error. Here's the formal documentation for internal packages: https://golang.org/cmd/go/#hdr-Internal_Directories.

@brainflake
Copy link

@brainflake brainflake commented Feb 10, 2020

Sound good! Thanks for the pointers.

One question - is it safe to rely on the pkgPath from the metadata passed in to typeCheck to compare against the imported pkgPath? I've been seeing it set to command-line-arguments during my testing, although it may be just that my VSCode has a misconfiguration.

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Feb 10, 2020

command-line-arguments can mean that your package is both outside of GOPATH and outside of a module, so if that's your setup then you may see that. But, yes, in general it is safe to compare these package paths.

@brainflake
Copy link

@brainflake brainflake commented Feb 10, 2020

Great - I created a PR here.

While looking through the code, I found an alternative way to perform the check (using the analysis API) which seems to have the benefit of working with the gopls CLI as well as the server. I'm unsure of the performance comparison with using the lsp cache, but just thought I would mention it in case it had merit.

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Feb 13, 2020

Anything in the gopls codebase will work with both the CLI and the server, so your change is the correct solution - thank you!

The analysis API is typically for analyses on top of type-checked code, but in the case of a package with an invalid import, it really shouldn't even type check. We could potentially add a diagnostic instead of failing to type-check, but that would have the misleading effect of showing users completions that won't compile.

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Feb 19, 2020

Fixed via CL 218977.

@stamblerre stamblerre closed this Feb 19, 2020
@golang golang locked and limited conversation to collaborators Feb 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants