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

[mvdan]

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

[mvdan]

Here is where I found the bug: burrowers/garble#236

As you might expect, the fix is: if we're typechecking a std package, use the default importer rather than our custom importer with a lookup func. That works, but it's unfortunate that it's inconsistent and requires a workaround.

[mvdan]

Forgot to cc @findleyr since he's been working on go/types recently.

[mvdan]

So the go/types.Importer implementation itself needs to close over that bit of information.

So I think we both agree that this is something to fix in go/types. I arrived at that conslusion after reading assume that caller has converted path to a canonical import path and double-checking that there isn't a bug in my lookup func or the gcimporter package.

I'd love to help fix this issue, though I'm not sure how to proceed unless a go/types maintainer can chime in :) I imagine few people are actually implementing a lookup func directly, given that it doesn't work on some std packages.

[findleyr]

A couple comments. A lot of this is new to me, so my opinions are not particularly informed.

The error message is ambiguous, which perhaps is a go/types bug.

@mvdan: This is a go/types bug, specifically #43119. It should get fixed for 1.17.

To give a little more detail about how std vendoring works at the moment:

@bcmills: Keying off the phrase 'at the moment': is this subject to change?

So I think we both agree that this is something to fix in go/types

@mvdan not sure if you meant that the code change belongs in go/types, but go/types should not need to know what the main module is: IIUC since this information is constant throughout the the typechecking pass, is only needed for importing, and is otherwise unnecessary for type checking, it should be closed over by the importer as @bcmills suggests. I don't think the comment "assume that caller has converted path to a canonical import path" means following the std vendoring rules that Bryan describes above.

Perhaps it would be possible to wrap importer.ForCompiler(...) and mutate import paths following the std vendoring rules.

[mvdan]

@mvdan: This is a go/types bug, specifically #43119. It should get fixed for 1.17.

Great, thank you.

not sure if you meant that the code change belongs in go/types

I did come to the conclusion that the fix probably belongs in go/types, simply by elimination that I could not find a bug to fix in any of the importer packages (and assuming that we don't want new API). But I'm not familiar with how go/types deals with importing packages internally, so I'm leaving that to the maintainers.

[bcmills]

@mvdan, I wonder if this is actually fixed by CL 297869 (#44725).

If so, would it make sense to backport that fix to Go 1.16.2 to resolve your issue?

[mvdan]

I tried re-running my tests with go version devel +cf59850466 Wed Mar 10 09:01:05 2021 +0000 linux/amd64 and without the importer.Default() workaround, and I still get the same error. The playground link in the original post is also the same, unfortunately.

[bcmills]

@findleyr

Keying off the phrase 'at the moment': is this subject to change?

I hope that eventually the vendored dependencies can participate in MVS instead of being stuck at specific versions (that's #30241). But that's not happening in the short term, if ever.

[bcmills]

@findleyr, my comment in #44630 (comment) is no longer correct as of CL 297869, so I'm going to rewrite it here with the up-to-date version.

If you are working in a module within GOROOT/src, then the dependencies in the vendor directory of that module are mostly treated as ordinary packages in the standard library, with the prefix vendor/ or cmd/vendor/. There are two exceptions:

1. Except in go get and go mod subcommands, an import statement in a source file within GOROOT/src that refers to a vendored package is interpreted as referring to the vendor/ or cmd/vendor/ path rather than the path as written.

2. In go get and go mod subcommands within GOROOT/src, the vendor/ packages in the current GOROOT/src module are ignored and the import statements in source files instead refer to the packages in their respective actual modules.

So in order to obtain the correct canonical name for an import, you need to know two things:

1. Is the import statement found within a package in std or cmd, or is it in some other module?
2. Which package is being imported?

The path argument to go/types.ImporterFrom answers (2), and the dir argument answers (1).

[gopherbot]

Change https://golang.org/cl/310036 mentions this issue: go/internal/gcimporter: support loading std-vendored packages with a lookup func

[mvdan]

I ended up fixing this on my side by wrapping the types.ImporterFrom to canonicalize import paths: burrowers/garble#316

Does this feel like the right solution overall?

If so, then the logical next question is - should we document the need for the creator of the importer to wrap it if using a lookup func? It took me months to arrive at that conclusion, and even though it works I'm still not sure that the API is meant to be used that way.

If the answer is "yes, you are meant to take care of this yourself", I think we need better docs, or ideally an API that is harder to misuse.

[bcmills]

even though it works I'm still not sure that the API is meant to be used that way.

I honestly don't know how go/importer is meant to be used. I agree that the API seems much more complex than necessary, but I'm not sure to what extent we can salvage it in a backward-compatible way (vs. creating a new, simpler entry-point).