gopls: filter out hints for closed files and make modernizers hints

- Document Analyzer.Severity to describe heuristics for how severity
  should be determined.
- Filter out hint diagnostics for closed files. VS Code already
  suppresses hint diagnostics from the Problems tab, but other clients
  do not. This change makes the visibility of Hint diagnostics more
  similar across clients.
- Downgrade 'modernize' to Hint level severity.

Updates golang/go#70815

Change-Id: If93b57d25ed3eb8dc253a3c7ef016c4148086dc9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/641796
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

Author: findleyr

gopls/internal/cache/errors.go

@@ -270,15 +270,10 @@ func toSourceDiagnostic(srcAnalyzer *settings.Analyzer, gobDiag *gobDiagnostic)
 		related = append(related, protocol.DiagnosticRelatedInformation(gobRelated))
 	}
 
-	severity := srcAnalyzer.Severity()
-	if severity == 0 {
-		severity = protocol.SeverityWarning
-	}
-
 	diag := &Diagnostic{
 		URI:      gobDiag.Location.URI,
 		Range:    gobDiag.Location.Range,
-		Severity: severity,
+		Severity: srcAnalyzer.Severity(),
 		Code:     gobDiag.Code,
 		CodeHref: gobDiag.CodeHref,
 		Source:   DiagnosticSource(gobDiag.Source),

gopls/internal/server/diagnostics.go

@@ -11,6 +11,7 @@ import (
 	"os"
 	"path/filepath"
 	"runtime"
+	"slices"
 	"sort"
 	"strings"
 	"sync"
@@ -511,6 +512,24 @@ func (s *server) diagnose(ctx context.Context, snapshot *cache.Snapshot) (diagMa
 		// TODO(rfindley): here and above, we should avoid using the first result
 		// if err is non-nil (though as of today it's OK).
 		analysisDiags, err = golang.Analyze(ctx, snapshot, toAnalyze, s.progress)
+
+		// Filter out Hint diagnostics for closed files.
+		// VS Code already omits Hint diagnostics in the Problems tab, but other
+		// clients do not. This filter makes the visibility of Hints more similar
+		// across clients.
+		for uri, diags := range analysisDiags {
+			if !snapshot.IsOpen(uri) {
+				newDiags := slices.DeleteFunc(diags, func(diag *cache.Diagnostic) bool {
+					return diag.Severity == protocol.SeverityHint
+				})
+				if len(newDiags) == 0 {
+					delete(analysisDiags, uri)
+				} else {
+					analysisDiags[uri] = newDiags
+				}
+			}
+		}
+
 		if err != nil {
 			event.Error(ctx, "warning: analyzing package", err, append(snapshot.Labels(), label.Package.Of(keys.Join(moremaps.KeySlice(toDiagnose))))...)
 			return

gopls/internal/settings/analysis.go

@@ -88,12 +88,35 @@ func (a *Analyzer) EnabledByDefault() bool { return !a.nonDefault }
 // TODO(rfindley): revisit.
 func (a *Analyzer) ActionKinds() []protocol.CodeActionKind { return a.actionKinds }
 
-// Severity is the severity set for diagnostics reported by this
-// analyzer. If left unset it defaults to Warning.
+// Severity is the severity set for diagnostics reported by this analyzer.
+// The default severity is SeverityWarning.
 //
-// Note: diagnostics with severity protocol.SeverityHint do not show up in
-// the VS Code "problems" tab.
-func (a *Analyzer) Severity() protocol.DiagnosticSeverity { return a.severity }
+// While the LSP spec does not specify how severity should be used, here are
+// some guiding heuristics:
+//   - Error: for parse and type errors, which would stop the build.
+//   - Warning: for analyzer diagnostics reporting likely bugs.
+//   - Info: for analyzer diagnostics that do not indicate bugs, but may
+//     suggest inaccurate or superfluous code.
+//   - Hint: for analyzer diagnostics that do not indicate mistakes, but offer
+//     simplifications or modernizations. By their nature, hints should
+//     generally carry quick fixes.
+//
+// The difference between Info and Hint is particularly subtle. Importantly,
+// Hint diagnostics do not appear in the Problems tab in VS Code, so they are
+// less intrusive than Info diagnostics. The rule of thumb is this: use Info if
+// the diagnostic is not a bug, but the author probably didn't mean to write
+// the code that way. Use Hint if the diagnostic is not a bug and the author
+// indended to write the code that way, but there is a simpler or more modern
+// way to express the same logic. An 'unused' diagnostic is Info level, since
+// the author probably didn't mean to check in unreachable code. A 'modernize'
+// or 'deprecated' diagnostic is Hint level, since the author intended to write
+// the code that way, but now there is a better way.
+func (a *Analyzer) Severity() protocol.DiagnosticSeverity {
+	if a.severity == 0 {
+		return protocol.SeverityWarning
+	}
+	return a.severity
+}
 
 // Tags is extra tags (unnecessary, deprecated, etc) for diagnostics
 // reported by this analyzer.
@@ -109,6 +132,7 @@ func (a *Analyzer) String() string { return a.analyzer.String() }
 var DefaultAnalyzers = make(map[string]*Analyzer) // initialized below
 
 func init() {
+	// See [Analyzer.Severity] for guidance on setting analyzer severity below.
 	analyzers := []*Analyzer{
 		// The traditional vet suite:
 		{analyzer: appends.Analyzer},
@@ -190,10 +214,12 @@ func init() {
 		{analyzer: unusedparams.Analyzer, severity: protocol.SeverityInformation},
 		{analyzer: unusedfunc.Analyzer, severity: protocol.SeverityInformation},
 		{analyzer: unusedwrite.Analyzer, severity: protocol.SeverityInformation}, // uses go/ssa
-		{analyzer: modernize.Analyzer, severity: protocol.SeverityInformation},
+		{analyzer: modernize.Analyzer, severity: protocol.SeverityHint},
 
 		// type-error analyzers
 		// These analyzers enrich go/types errors with suggested fixes.
+		// Since they exist only to attach their fixes to type errors, their
+		// severity is irrelevant.
 		{analyzer: fillreturns.Analyzer},
 		{analyzer: nonewvars.Analyzer},
 		{analyzer: noresultvalues.Analyzer},

gopls/internal/test/integration/diagnostics/analysis_test.go

@@ -125,3 +125,32 @@ func main() {
 		}
 	})
 }
+
+func TestAnalysisFiltering(t *testing.T) {
+	// This test checks that hint level diagnostics are only surfaced for open
+	// files.
+
+	const src = `
+-- go.mod --
+module mod.com
+
+go 1.20
+
+-- a.go --
+package p
+
+var x interface{}
+
+-- b.go --
+package p
+
+var y interface{}
+`
+	Run(t, src, func(t *testing.T, env *Env) {
+		env.OpenFile("a.go")
+		env.AfterChange(
+			Diagnostics(ForFile("a.go"), WithMessage("replaced by any")),
+			NoDiagnostics(ForFile("b.go")),
+		)
+	})
+}