go/packages: include "unsafe".GoFiles=["unsafe.go"]

This change causes the unsafe package's GoFiles list
to include unsafe.go, and documents the field more precisely.
This file is never compiled, but it is legal Go syntax,
and serves as documentation. It is useful for client
tools to know where to find it.

Also, remove the corresponding workaround in gopls.

Fixes golang/go#59929

Change-Id: I4ef9f4c16c5b5b74ee7a7c4d1f7eb3736f779b91
Reviewed-on: https://go-review.googlesource.com/c/tools/+/491375
Reviewed-by: Robert Findley <[email protected]>
Run-TryBot: Alan Donovan <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>
Reviewed-by: Michael Matloob <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>

Author: adonovan

go/packages/golist.go

@@ -625,7 +625,12 @@ func (state *golistState) createDriverResponse(words ...string) (*driverResponse
 		}
 
 		if pkg.PkgPath == "unsafe" {
-			pkg.GoFiles = nil // ignore fake unsafe.go file
+			pkg.CompiledGoFiles = nil // ignore fake unsafe.go file (#59929)
+		} else if len(pkg.CompiledGoFiles) == 0 {
+			// Work around for pre-go.1.11 versions of go list.
+			// TODO(matloob): they should be handled by the fallback.
+			// Can we delete this?
+			pkg.CompiledGoFiles = pkg.GoFiles
 		}
 
 		// Assume go list emits only absolute paths for Dir.
@@ -663,13 +668,6 @@ func (state *golistState) createDriverResponse(words ...string) (*driverResponse
 			response.Roots = append(response.Roots, pkg.ID)
 		}
 
-		// Work around for pre-go.1.11 versions of go list.
-		// TODO(matloob): they should be handled by the fallback.
-		// Can we delete this?
-		if len(pkg.CompiledGoFiles) == 0 {
-			pkg.CompiledGoFiles = pkg.GoFiles
-		}
-
 		// Temporary work-around for golang/go#39986. Parse filenames out of
 		// error messages. This happens if there are unrecoverable syntax
 		// errors in the source, so we can't match on a specific error message.

go/packages/packages.go

@@ -308,6 +308,9 @@ type Package struct {
 	TypeErrors []types.Error
 
 	// GoFiles lists the absolute file paths of the package's Go source files.
+	// It may include files that should not be compiled, for example because
+	// they contain non-matching build tags, are documentary pseudo-files such as
+	// unsafe/unsafe.go or builtin/builtin.go, or are subject to cgo preprocessing.
 	GoFiles []string
 
 	// CompiledGoFiles lists the absolute file paths of the package's source

go/packages/packages_test.go

@@ -217,7 +217,7 @@ func testLoadImportsGraph(t *testing.T, exporter packagestest.Exporter) {
 		id          string
 		wantName    string
 		wantKind    string
-		wantSrcs    string
+		wantSrcs    string // = {Go,Other,Embed}Files
 		wantIgnored string
 	}{
 		{"golang.org/fake/a", "a", "package", "a.go", ""},
@@ -227,7 +227,7 @@ func testLoadImportsGraph(t *testing.T, exporter packagestest.Exporter) {
 		{"container/list", "list", "package", "list.go", ""},
 		{"golang.org/fake/subdir/d", "d", "package", "d.go", ""},
 		{"golang.org/fake/subdir/d.test", "main", "command", "0.go", ""},
-		{"unsafe", "unsafe", "package", "", ""},
+		{"unsafe", "unsafe", "package", "unsafe.go", ""},
 	} {
 		p, ok := all[test.id]
 		if !ok {
@@ -250,10 +250,10 @@ func testLoadImportsGraph(t *testing.T, exporter packagestest.Exporter) {
 		}
 
 		if srcs := strings.Join(srcs(p), " "); srcs != test.wantSrcs {
-			t.Errorf("%s.Srcs = [%s], want [%s]", test.id, srcs, test.wantSrcs)
+			t.Errorf("%s.{Go,Other,Embed}Files = [%s], want [%s]", test.id, srcs, test.wantSrcs)
 		}
 		if ignored := strings.Join(cleanPaths(p.IgnoredFiles), " "); ignored != test.wantIgnored {
-			t.Errorf("%s.Srcs = [%s], want [%s]", test.id, ignored, test.wantIgnored)
+			t.Errorf("%s.IgnoredFiles = [%s], want [%s]", test.id, ignored, test.wantIgnored)
 		}
 	}
 
@@ -2788,7 +2788,11 @@ func errorMessages(errors []packages.Error) []string {
 }
 
 func srcs(p *packages.Package) []string {
-	return cleanPaths(append(append(p.GoFiles[:len(p.GoFiles):len(p.GoFiles)], p.OtherFiles...), p.EmbedFiles...))
+	var files []string
+	files = append(files, p.GoFiles...)
+	files = append(files, p.OtherFiles...)
+	files = append(files, p.EmbedFiles...)
+	return cleanPaths(files)
 }
 
 // cleanPaths attempts to reduce path names to stable forms

gopls/internal/lsp/cache/load.go

@@ -159,32 +159,6 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc
 		return bug.Errorf("internal error: go/packages returned multiple packages for standalone file")
 	}
 
-	// Workaround for a bug (?) that has been in go/packages since
-	// the outset: Package("unsafe").GoFiles=[], whereas it should
-	// include unsafe/unsafe.go. Derive it from builtins.go.
-	//
-	// This workaround relies on the fact that we always add both
-	// builtins and unsafe to the set of scopes in the workspace load.
-	//
-	// TODO(adonovan): fix upstream in go/packages.
-	// (Does this need a proposal? Arguably not.)
-	{
-		var builtin, unsafe *packages.Package
-		for _, pkg := range pkgs {
-			switch pkg.ID {
-			case "unsafe":
-				unsafe = pkg
-			case "builtin":
-				builtin = pkg
-			}
-		}
-		if builtin != nil && unsafe != nil && len(builtin.GoFiles) == 1 {
-			unsafe.GoFiles = []string{
-				filepath.Join(filepath.Dir(builtin.GoFiles[0]), "../unsafe/unsafe.go"),
-			}
-		}
-	}
-
 	moduleErrs := make(map[string][]packages.Error) // module path -> errors
 	filterFunc := s.view.filterFunc()
 	newMetadata := make(map[PackageID]*source.Metadata)

gopls/internal/lsp/cache/view.go

@@ -862,10 +862,8 @@ func (s *snapshot) loadWorkspace(ctx context.Context, firstAttempt bool) (loadEr
 	// If we're loading anything, ensure we also load builtin,
 	// since it provides fake definitions (and documentation)
 	// for types like int that are used everywhere.
-	// ("unsafe" is also needed since its sole GoFiles is
-	// derived from that of "builtin" via a workaround in load.)
 	if len(scopes) > 0 {
-		scopes = append(scopes, packageLoadScope("builtin"), packageLoadScope("unsafe"))
+		scopes = append(scopes, packageLoadScope("builtin"))
 	}
 	loadErr = s.load(ctx, true, scopes...)
 

gopls/internal/regtest/misc/references_test.go

@@ -104,7 +104,7 @@ func _() {
 
 func TestDefsRefsBuiltins(t *testing.T) {
 	testenv.NeedsGo1Point(t, 17) // for unsafe.{Add,Slice}
-	// TODO(adonovan): add unsafe.SliceData,String,StringData} in later go versions.
+	// TODO(adonovan): add unsafe.{SliceData,String,StringData} in later go versions.
 	const files = `
 -- go.mod --
 module example.com

gopls/internal/regtest/misc/workspace_symbol_test.go

@@ -32,8 +32,6 @@ package exclude
 const K2 = "exclude.go"
 `
 
-	// NB: the name K was chosen to avoid spurious
-	// matches in the always-present "unsafe" package.
 	Run(t, files, func(t *testing.T, env *Env) {
 		env.OpenFile("a.go")
 		checkSymbols(env, "K", "K1")
@@ -73,7 +71,6 @@ const (
 			"Fooex",  // shorter than Fooest, FooBar, lexically before Fooey
 			"Fooey",  // shorter than Fooest, Foobar
 			"Fooest",
-			"unsafe.Offsetof", // a very fuzzy match
 		)
 	})
 }