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

go/types: Importers should create missing Packages #16088

Closed
adonovan opened this issue Jun 16, 2016 · 8 comments
Closed

go/types: Importers should create missing Packages #16088

adonovan opened this issue Jun 16, 2016 · 8 comments

Comments

@adonovan
Copy link
Contributor

@adonovan adonovan commented Jun 16, 2016

We should change the go/types.Importer contract so that implementations are encouraged to return a Package always, even if they also return an error. This would enable the type checker to do a better job when imported packages are missing. For example, it would avoid many secondary "undeclared name" errors.

Consider this code:

package main
import "encoding/json"
var x json.Marshaler

Assume that the importer cannot find "encoding/json" for some reason. If it nonetheless returned a Package (named "json") along with a "no such package" error, the type checker would know that the json identifier on line 3 was a reference to the (missing) package imported on line 2, avoiding an "undefined: json" error.

Another reason to implement this change: While adding a new check to the vet command, I rediscovered how tricky it is to use go/types when imports may be missing. The new vet check needs to identify calls to the function context.WithCancel and it inspects each ast.SelectorExpr in turn. When imports succeed, we can look up the .Sel identifier in the Uses map; when imports fail, this identifier is obviously not in the map, but it's easy enough to see that it is spelled "WithCancel". The situation for the package name is trickier: if the import failed, the "context" identifier is not present in the Uses map either, forcing us to use the more fragile heuristic that its name is spelled "context". This heuristic is reasonable for a single-segment name, but what if the package was called "foo/context" (as is the case for Google's version of Context, for some value of foo)? There is no connection between the identifier and the import declaration, and we're left guessing. Had the importer returned a Package for the missing import, the type checker could have created a PkgName object and inserted it into the Uses map, making it easier to identify the import from a SelectorExpr.

BTW: The binary importers already do something like this. They create packages when they are first referenced by the export data of another package, even if they later turn out not to be importable. Indeed, they must create "phantom" objects for references that precede declarations, to ensure that objects are unique and canonical. And for this reason, the creation of phantoms must occur in the importer---it can't be done in the type checker itself.

I'm happy to make the change if you agree it's worthwhile. It is backward-compatible.

@griesemer griesemer added this to the Go1.8 milestone Jun 16, 2016
@griesemer griesemer self-assigned this Jun 16, 2016
@griesemer
Copy link
Contributor

@griesemer griesemer commented Aug 18, 2016

@adonovan This seems reasonable. One question I have though: Why shouldn't go/types simply create a fake package if there's none, like we do for import "C". It could also do that where the importer is not installed. Furthermore, w/o an API change, it's only go/types that can set the fake bit in a package at the moment.

An issue with go/types creating fake packages is that they won't be added to the importer-known list of packages - but since they are fake it shouldn't matter.

Comments?

Loading

@alandonovan
Copy link
Contributor

@alandonovan alandonovan commented Feb 22, 2017

It's more likely to create inconsistencies for applications if the types.Package denoted by a given import path varies from one package to the next and is not equal to any package known to the importer. Also, in the vet example above, creating fake packages would cause all references to missingpkg.Member to resolve to the same (fake) object, no matter which package contains the reference.

Loading

@griesemer
Copy link
Contributor

@griesemer griesemer commented Feb 24, 2017

@alandonovan go/types could maintain its own map of fake packages for consistency. But the real issue is import path resolution: Only the importers (can) do the mapping of (import path, src directory) -> package path, and it's the package path that's the key for the package. Thus the importers must do it.

Loading

@alandonovan
Copy link
Contributor

@alandonovan alandonovan commented Feb 24, 2017

Good point.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 2, 2017

CL https://golang.org/cl/37697 mentions this issue.

Loading

@griesemer
Copy link
Contributor

@griesemer griesemer commented Mar 3, 2017

I've now written 2 (pending) CLs and the suggested approach of always returning a package is starting to feel wrong: There's simply too much machinery involved for a (relatively) unlikely error scenario. The current approach requires every importer (we have 3 now) to handle this, and there are many possible error conditions. Either we are deliberate and handle each case separately (too complicated and error prone), or we factor it out somehow. Ideally the go/importer should handle it (in fact it could).

However, once the go/importer can handle this, we can just as well have go/types do the work - it's the one major (perhaps only) client.

The problem is significantly simplified if we change the importer contract only slightly in case of an import failure:

  • if a package has already been created, then return that package and the error
  • otherwise, just return a nil package and the error

The former case appears if we do have a package that couldn't be imported entirely for some reason, or that has been partially imported. It's fine to use that package, while keeping in mind that follow-on errors from not finding objects in the package should be ignored.

The latter case appears when the package cannot be found in the first place. In that case we don't have a package at all, and we can leave it to go/types to (internally) create a fake (placeholder) package. Since the package couldn't be found in the first place, it doesn't matter what import path resolution amounted to. go/types simply creates a new fake package for each distinct (import path, src dir) pair thus avoiding collapse of multiple different packages into a single one.

Such an approach is much simpler to implement, more localized, and should essentially provide the same effect.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 3, 2017

CL https://golang.org/cl/37755 mentions this issue.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 3, 2017

CL https://golang.org/cl/37756 mentions this issue.

Loading

@gopherbot gopherbot closed this in 2ad7453 Mar 6, 2017
gopherbot pushed a commit that referenced this issue Mar 6, 2017
… of error

For #16088.

Change-Id: I0ff480e95ef5af375be2ccc655f8b233a7bcd39d
Reviewed-on: https://go-review.googlesource.com/37755
Reviewed-by: Alan Donovan <adonovan@google.com>
gopherbot pushed a commit that referenced this issue Mar 6, 2017
…of error

For #16088.

Change-Id: Ib38bda06a5c5d110ca86510043775c5cf229e6a8
Reviewed-on: https://go-review.googlesource.com/37756
Reviewed-by: Alan Donovan <adonovan@google.com>
@golang golang locked and limited conversation to collaborators Mar 6, 2018
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
6 participants