gopls/internal/server: disambiguate diagnostics by OS,ARCH

This change adds a disambiguating suffix such as " [windows,arm64]"
to diagnostics that do not appear in the default build configuration.

Fixes golang/go#65496

Change-Id: Ided7a7110ff630e57b7a96a311a240fef210ca93
Reviewed-on: https://go-review.googlesource.com/c/tools/+/563876
Reviewed-by: Robert Findley <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: adonovan

gopls/internal/server/diagnostics.go

@@ -11,6 +11,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"runtime"
 	"sort"
 	"strings"
 	"sync"
@@ -61,7 +62,9 @@ type (
 	diagMap = map[protocol.DocumentURI][]*cache.Diagnostic
 )
 
-// hashDiagnostics computes a hash to identify a diagnostic.
+// hashDiagnostic computes a hash to identify a diagnostic.
+// The hash is for deduplicating within a file,
+// so it need not incorporate d.URI.
 func hashDiagnostic(d *cache.Diagnostic) file.Hash {
 	h := sha256.New()
 	for _, t := range d.Tags {
@@ -822,23 +825,25 @@ func (s *server) updateOrphanedFileDiagnostics(ctx context.Context, modID uint64
 //
 // If the publication succeeds, it updates f.publishedHash and f.mustPublish.
 func (s *server) publishFileDiagnosticsLocked(ctx context.Context, views viewSet, uri protocol.DocumentURI, version int32, f *fileDiagnostics) error {
-	// Check that the set of views is up-to-date, and de-dupe diagnostics
-	// across views.
-	var (
-		diagHashes = make(map[file.Hash]unit) // unique diagnostic hashes
-		hash       file.Hash                  // XOR of diagnostic hashes
-		unique     []*cache.Diagnostic        // unique diagnostics
-	)
-	add := func(diag *cache.Diagnostic) {
+
+	// We add a disambiguating suffix (e.g. " [darwin,arm64]") to
+	// each diagnostic that doesn't occur in the default view;
+	// see golang/go#65496.
+	type diagSuffix struct {
+		diag   *cache.Diagnostic
+		suffix string // "" for default build (or orphans)
+	}
+
+	// diagSuffixes records the set of view suffixes for a given diagnostic.
+	diagSuffixes := make(map[file.Hash][]diagSuffix)
+	add := func(diag *cache.Diagnostic, suffix string) {
 		h := hashDiagnostic(diag)
-		if _, ok := diagHashes[h]; !ok {
-			diagHashes[h] = unit{}
-			unique = append(unique, diag)
-			hash.XORWith(h)
-		}
+		diagSuffixes[h] = append(diagSuffixes[h], diagSuffix{diag, suffix})
 	}
+
+	// Construct the inverse mapping, from diagnostic (hash) to its suffixes (views).
 	for _, diag := range f.orphanedFileDiagnostics {
-		add(diag)
+		add(diag, "")
 	}
 	for view, viewDiags := range f.byView {
 		if _, ok := views[view]; !ok {
@@ -848,10 +853,53 @@ func (s *server) publishFileDiagnosticsLocked(ctx context.Context, views viewSet
 		if viewDiags.version != version {
 			continue // a payload of diagnostics applies to a specific file version
 		}
+
+		// Compute the view's suffix (e.g. " [darwin,arm64]").
+		var suffix string
+		{
+			var words []string
+			if view.GOOS() != runtime.GOOS {
+				words = append(words, view.GOOS())
+			}
+			if view.GOARCH() != runtime.GOARCH {
+				words = append(words, view.GOARCH())
+			}
+			if len(words) > 0 {
+				suffix = fmt.Sprintf(" [%s]", strings.Join(words, ","))
+			}
+		}
+
 		for _, diag := range viewDiags.diagnostics {
-			add(diag)
+			add(diag, suffix)
 		}
 	}
+
+	// De-dup diagnostics across views by hash, and sort.
+	var (
+		hash   file.Hash
+		unique []*cache.Diagnostic
+	)
+	for h, items := range diagSuffixes {
+		// Sort the items by ascending suffix, so that the
+		// default view (if present) is first.
+		// (The others are ordered arbitrarily.)
+		sort.Slice(items, func(i, j int) bool {
+			return items[i].suffix < items[j].suffix
+		})
+
+		// If the diagnostic was not present in
+		// the default view, add the view suffix.
+		first := items[0]
+		if first.suffix != "" {
+			diag2 := *first.diag // shallow copy
+			diag2.Message += first.suffix
+			first.diag = &diag2
+			h = hashDiagnostic(&diag2) // update the hash
+		}
+
+		hash.XORWith(h)
+		unique = append(unique, first.diag)
+	}
 	sortDiagnostics(unique)
 
 	// Publish, if necessary.

gopls/internal/test/marker/testdata/diagnostics/osarch_suffix.txt

@@ -0,0 +1,46 @@
+This test verifies that we add an [os,arch] suffix to each diagnostic
+that doesn't appear in the default build (=runtime.{GOOS,GOARCH}).
+
+See golang/go#65496.
+
+The two p/*.go files below are written to trigger the same diagnostic
+(range, message, source, etc) but varying only by URI.
+
+In the q test, a single location in the common code q.go has two
+diagnostics, one of which is tagged.
+
+This test would fail on openbsd/mips64 because it will be
+the same as the default build, so we skip that platform.
+
+-- flags --
+-skip_goos=openbsd
+
+-- go.mod --
+module example.com
+
+-- p/p.go --
+package p
+
+var _ fmt.Stringer //@diag("fmt", re"unde.*: fmt$")
+
+-- p/p_openbsd_mips64.go --
+package p
+
+var _ fmt.Stringer //@diag("fmt", re"unde.*: fmt \\[openbsd,mips64\\]")
+
+-- q/q_default.go --
+//+build !openbsd && !mips64
+
+package q
+
+func f(int) int
+
+-- q/q_openbsd_mips64.go --
+package q
+
+func f(string) int
+
+-- q/q.go --
+package q
+
+var _ = f() //@ diag(")", re`.*want \(string\) \[openbsd,mips64\]`), diag(")", re`.*want \(int\)$`)