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: preserve information about original errors #30379

Open
kujenga opened this issue Feb 24, 2019 · 4 comments
Open

x/tools/go/packages: preserve information about original errors #30379

kujenga opened this issue Feb 24, 2019 · 4 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@kujenga
Copy link

kujenga commented Feb 24, 2019

I was looking at migrating a tool off of the golang.org/x/tools/go/loader package, as it is seemingly deprecated for use with Go modules, switching it onto https://godoc.org/golang.org/x/tools/go/packages. I ran into a disparity between the two packages in the type of errors that are available to the user after loading in the packages of interest.

A loader.PackageInfo provides a struct field Errors []error, which allows one to do type introspection on the actual error returned from within the loading process.

In contrast, the packages.Package has an Errors []Error struct field, where all the errors from the underlying loading process have been one-way transformed into a new struct that loses information, or at least makes it harder to get at. That conversion process happens here: https://github.com/golang/tools/blob/83362c3779f5f48611068d488a03ea7bbaddc81e/go/packages/packages.go#L613-L660

The specific use case I have is to introspect certain types of types.Error values so that the tool can then go and attempt to fix those errors in an automated fashion. Since the types.Error gets stringified, that information is no longer readily available.

Proposal

My proposal is to augment the packages.Error type to include an Err error field that a user of the package can introspect further if desired.

// An Error describes a problem with a package's metadata, syntax, or types.
type Error struct {
	Pos  string // "file:line:col" or "file:line" or "" or "-"
	Msg  string
	Kind ErrorKind
    Err error
}

Alternative workarounds

One can write parsing logic for the current Error.Pos field based on what is currently done in the source code, but this seems unnecessary given that the underlying information was already present in a more structured form.

@gopherbot gopherbot added this to the Unreleased milestone Feb 24, 2019
@josharian
Copy link
Contributor

@matloob @alandonovan

@ianthehat
Copy link

The transformation is deliberately in place to allow for serialization between the host application and the build system driver when needed.
The serialization format is JSON, and serializing interface fields (like error) does not work.
If you need some specific information, please let us know and we will look at how to add it to the Error struct, but adding the underlying error directly is not going to be viable and I would encourage you to also not try to parse/detect the message text.
If you are looking into detecting errors and trying to automate fixes, you probably don't want to be using the errors out of go/packages directly for that anyway, you probably want to be writing an analysis.Analyzer, matloob is currently looking at adding more support for producing findings with "fixes" attached.

@bcmills
Copy link
Contributor

bcmills commented Feb 25, 2019

The serialization format is JSON, and serializing interface fields (like error) does not work.

One alternative would be to try to encode the error using encoding/gob and embed the resulting blob in the JSON output. encoding/gob requires interface implementations to be passed to Register prior to encoding, but we could at least pre-register a common set of expected error types.

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 25, 2019
@kujenga
Copy link
Author

kujenga commented Feb 25, 2019

@ianthehat Got it, thank you for the response! I was not aware of the serialization use case, and was also had not taken a look at the analysis package, thank you for pointing that out.

The serialization format is JSON, and serializing interface fields (like error) does not work.

I realize that you pointed out higher-level concerns as well, but in addition to the possible use of gob pointed out above, I think one possible alternative here be to do something like the following, and avoid serializing the error field:

Err error `json:"-"`

If you need some specific information, please let us know and we will look at how to add it to the Error struct

The specific information that my code is using currently is the numeric token.Pos value from the types.Error struct, as well as the *token.FileSet (though this is available from the packages.Package struct), which I make use of when walking the AST for the purposes of re-writing it to resolve various errors, for example propagating context.Context through multiple layers of APIs that need it because something within it now requires a context.

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

5 participants