Revert "gopls/internal/server: rewrite the server diagnostic tracking"

This reverts commit 184d2a7.

Reason for revert: Seems to have caused some flakes.

Fixes golang/go#64533
Fixes golang/go#64532

Change-Id: I76ffd07ebca405aed2b111668c97923b578563f8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/546855
Reviewed-by: Bryan Mills <[email protected]>
Auto-Submit: Robert Findley <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: findleyr

gopls/internal/debug/serve.go

@@ -791,6 +791,29 @@ Using session: <b>{{template "sessionlink" .Session.ID}}</b><br>
 {{if .DebugAddress}}Debug this client at: <a href="http://{{localAddress .DebugAddress}}">{{localAddress .DebugAddress}}</a><br>{{end}}
 Logfile: {{.Logfile}}<br>
 Gopls Path: {{.GoplsPath}}<br>
+<h2>Diagnostics</h2>
+{{/*Service: []protocol.Server; each server has map[uri]fileReports;
+	each fileReport: map[diagnosticSoure]diagnosticReport
+	diagnosticSource is one of 5 source
+	diagnosticReport: snapshotID and map[hash]*source.Diagnostic
+	sourceDiagnostic: struct {
+		Range    protocol.Range
+		Message  string
+		Source   string
+		Code     string
+		CodeHref string
+		Severity protocol.DiagnosticSeverity
+		Tags     []protocol.DiagnosticTag
+
+		Related []RelatedInformation
+	}
+	RelatedInformation: struct {
+		URI     protocol.DocumentURI
+		Range   protocol.Range
+		Message string
+	}
+	*/}}
+<ul>{{range $k, $v := .Service.Diagnostics}}<li>{{$k}}:<ol>{{range $v}}<li>{{.}}</li>{{end}}</ol></li>{{end}}</ul>
 {{end}}
 `))
 

gopls/internal/lsp/cache/session.go

@@ -187,6 +187,7 @@ func (s *Session) createView(ctx context.Context, def *viewDefinition, folder *F
 
 	v.snapshot = &Snapshot{
 		sequenceID:       seqID,
+		globalID:         nextSnapshotID(),
 		view:             v,
 		backgroundCtx:    backgroundCtx,
 		cancel:           cancel,

gopls/internal/lsp/cache/snapshot.go

@@ -55,6 +55,15 @@ import (
 	"golang.org/x/tools/internal/typesinternal"
 )
 
+// A GlobalSnapshotID uniquely identifies a snapshot within this process and
+// increases monotonically with snapshot creation time.
+//
+// We use a distinct integral type for global IDs to help enforce correct
+// usage.
+//
+// TODO(rfindley): remove this as it should not be necessary for correctness.
+type GlobalSnapshotID uint64
+
 // A Snapshot represents the current state for a given view.
 //
 // It is first and foremost an idempotent implementation of file.Source whose
@@ -68,6 +77,7 @@ import (
 // implemented in Snapshot.clone.
 type Snapshot struct {
 	sequenceID uint64
+	globalID   GlobalSnapshotID
 
 	// TODO(rfindley): the snapshot holding a reference to the view poses
 	// lifecycle problems: a view may be shut down and waiting for work
@@ -192,6 +202,12 @@ type Snapshot struct {
 	gcOptimizationDetails map[metadata.PackageID]unit
 }
 
+var globalSnapshotID uint64
+
+func nextSnapshotID() GlobalSnapshotID {
+	return GlobalSnapshotID(atomic.AddUint64(&globalSnapshotID, 1))
+}
+
 var _ memoize.RefCounted = (*Snapshot)(nil) // snapshots are reference-counted
 
 // Acquire prevents the snapshot from being destroyed until the returned function is called.
@@ -206,7 +222,7 @@ var _ memoize.RefCounted = (*Snapshot)(nil) // snapshots are reference-counted
 func (s *Snapshot) Acquire() func() {
 	type uP = unsafe.Pointer
 	if destroyedBy := atomic.LoadPointer((*uP)(uP(&s.destroyedBy))); destroyedBy != nil {
-		log.Panicf("%s:%d: acquire() after Destroy(%q)", s.view.id, s.sequenceID, *(*string)(destroyedBy))
+		log.Panicf("%d: acquire() after Destroy(%q)", s.globalID, *(*string)(destroyedBy))
 	}
 	s.refcount.Add(1)
 	return s.refcount.Done
@@ -245,7 +261,7 @@ func (s *Snapshot) destroy(destroyedBy string) {
 	// Not foolproof: another thread could acquire() at this moment.
 	type uP = unsafe.Pointer // looking forward to generics...
 	if old := atomic.SwapPointer((*uP)(uP(&s.destroyedBy)), uP(&destroyedBy)); old != nil {
-		log.Panicf("%s:%d: Destroy(%q) after Destroy(%q)", s.view.id, s.sequenceID, destroyedBy, *(*string)(old))
+		log.Panicf("%d: Destroy(%q) after Destroy(%q)", s.globalID, destroyedBy, *(*string)(old))
 	}
 
 	s.packages.Destroy()
@@ -273,6 +289,13 @@ func (s *Snapshot) SequenceID() uint64 {
 	return s.sequenceID
 }
 
+// GlobalID is a globally unique identifier for this snapshot. Global IDs are
+// monotonic: subsequent snapshots will have higher global ID, though
+// subsequent snapshots in a view may not have adjacent global IDs.
+func (s *Snapshot) GlobalID() GlobalSnapshotID {
+	return s.globalID
+}
+
 // SnapshotLabels returns a new slice of labels that should be used for events
 // related to a snapshot.
 func (s *Snapshot) Labels() []label.Label {
@@ -1929,6 +1952,7 @@ func (s *Snapshot) clone(ctx, bgCtx context.Context, changed StateChange) (*Snap
 	bgCtx, cancel := context.WithCancel(bgCtx)
 	result := &Snapshot{
 		sequenceID:        s.sequenceID + 1,
+		globalID:          nextSnapshotID(),
 		store:             s.store,
 		view:              s.view,
 		backgroundCtx:     bgCtx,

gopls/internal/lsp/source/util.go

@@ -11,6 +11,7 @@ import (
 	"go/token"
 	"go/types"
 	"regexp"
+	"sort"
 	"strings"
 
 	"golang.org/x/tools/gopls/internal/lsp/cache"
@@ -138,6 +139,31 @@ func Deref(typ types.Type) types.Type {
 	}
 }
 
+func SortDiagnostics(d []*cache.Diagnostic) {
+	sort.Slice(d, func(i int, j int) bool {
+		return CompareDiagnostic(d[i], d[j]) < 0
+	})
+}
+
+func CompareDiagnostic(a, b *cache.Diagnostic) int {
+	if r := protocol.CompareRange(a.Range, b.Range); r != 0 {
+		return r
+	}
+	if a.Source < b.Source {
+		return -1
+	}
+	if a.Source > b.Source {
+		return +1
+	}
+	if a.Message < b.Message {
+		return -1
+	}
+	if a.Message > b.Message {
+		return +1
+	}
+	return 0
+}
+
 // findFileInDeps finds package metadata containing URI in the transitive
 // dependencies of m. When using the Go command, the answer is unique.
 //

gopls/internal/server/code_action.go

@@ -286,8 +286,8 @@ func (s *server) findMatchingDiagnostics(uri protocol.DocumentURI, pd protocol.D
 	defer s.diagnosticsMu.Unlock()
 
 	var sds []*cache.Diagnostic
-	for _, viewDiags := range s.diagnostics[uri].byView {
-		for _, sd := range viewDiags.diagnostics {
+	for _, report := range s.diagnostics[uri].reports {
+		for _, sd := range report.diags {
 			sameDiagnostic := (pd.Message == strings.TrimSpace(sd.Message) && // extra space may have been trimmed when converting to protocol.Diagnostic
 				protocol.CompareRange(pd.Range, sd.Range) == 0 &&
 				pd.Source == string(sd.Source))