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/go/packages: handle import cycles in tests #38826

Open
dominikh opened this issue May 3, 2020 · 12 comments
Open

x/tools/go/packages: handle import cycles in tests #38826

dominikh opened this issue May 3, 2020 · 12 comments

Comments

@dominikh
Copy link
Member

@dominikh dominikh commented May 3, 2020

In its current implementation, the go list driver in go/packages looks for the error message import cycle not allowed and if found appends the error's ImportStack to the error message, resulting in import cycle not allowed: import stack: [sandbox/foo sandbox/bar sandbox/foo]. This misses import cycles in tests, which instead use the error message import cycle not allowed in test. However, simply checking for this error and appending the import stack is not a viable solution. The message produced would look like this: import cycle not allowed in test: import stack: [honnef.co/go/tools/unused (test) honnef.co/go/tools/lint honnef.co/go/tools/runner honnef.co/go/tools/unused] – this is problematic because gopls extracts the import stack from the string via some basic string matching and splitting on whitespace. It would ultimately believe that (test) is the package we're trying to import.

Unfortunately, go/packages returns a type Error struct, and not an interface, so the obvious solution of returning a structured type is not applicable. We could wrap each import path in quotes, but then gopls has a harder time extracting the paths. However, I don't see any other way.

/cc @matloob @stamblerre

@dominikh
Copy link
Member Author

@dominikh dominikh commented May 3, 2020

Actually, is there any reason other than convenience for why we currently use msg += fmt.Sprintf(": import stack: %v", p.Error.ImportStack)? could we instead switch to nicely formatted multi-line errors, akin to what go build prints?

@matloob
Copy link
Contributor

@matloob matloob commented May 4, 2020

I'm going to wait for @stamblerre's take on this, and I'm okay with a temporary workaround to surface the import stack... but:

I'm wondering whether we should add import stack to the errors as an optionally present field if the lsp needs it? I think if we're handling import stacks for multiple errors, we should probably handle them consistently for all errors that have them.

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented May 5, 2020

I'm wondering whether we should add import stack to the errors as an optionally present field if the lsp needs it? I think if we're handling import stacks for multiple errors, we should probably handle them consistently for all errors that have them.

The way things are done right now in gopls is really brittle, so anything to avoid parsing error messages makes me happy :) Which other types of errors have import stacks?

@stamblerre stamblerre modified the milestones: Unreleased, gopls/v0.5.0 May 5, 2020
@matloob
Copy link
Contributor

@matloob matloob commented May 5, 2020

Hm, a bunch of other errors have import stacks, pretty much anything that happens when importing other packages, for example, if there's an import of a package/directory with no go files, or a package contains an invalid import path.

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented May 5, 2020

In that case, adding it as a field SGTM.

@matloob
Copy link
Contributor

@matloob matloob commented May 5, 2020

@dominikh @ianthehat any objections to adding import path as a field?
@dominikh do you think that would fix this issue?

@dominikh
Copy link
Member Author

@dominikh dominikh commented May 6, 2020

@dominikh @ianthehat any objections to adding import path as a field?

import stack, I presume.

My only issue is that in all cases but import cycles, the import stack is redundant information: it is already encoded in the graph. In fact, the information in the graph is more accurate: if both foo and bar depend on a non-existent baz, then baz has a single ImportStack choosing one of the possible paths; a graph traversal can tell us all paths.

Import cycles are special, because we actually break the cycle in the graph, so we depend on the import stack that go list tells us.

But I also vastly prefer actual data structures over string parsing…

@matloob
Copy link
Contributor

@matloob matloob commented May 6, 2020

Yes, import stack. Sorry about that.

Yeah I see that the import stack could be redundant information, on the other hand, I'm not sure if go/packages should be in the business of deciding either whether to propagate the import stack based on the type of error, or whether to append the import stack as text based on the type of error. Ideally go/packages shouldn't be doing any proccesing of errors produced by the driver other than propagating them, or failing the load operation in progress because it encountered a certain error.

@ianthehat, what do you think?

@bcmills
Copy link
Member

@bcmills bcmills commented May 6, 2020

Perhaps go/packages should always prune the import stack? The import stack is important on the go command line because the user often doesn't have the full package graph available, but go/packages is already producing the full package graph (perhaps even with cycles via the Imports field?).

@dominikh
Copy link
Member Author

@dominikh dominikh commented May 7, 2020

go/packages is already producing the full package graph (perhaps even with cycles via the Imports field?).

go/packages breaks cycles and only returns DAGs to the user. For a cycle foo -> bar -> foo, foo may have bar as a dependency, but bar won't depend on foo.

@bcmills
Copy link
Member

@bcmills bcmills commented May 7, 2020

Perhaps we should fix that. (Perhaps we should only guarantee that either the graph is acyclic, or at least one package in each cycle has len(Errors) ≥ 1.)

@matloob
Copy link
Contributor

@matloob matloob commented May 7, 2020

I think that would break go/packages users who are depending on the current behavior? I don't know if that's okay

@stamblerre stamblerre removed this from the gopls/v0.5.0 milestone Jun 24, 2020
@stamblerre stamblerre removed the gopls label Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants