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?

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.

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?

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.

In that case, adding it as a field SGTM.

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

@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…

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?

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?).

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.

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.)

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