gopls/internal/cache: fix bug in toGobDiagnostic(Related)

Analyzers report Diagnostics, SuggestedFixes, and RelatedInformation
containing token.Pos positions that need to be mapped to
protocol.Location. Unlike the first two kinds, RelatedInformation
positions needn't be in the current package, so the existing
assertion was overstrict.

This CL relaxes the assertion and attempts to map positions
in dependencies to protocol.Location form, though it can do
so at best heuristically since Mappers are not generally
available.

Also, extract posToLocation to a top-level function.

+ test

Fixes golang/go#70791

Change-Id: Ibec2804b1b4b0574edd53b8ae2e0484630f01d6e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/667517
Auto-Submit: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Jonathan Amsterdam <[email protected]>

Author: adonovan

gopls/internal/cache/analysis.go

@@ -44,6 +44,7 @@ import (
 	"golang.org/x/tools/gopls/internal/util/frob"
 	"golang.org/x/tools/gopls/internal/util/moremaps"
 	"golang.org/x/tools/gopls/internal/util/persistent"
+	"golang.org/x/tools/gopls/internal/util/safetoken"
 	"golang.org/x/tools/internal/analysisinternal"
 	"golang.org/x/tools/internal/event"
 	"golang.org/x/tools/internal/facts"
@@ -1019,93 +1020,6 @@ func (act *action) exec(ctx context.Context) (any, *actionSummary, error) {
 		factFilter[reflect.TypeOf(f)] = true
 	}
 
-	// posToLocation converts from token.Pos to protocol form.
-	posToLocation := func(start, end token.Pos) (protocol.Location, error) {
-		tokFile := apkg.pkg.FileSet().File(start)
-
-		// Find existing mapper by file name.
-		// (Don't require an exact token.File match
-		// as the analyzer may have re-parsed the file.)
-		var (
-			mapper *protocol.Mapper
-			fixed  bool
-		)
-		for _, p := range apkg.pkg.CompiledGoFiles() {
-			if p.Tok.Name() == tokFile.Name() {
-				mapper = p.Mapper
-				fixed = p.Fixed() // suppress some assertions after parser recovery
-				break
-			}
-		}
-		if mapper == nil {
-			// The start position was not among the package's parsed
-			// Go files, indicating that the analyzer added new files
-			// to the FileSet.
-			//
-			// For example, the cgocall analyzer re-parses and
-			// type-checks some of the files in a special environment;
-			// and asmdecl and other low-level runtime analyzers call
-			// ReadFile to parse non-Go files.
-			// (This is a supported feature, documented at go/analysis.)
-			//
-			// In principle these files could be:
-			//
-			// - OtherFiles (non-Go files such as asm).
-			//   However, we set Pass.OtherFiles=[] because
-			//   gopls won't service "diagnose" requests
-			//   for non-Go files, so there's no point
-			//   reporting diagnostics in them.
-			//
-			// - IgnoredFiles (files tagged for other configs).
-			//   However, we set Pass.IgnoredFiles=[] because,
-			//   in most cases, zero-config gopls should create
-			//   another view that covers these files.
-			//
-			// - Referents of //line directives, as in cgo packages.
-			//   The file names in this case are not known a priori.
-			//   gopls generally tries to avoid honoring line directives,
-			//   but analyzers such as cgocall may honor them.
-			//
-			// In short, it's unclear how this can be reached
-			// other than due to an analyzer bug.
-			return protocol.Location{}, bug.Errorf("diagnostic location is not among files of package: %s", tokFile.Name())
-		}
-		// Inv: mapper != nil
-
-		if end == token.NoPos {
-			end = start
-		}
-
-		// debugging #64547
-		fileStart := token.Pos(tokFile.Base())
-		fileEnd := fileStart + token.Pos(tokFile.Size())
-		if start < fileStart {
-			if !fixed {
-				bug.Reportf("start < start of file")
-			}
-			start = fileStart
-		}
-		if end < start {
-			// This can happen if End is zero (#66683)
-			// or a small positive displacement from zero
-			// due to recursive Node.End() computation.
-			// This usually arises from poor parser recovery
-			// of an incomplete term at EOF.
-			if !fixed {
-				bug.Reportf("end < start of file")
-			}
-			end = fileEnd
-		}
-		if end > fileEnd+1 {
-			if !fixed {
-				bug.Reportf("end > end of file + 1")
-			}
-			end = fileEnd
-		}
-
-		return mapper.PosLocation(tokFile, start, end)
-	}
-
 	// Now run the (pkg, analyzer) action.
 	var diagnostics []gobDiagnostic
 
@@ -1130,7 +1044,7 @@ func (act *action) exec(ctx context.Context) (any, *actionSummary, error) {
 				bug.Reportf("invalid SuggestedFixes: %v", err)
 				d.SuggestedFixes = nil
 			}
-			diagnostic, err := toGobDiagnostic(posToLocation, analyzer, d)
+			diagnostic, err := toGobDiagnostic(apkg.pkg, analyzer, d)
 			if err != nil {
 				// Don't bug.Report here: these errors all originate in
 				// posToLocation, and we can more accurately discriminate
@@ -1322,12 +1236,12 @@ type gobTextEdit struct {
 
 // toGobDiagnostic converts an analysis.Diagnosic to a serializable gobDiagnostic,
 // which requires expanding token.Pos positions into protocol.Location form.
-func toGobDiagnostic(posToLocation func(start, end token.Pos) (protocol.Location, error), a *analysis.Analyzer, diag analysis.Diagnostic) (gobDiagnostic, error) {
+func toGobDiagnostic(pkg *Package, a *analysis.Analyzer, diag analysis.Diagnostic) (gobDiagnostic, error) {
 	var fixes []gobSuggestedFix
 	for _, fix := range diag.SuggestedFixes {
 		var gobEdits []gobTextEdit
 		for _, textEdit := range fix.TextEdits {
-			loc, err := posToLocation(textEdit.Pos, textEdit.End)
+			loc, err := diagnosticPosToLocation(pkg, false, textEdit.Pos, textEdit.End)
 			if err != nil {
 				return gobDiagnostic{}, fmt.Errorf("in SuggestedFixes: %w", err)
 			}
@@ -1344,7 +1258,10 @@ func toGobDiagnostic(posToLocation func(start, end token.Pos) (protocol.Location
 
 	var related []gobRelatedInformation
 	for _, r := range diag.Related {
-		loc, err := posToLocation(r.Pos, r.End)
+		// The position of RelatedInformation may be
+		// within another (dependency) package.
+		const allowDeps = true
+		loc, err := diagnosticPosToLocation(pkg, allowDeps, r.Pos, r.End)
 		if err != nil {
 			return gobDiagnostic{}, fmt.Errorf("in Related: %w", err)
 		}
@@ -1354,7 +1271,7 @@ func toGobDiagnostic(posToLocation func(start, end token.Pos) (protocol.Location
 		})
 	}
 
-	loc, err := posToLocation(diag.Pos, diag.End)
+	loc, err := diagnosticPosToLocation(pkg, false, diag.Pos, diag.End)
 	if err != nil {
 		return gobDiagnostic{}, err
 	}
@@ -1382,6 +1299,126 @@ func toGobDiagnostic(posToLocation func(start, end token.Pos) (protocol.Location
 	}, nil
 }
 
+// diagnosticPosToLocation converts from token.Pos to protocol form, in the
+// context of the specified package and, optionally, its dependencies.
+func diagnosticPosToLocation(pkg *Package, allowDeps bool, start, end token.Pos) (protocol.Location, error) {
+	if end == token.NoPos {
+		end = start
+	}
+
+	fset := pkg.FileSet()
+	tokFile := fset.File(start)
+
+	// Find existing mapper by file name.
+	// (Don't require an exact token.File match
+	// as the analyzer may have re-parsed the file.)
+	var (
+		mapper *protocol.Mapper
+		fixed  bool
+	)
+	for _, p := range pkg.CompiledGoFiles() {
+		if p.Tok.Name() == tokFile.Name() {
+			mapper = p.Mapper
+			fixed = p.Fixed() // suppress some assertions after parser recovery
+			break
+		}
+	}
+	// TODO(adonovan): search pkg.AsmFiles too; see #71754.
+	if mapper != nil {
+		// debugging #64547
+		fileStart := token.Pos(tokFile.Base())
+		fileEnd := fileStart + token.Pos(tokFile.Size())
+		if start < fileStart {
+			if !fixed {
+				bug.Reportf("start < start of file")
+			}
+			start = fileStart
+		}
+		if end < start {
+			// This can happen if End is zero (#66683)
+			// or a small positive displacement from zero
+			// due to recursive Node.End() computation.
+			// This usually arises from poor parser recovery
+			// of an incomplete term at EOF.
+			if !fixed {
+				bug.Reportf("end < start of file")
+			}
+			end = fileEnd
+		}
+		if end > fileEnd+1 {
+			if !fixed {
+				bug.Reportf("end > end of file + 1")
+			}
+			end = fileEnd
+		}
+
+		return mapper.PosLocation(tokFile, start, end)
+	}
+
+	// Inv: the positions are not within this package.
+
+	if allowDeps {
+		// Positions in Diagnostic.RelatedInformation may belong to a
+		// dependency package. We cannot accurately map them to
+		// protocol.Location coordinates without a Mapper for the
+		// relevant file, but none exists if the file was loaded from
+		// export data, and we have no means (Snapshot) of loading it.
+		//
+		// So, fall back to approximate conversion to UTF-16:
+		// for non-ASCII text, the column numbers may be wrong.
+		var (
+			startPosn = safetoken.StartPosition(fset, start)
+			endPosn   = safetoken.EndPosition(fset, end)
+		)
+		return protocol.Location{
+			URI: protocol.URIFromPath(startPosn.Filename),
+			Range: protocol.Range{
+				Start: protocol.Position{
+					Line:      uint32(startPosn.Line - 1),
+					Character: uint32(startPosn.Column - 1),
+				},
+				End: protocol.Position{
+					Line:      uint32(endPosn.Line - 1),
+					Character: uint32(endPosn.Column - 1),
+				},
+			},
+		}, nil
+	}
+
+	// The start position was not among the package's parsed
+	// Go files, indicating that the analyzer added new files
+	// to the FileSet.
+	//
+	// For example, the cgocall analyzer re-parses and
+	// type-checks some of the files in a special environment;
+	// and asmdecl and other low-level runtime analyzers call
+	// ReadFile to parse non-Go files.
+	// (This is a supported feature, documented at go/analysis.)
+	//
+	// In principle these files could be:
+	//
+	// - OtherFiles (non-Go files such as asm).
+	//   However, we set Pass.OtherFiles=[] because
+	//   gopls won't service "diagnose" requests
+	//   for non-Go files, so there's no point
+	//   reporting diagnostics in them.
+	//
+	// - IgnoredFiles (files tagged for other configs).
+	//   However, we set Pass.IgnoredFiles=[] because,
+	//   in most cases, zero-config gopls should create
+	//   another view that covers these files.
+	//
+	// - Referents of //line directives, as in cgo packages.
+	//   The file names in this case are not known a priori.
+	//   gopls generally tries to avoid honoring line directives,
+	//   but analyzers such as cgocall may honor them.
+	//
+	// In short, it's unclear how this can be reached
+	// other than due to an analyzer bug.
+
+	return protocol.Location{}, bug.Errorf("diagnostic location is not among files of package: %s", tokFile.Name())
+}
+
 // effectiveURL computes the effective URL of diag,
 // using the algorithm specified at Diagnostic.URL.
 func effectiveURL(a *analysis.Analyzer, diag analysis.Diagnostic) string {

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

@@ -0,0 +1,29 @@
+In addition to the Diagnostic, the SA4023 analyzer reports a
+RelatedInformation at the position of b.B, in an another package.
+Since this is in a dependency package, we cannot resolve to
+protocol.Location coordinates. This used to trigger an assertion, but
+now we resolve the location approximately.
+
+This is a regression test for #70791.
+
+-- settings.json --
+{"analyses": {"SA4023": true}}
+
+-- go.mod --
+module example.com
+go 1.18
+
+-- a/a.go --
+package a
+
+import "example.com/b"
+
+var _ = b.B() == nil //@ diag("b.B", re"comparison is never true")
+
+-- b/b.go --
+package b
+
+func B() any { return (*int)(nil) }
+
+
+