Repository: go
Issue: go/internal/gcimporter: does not support loading vendor packages in std with a lookup func
Labels: NeedsInvestigation

This happens with both `go version devel +e25040d162 Fri Feb 26 02:52:33 2021 +0000 linux/amd64` and `go version go1.16 linux/amd64`. This will be a bit long, as it took me a while to fully understand what's going on.

Take the following main program: https://play.golang.org/p/tT_ocdRrssx

It parses and typechecks the `crypto/ecdsa` package in two ways. Note that this package is special, because it imports two vendored `golang.org/x/crypto` packages:

```
$ go list -json crypto/ecdsa
{
	"Dir": "/home/mvdan/tip/src/crypto/ecdsa",
	"ImportPath": "crypto/ecdsa",
	[...]
	"ImportMap": {
		"golang.org/x/crypto/cryptobyte": "vendor/golang.org/x/crypto/cryptobyte",
		"golang.org/x/crypto/cryptobyte/asn1": "vendor/golang.org/x/crypto/cryptobyte/asn1"
	},
```

First, we try using `importer.ForCompiler(fset, "gc", nil)`. This works fine; when no lookup func is used, it calls https://golang.org/pkg/go/internal/gcimporter/#FindPkg, which works well.

Second, we try using the same as the above, but with a custom lookup func. The custom lookup func is backed by a single `go list -json -deps -export` call before typechecking begins. I'm fairly certain this *should* work, because the `go list` call returns all information needed, and because I can correctly resolve the lookup of those vendored packages.

However, the second method fails to typecheck:

```
$ go run importer.go

Without lookup func:


With lookup func:

2021/02/26 10:37:39 typecheck with lookup via 'go list': typecheck error: /home/mvdan/tip/src/crypto/ecdsa/ecdsa.go:122:12: cannot use asn1.SEQUENCE (constant 48 of type asn1.Tag) as asn1.Tag value in argument to b.AddASN1
exit status 1
```

The error message is ambiguous, which perhaps is a go/types bug. I use the following patch on Go master to get more details on what's going on:

```diff
diff --git a/src/go/internal/gcimporter/gcimporter.go b/src/go/internal/gcimporter/gcimporter.go
index b74daca246..10f15bd059 100644
--- a/src/go/internal/gcimporter/gcimporter.go
+++ b/src/go/internal/gcimporter/gcimporter.go
@@ -132,6 +132,7 @@ func Import(fset *token.FileSet, packages map[string]*types.Package, path, srcDi
 		}()
 		rc = f
 	}
+	fmt.Printf("Importing package %s as %s\n", path, id)
 	defer rc.Close()
 
 	var hdr string
diff --git a/src/go/types/assignments.go b/src/go/types/assignments.go
index f223cb7574..9dad68c593 100644
--- a/src/go/types/assignments.go
+++ b/src/go/types/assignments.go
@@ -7,6 +7,7 @@
 package types
 
 import (
+	"fmt"
 	"go/ast"
 	"go/token"
 )
@@ -87,6 +88,7 @@ func (check *Checker) assignment(x *operand, T Type, context string) {
 		if reason != "" {
 			check.errorf(x, code, "cannot use %s as %s value in %s: %s", x, T, context, reason)
 		} else {
+			fmt.Printf("Full types error: cannot use %s as %s value in %s\n", x, T, context)
 			check.errorf(x, code, "cannot use %s as %s value in %s", x, T, context)
 		}
 		x.mode = invalid
```

Now, we get:

```
$ go run importer.go

Without lookup func:

Importing package crypto as crypto
Importing package crypto/aes as crypto/aes
Importing package crypto/cipher as crypto/cipher
Importing package crypto/elliptic as crypto/elliptic
Importing package crypto/internal/randutil as crypto/internal/randutil
Importing package crypto/sha512 as crypto/sha512
Importing package errors as errors
Importing package io as io
Importing package math/big as math/big
Importing package golang.org/x/crypto/cryptobyte as vendor/golang.org/x/crypto/cryptobyte
Importing package golang.org/x/crypto/cryptobyte/asn1 as vendor/golang.org/x/crypto/cryptobyte/asn1

With lookup func:

Importing package crypto as crypto
Importing package crypto/aes as crypto/aes
Importing package crypto/cipher as crypto/cipher
Importing package crypto/elliptic as crypto/elliptic
Importing package crypto/internal/randutil as crypto/internal/randutil
Importing package crypto/sha512 as crypto/sha512
Importing package errors as errors
Importing package io as io
Importing package math/big as math/big
Importing package golang.org/x/crypto/cryptobyte as golang.org/x/crypto/cryptobyte
Importing package golang.org/x/crypto/cryptobyte/asn1 as golang.org/x/crypto/cryptobyte/asn1
Full types error: cannot use asn1.SEQUENCE (constant 48 of type golang.org/x/crypto/cryptobyte/asn1.Tag) as vendor/golang.org/x/crypto/cryptobyte/asn1.Tag value in argument to b.AddASN1
2021/02/26 10:39:20 typecheck with lookup via 'go list': typecheck error: /home/mvdan/tip/src/crypto/ecdsa/ecdsa.go:122:12: cannot use asn1.SEQUENCE (constant 48 of type asn1.Tag) as asn1.Tag value in argument to b.AddASN1
exit status 1
```

Two important things to note here.

First, the error is because the constant comes from the package `golang.org/x/crypto/cryptobyte/asn1`, while the argument type comes from the package `vendor/golang.org/x/crypto/cryptobyte/asn1`. The first is wrong, as it's lacking the `vendor/` prefix. In the first method, both end up with the vendor prefix; this is required, because a Go module can also import the actual `golang.org/x/crypto/cryptobyte/asn1` package from the real module, and that needs to be a separate import path from the std-vendored version.

Second, note that the first method ends up importing both golang.org/x/crypto packages with the `vendor/` prefix, but the second method does not. Our lookup func does return the right export data file thanks to ImportMap, but since we can *only* return the file, we cannot tell gcimporter what import path the original path should be replaced with.

In the source of `gcimporter.Import`, here's why the "missing lookup func" case works; `id` is the canonical import path for the package that is being imported:

```
filename, id = FindPkg(path, srcDir)
```

`FindPkg` correctly returns `id = "vendor/golang.org/x/crypto/cryptobyte/asn1"` for `path = "golang.org/x/crypto/cryptobyte/asn1"`.

However, in the case where the lookup func is specified, gcimporter forces `id = path`:

```
// With custom lookup specified, assume that caller has
// converted path to a canonical import path for use in the map.
if path == "unsafe" {
	return types.Unsafe, nil
}
id = path
```

There is a comment explaining this inconsistency, but I still think there's a bug somewhere. The code calling `gcimporter.Import` is not my code, it's go/types. So it appears that if the bug is not in `gcimporter`, and it's on the caller to canonicalize the import path - then `go/types` has a bug as it is not doing that.

cc @griesemer @alandonovan as owners