gopls/internal/lsp/cache: fix bug reports from toGobDiagnostics

Fix two cases that could lead to the bug reports related to position
conversion reported in golang/go#64547:
 1. The lostcancel analyzer reports diagnostics for a synthetic range
    that could overflow the file.
 2. The cgocall analyzer, which runs despite errors, could report
    diagnostics for invalid "fixed" positions.

Issue (1) was easy enough to prove with a test, though (2) is admittedly
just a hypothesis. Nevertheless, we should close the telemetry-derived
issue and see if it recurs.

Fixes golang/go#64547

Change-Id: Ie69bdae474abdd25235d856fc0ae0bfaf7e214ed
Reviewed-on: https://go-review.googlesource.com/c/tools/+/556815
Reviewed-by: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: findleyr

go/analysis/passes/lostcancel/lostcancel.go

@@ -172,7 +172,18 @@ func runFunc(pass *analysis.Pass, node ast.Node) {
 		if ret := lostCancelPath(pass, g, v, stmt, sig); ret != nil {
 			lineno := pass.Fset.Position(stmt.Pos()).Line
 			pass.ReportRangef(stmt, "the %s function is not used on all paths (possible context leak)", v.Name())
-			pass.ReportRangef(ret, "this return statement may be reached without using the %s var defined on line %d", v.Name(), lineno)
+
+			pos, end := ret.Pos(), ret.End()
+			// golang/go#64547: cfg.Block.Return may return a synthetic
+			// ReturnStmt that overflows the file.
+			if pass.Fset.File(pos) != pass.Fset.File(end) {
+				end = pos
+			}
+			pass.Report(analysis.Diagnostic{
+				Pos:     pos,
+				End:     end,
+				Message: fmt.Sprintf("this return statement may be reached without using the %s var defined on line %d", v.Name(), lineno),
+			})
 		}
 	}
 }

go/cfg/cfg.go

@@ -113,7 +113,11 @@ func (b *Block) String() string {
 	return fmt.Sprintf("block %d (%s)", b.Index, b.comment)
 }
 
-// Return returns the return statement at the end of this block if present, nil otherwise.
+// Return returns the return statement at the end of this block if present, nil
+// otherwise.
+//
+// When control falls off the end of the function, the ReturnStmt is synthetic
+// and its [ast.Node.End] position may be beyond the end of the file.
 func (b *Block) Return() (ret *ast.ReturnStmt) {
 	if len(b.Nodes) > 0 {
 		ret, _ = b.Nodes[len(b.Nodes)-1].(*ast.ReturnStmt)

gopls/internal/lsp/cache/analysis.go

@@ -1269,10 +1269,21 @@ func (act *action) exec() (interface{}, *actionSummary, error) {
 		factFilter[reflect.TypeOf(f)] = true
 	}
 
+	// If the package contains "fixed" files, it's not necessarily an error if we
+	// can't convert positions.
+	hasFixedFiles := false
+	for _, p := range pkg.parsed {
+		if p.Fixed() {
+			hasFixedFiles = true
+			break
+		}
+	}
+
 	// posToLocation converts from token.Pos to protocol form.
 	// TODO(adonovan): improve error messages.
 	posToLocation := func(start, end token.Pos) (protocol.Location, error) {
 		tokFile := pkg.fset.File(start)
+
 		for _, p := range pkg.parsed {
 			if p.Tok == tokFile {
 				if end == token.NoPos {
@@ -1281,8 +1292,11 @@ func (act *action) exec() (interface{}, *actionSummary, error) {
 				return p.PosLocation(start, end)
 			}
 		}
-		return protocol.Location{},
-			bug.Errorf("internal error: token.Pos not within package")
+		errorf := bug.Errorf
+		if hasFixedFiles {
+			errorf = fmt.Errorf
+		}
+		return protocol.Location{}, errorf("token.Pos not within package")
 	}
 
 	// Now run the (pkg, analyzer) action.
@@ -1299,7 +1313,9 @@ func (act *action) exec() (interface{}, *actionSummary, error) {
 		Report: func(d analysis.Diagnostic) {
 			diagnostic, err := toGobDiagnostic(posToLocation, analyzer, d)
 			if err != nil {
-				bug.Reportf("internal error converting diagnostic from analyzer %q: %v", analyzer.Name, err)
+				if !hasFixedFiles {
+					bug.Reportf("internal error converting diagnostic from analyzer %q: %v", analyzer.Name, err)
+				}
 				return
 			}
 			diagnostics = append(diagnostics, diagnostic)

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

@@ -0,0 +1,14 @@
+This test checks the fix for golang/go#64547: the lostcancel analyzer reports
+diagnostics that overflow the file.
+
+-- p.go --
+package p
+
+import "context"
+
+func _() {
+	_, cancel := context.WithCancel(context.Background()) //@diag("_, cancel", re"not used on all paths")
+	if false {
+		cancel()
+	}
+} //@diag("}", re"may be reached without using the cancel")