all: overcoming srcimporter issues (#288)

See go-critic/go-critic#1126

We're using the host-specific `go env` info to avoid incorrect
GOROOT and other Go build context variables.

Author: quasilyte

Makefile

@@ -30,7 +30,7 @@ lint:
 	curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(GOPATH_DIR)/bin v1.30.0
 	$(GOPATH_DIR)/bin/golangci-lint run ./...
 	go build -o go-ruleguard ./cmd/ruleguard
-	./go-ruleguard -rules rules.go ./...
+	./go-ruleguard -debug-imports -rules rules.go ./...
 	@echo "everything is OK"
 
 .PHONY: lint test test-master build build-release

analyzer/analyzer.go

@@ -169,6 +169,7 @@ func prepareEngine() (*ruleguard.Engine, error) {
 
 func newEngine() (*ruleguard.Engine, error) {
 	e := ruleguard.NewEngine()
+	e.InferBuildContext()
 	fset := token.NewFileSet()
 
 	disabledGroups := make(map[string]bool)

cmd/ruleguard/main.go

@@ -1,36 +1,10 @@
 package main
 
 import (
-	"go/build"
-	"log"
-	"os/exec"
-	"strings"
-
 	"github.com/quasilyte/go-ruleguard/analyzer"
 	"golang.org/x/tools/go/analysis/singlechecker"
 )
 
 func main() {
-	// If we don't do this, release binaries will have GOROOT set
-	// to the `go env GOROOT` of the machine that built them.
-	//
-	// Usually, it doesn't matter, but since we're using "source"
-	// importers, it *will* use build.Default.GOROOT to locate packages.
-	//
-	// Example: release binary was built with GOROOT="/foo/bar/go",
-	// user has GOROOT at "/usr/local/go"; if we don't adjust GOROOT
-	// field here, it'll be "/foo/bar/go".
-	build.Default.GOROOT = hostGOROOT()
-
 	singlechecker.Main(analyzer.Analyzer)
 }
-
-func hostGOROOT() string {
-	// `go env GOROOT` should return the correct value even
-	// if it was overwritten by explicit GOROOT env var.
-	out, err := exec.Command("go", "env", "GOROOT").CombinedOutput()
-	if err != nil {
-		log.Fatalf("infer GOROOT: %v: %s", err, out)
-	}
-	return strings.TrimSpace(string(out))
-}

internal/xsrcimporter/xsrcimporter.go

@@ -0,0 +1,29 @@
+package xsrcimporter
+
+import (
+	"go/build"
+	"go/importer"
+	"go/token"
+	"go/types"
+	"unsafe"
+)
+
+func New(ctxt *build.Context, fset *token.FileSet) types.Importer {
+	imp := importer.ForCompiler(fset, "source", nil)
+	ifaceVal := *(*iface)(unsafe.Pointer(&imp))
+	srcImp := (*srcImporter)(ifaceVal.data)
+	srcImp.ctxt = ctxt
+	return imp
+}
+
+type iface struct {
+	_    *byte
+	data unsafe.Pointer
+}
+
+type srcImporter struct {
+	ctxt *build.Context
+	_    *token.FileSet
+	_    types.Sizes
+	_    map[string]*types.Package
+}

internal/xsrcimporter/xsrcimporter_test.go

@@ -0,0 +1,45 @@
+package xsrcimporter
+
+import (
+	"go/build"
+	"go/importer"
+	"go/token"
+	"reflect"
+	"testing"
+	"unsafe"
+)
+
+func TestSize(t *testing.T) {
+	fset := token.NewFileSet()
+	imp := importer.ForCompiler(fset, "source", nil)
+	have := unsafe.Sizeof(srcImporter{})
+	want := reflect.ValueOf(imp).Elem().Type().Size()
+	if have != want {
+		t.Errorf("sizes mismatch: have %d want %d", have, want)
+	}
+}
+
+func TestImport(t *testing.T) {
+	fset := token.NewFileSet()
+	imp := New(&build.Default, fset)
+
+	packages := []string{
+		"errors",
+		"fmt",
+		"encoding/json",
+	}
+
+	for _, pkgPath := range packages {
+		pkg, err := imp.Import(pkgPath)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if pkg.Path() != pkgPath {
+			t.Fatalf("%s: pkg path mismatch (got %s)", pkgPath, pkg.Path())
+		}
+		if !pkg.Complete() {
+			t.Fatalf("%s is incomplete", pkgPath)
+		}
+	}
+
+}

ruleguard/engine.go

@@ -1,14 +1,18 @@
 package ruleguard
 
 import (
+	"bytes"
 	"errors"
 	"fmt"
 	"go/ast"
+	"go/build"
 	"go/token"
 	"go/types"
 	"io"
 	"io/ioutil"
+	"os/exec"
 	"sort"
+	"strconv"
 	"strings"
 	"sync"
 
@@ -41,7 +45,7 @@ func (e *engine) LoadedGroups() []GoRuleGroup {
 	return result
 }
 
-func (e *engine) Load(ctx *LoadContext, filename string, r io.Reader) error {
+func (e *engine) Load(ctx *LoadContext, buildContext *build.Context, filename string, r io.Reader) error {
 	data, err := ioutil.ReadAll(r)
 	if err != nil {
 		return err
@@ -50,6 +54,7 @@ func (e *engine) Load(ctx *LoadContext, filename string, r io.Reader) error {
 		fset:         ctx.Fset,
 		debugImports: ctx.DebugImports,
 		debugPrint:   ctx.DebugPrint,
+		buildContext: buildContext,
 	})
 	irfile, pkg, err := convertAST(ctx, imp, filename, data)
 	if err != nil {
@@ -82,11 +87,12 @@ func (e *engine) Load(ctx *LoadContext, filename string, r io.Reader) error {
 	return nil
 }
 
-func (e *engine) LoadFromIR(ctx *LoadContext, filename string, f *ir.File) error {
+func (e *engine) LoadFromIR(ctx *LoadContext, buildContext *build.Context, filename string, f *ir.File) error {
 	imp := newGoImporter(e.state, goImporterConfig{
 		fset:         ctx.Fset,
 		debugImports: ctx.DebugImports,
 		debugPrint:   ctx.DebugPrint,
+		buildContext: buildContext,
 	})
 	config := irLoaderConfig{
 		state:      e.state,
@@ -114,12 +120,12 @@ func (e *engine) LoadFromIR(ctx *LoadContext, filename string, f *ir.File) error
 	return nil
 }
 
-func (e *engine) Run(ctx *RunContext, f *ast.File) error {
+func (e *engine) Run(ctx *RunContext, buildContext *build.Context, f *ast.File) error {
 	if e.ruleSet == nil {
 		return errors.New("used Run() with an empty rule set; forgot to call Load() first?")
 	}
 	rset := e.ruleSet
-	return newRulesRunner(ctx, e.state, rset).run(f)
+	return newRulesRunner(ctx, buildContext, e.state, rset).run(f)
 }
 
 // engineState is a shared state inside the engine.
@@ -231,3 +237,37 @@ func (state *engineState) findTypeNoCache(importer *goImporter, currentPkg *type
 	state.typeByFQN[fqn] = typ
 	return typ, nil
 }
+
+func inferBuildContext() *build.Context {
+	goEnv := func() map[string]string {
+		out, err := exec.Command("go", "env").CombinedOutput()
+		if err != nil {
+			return nil
+		}
+		vars := make(map[string]string)
+		for _, l := range bytes.Split(out, []byte("\n")) {
+			parts := strings.Split(strings.TrimSpace(string(l)), "=")
+			if len(parts) != 2 {
+				continue
+			}
+			val, err := strconv.Unquote(parts[1])
+			if err != nil {
+				continue
+			}
+			vars[parts[0]] = val
+		}
+		return vars
+	}
+
+	// Inherit most fields from the build.Default.
+	ctx := build.Default
+
+	env := goEnv()
+
+	ctx.GOROOT = env["GOROOT"]
+	ctx.GOPATH = env["GOPATH"]
+	ctx.GOARCH = env["GOARCH"]
+	ctx.GOOS = env["GOOS"]
+
+	return &ctx
+}

ruleguard/importer.go

@@ -2,15 +2,13 @@ package ruleguard
 
 import (
 	"fmt"
-	"go/ast"
+	"go/build"
 	"go/importer"
-	"go/parser"
 	"go/token"
 	"go/types"
-	"path/filepath"
 	"runtime"
 
-	"github.com/quasilyte/go-ruleguard/internal/golist"
+	"github.com/quasilyte/go-ruleguard/internal/xsrcimporter"
 )
 
 // goImporter is a `types.Importer` that tries to load a package no matter what.
@@ -23,7 +21,8 @@ type goImporter struct {
 	defaultImporter types.Importer
 	srcImporter     types.Importer
 
-	fset *token.FileSet
+	fset         *token.FileSet
+	buildContext *build.Context
 
 	debugImports bool
 	debugPrint   func(string)
@@ -33,17 +32,20 @@ type goImporterConfig struct {
 	fset         *token.FileSet
 	debugImports bool
 	debugPrint   func(string)
+	buildContext *build.Context
 }
 
 func newGoImporter(state *engineState, config goImporterConfig) *goImporter {
-	return &goImporter{
+	imp := &goImporter{
 		state:           state,
 		fset:            config.fset,
 		debugImports:    config.debugImports,
 		debugPrint:      config.debugPrint,
 		defaultImporter: importer.Default(),
-		srcImporter:     importer.ForCompiler(config.fset, "source", nil),
+		buildContext:    config.buildContext,
 	}
+	imp.initSourceImporter()
+	return imp
 }
 
 func (imp *goImporter) Import(path string) (*types.Package, error) {
@@ -54,63 +56,40 @@ func (imp *goImporter) Import(path string) (*types.Package, error) {
 		return pkg, nil
 	}
 
-	pkg, err1 := imp.srcImporter.Import(path)
-	if err1 == nil {
+	pkg, srcErr := imp.srcImporter.Import(path)
+	if srcErr == nil {
 		imp.state.AddCachedPackage(path, pkg)
 		if imp.debugImports {
 			imp.debugPrint(fmt.Sprintf(`imported "%s" from source importer`, path))
 		}
 		return pkg, nil
 	}
 
-	pkg, err2 := imp.defaultImporter.Import(path)
-	if err2 == nil {
+	pkg, defaultErr := imp.defaultImporter.Import(path)
+	if defaultErr == nil {
 		imp.state.AddCachedPackage(path, pkg)
 		if imp.debugImports {
 			imp.debugPrint(fmt.Sprintf(`imported "%s" from %s importer`, path, runtime.Compiler))
 		}
 		return pkg, nil
 	}
 
-	// Fallback to `go list` as a last resort.
-	pkg, err3 := imp.golistImport(path)
-	if err3 == nil {
-		imp.state.AddCachedPackage(path, pkg)
-		if imp.debugImports {
-			imp.debugPrint(fmt.Sprintf(`imported "%s" from golist importer`, path))
-		}
-		return pkg, nil
-	}
-
 	if imp.debugImports {
 		imp.debugPrint(fmt.Sprintf(`failed to import "%s":`, path))
-		imp.debugPrint(fmt.Sprintf("  source importer: %v", err1))
-		imp.debugPrint(fmt.Sprintf("  %s importer: %v", runtime.Compiler, err2))
-		imp.debugPrint(fmt.Sprintf("  golist importer: %v", err3))
+		imp.debugPrint(fmt.Sprintf("  %s importer: %v", runtime.Compiler, defaultErr))
+		imp.debugPrint(fmt.Sprintf("  source importer: %v", srcErr))
+		imp.debugPrint(fmt.Sprintf("  GOROOT=%q GOPATH=%q", imp.buildContext.GOROOT, imp.buildContext.GOPATH))
 	}
 
-	return nil, err2
+	return nil, defaultErr
 }
 
-func (imp *goImporter) golistImport(path string) (*types.Package, error) {
-	golistPkg, err := golist.JSON(path)
-	if err != nil {
-		return nil, err
-	}
-
-	files := make([]*ast.File, 0, len(golistPkg.GoFiles))
-	for _, filename := range golistPkg.GoFiles {
-		fullname := filepath.Join(golistPkg.Dir, filename)
-		f, err := parser.ParseFile(imp.fset, fullname, nil, 0)
-		if err != nil {
-			return nil, err
+func (imp *goImporter) initSourceImporter() {
+	if imp.buildContext == nil {
+		if imp.debugImports {
+			imp.debugPrint("using build.Default context")
 		}
-		files = append(files, f)
+		imp.buildContext = &build.Default
 	}
-
-	// TODO: do we want to assign imp as importer for this nested typecherker?
-	// Otherwise it won't be able to resolve imports.
-	var typecheker types.Config
-	var info types.Info
-	return typecheker.Check(path, imp.fset, files, &info)
+	imp.srcImporter = xsrcimporter.New(imp.buildContext, imp.fset)
 }