gopls/internal/golang: InlayHint: reveal ignored errors

This new inlay hint adds "// ignore error" after a call
statement that discards its error result.

+ doc, test, relnote

Fixes golang/go#73930

Change-Id: I051b68c80b24eeeecf393928d8942114c519f9fa
Reviewed-on: https://go-review.googlesource.com/c/tools/+/683835
Reviewed-by: Robert Findley <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: adonovan

gopls/doc/inlayHints.md

@@ -66,6 +66,25 @@ from `gopls`.
 
 **Disabled by default. Enable it by setting `"hints": {"functionTypeParameters": true}`.**
 
+## **ignoredError**
+
+`"ignoredError"` inlay hints for implicitly discarded errors:
+```go
+	f.Close() // ignore error
+```
+This check inserts an `// ignore error` hint following any
+statement that is a function call whose error result is
+implicitly ignored.
+
+To suppress the hint, write an actual comment containing
+"ignore error" following the call statement, or explictly
+assign the result to a blank variable. A handful of common
+functions such as `fmt.Println` are excluded from the
+check.
+
+
+**Disabled by default. Enable it by setting `"hints": {"ignoredError": true}`.**
+
 ## **parameterNames**
 
 `"parameterNames"` controls inlay hints for parameter names:

gopls/doc/release/v0.20.0.md

@@ -12,6 +12,28 @@ $description
 
 # Analysis features
 
+## `ignoredError` inlay hint
+
+The new `ignoredError` inlay hint helps catch mistakenly discarded
+errors. It inserts an `// ignore error` hint following any statement
+that is a function call whose error result is implicitly ignored. For
+example, this code:
+
+```go
+f.Close()
+```
+will appear as:
+```go
+f.Close() // ignore error
+```
+
+To suppress the hint, write an actual comment containing `ignore
+error` following the call statement, or explictly assign the result
+to a blank `_` variable. A handful of common functions such as
+`fmt.Println` are excluded from the check.
+
+Enable it using this configuration: `{"hints": {"ignoredError": true}}`.
+
 ## `unusedfunc` reports unused `type`, `var`, and `const` declarations too
 
 <!-- golang/go#40862 -->

gopls/internal/doc/api.json

@@ -1878,6 +1878,12 @@
 							"Default": "false",
 							"Status": ""
 						},
+						{
+							"Name": "\"ignoredError\"",
+							"Doc": "`\"ignoredError\"` inlay hints for implicitly discarded errors:\n```go\n\tf.Close() // ignore error\n```\nThis check inserts an `// ignore error` hint following any\nstatement that is a function call whose error result is\nimplicitly ignored.\n\nTo suppress the hint, write an actual comment containing\n\"ignore error\" following the call statement, or explictly\nassign the result to a blank variable. A handful of common\nfunctions such as `fmt.Println` are excluded from the\ncheck.\n",
+							"Default": "false",
+							"Status": ""
+						},
 						{
 							"Name": "\"parameterNames\"",
 							"Doc": "`\"parameterNames\"` controls inlay hints for parameter names:\n```go\n\tparseInt(/* str: */ \"123\", /* radix: */ 8)\n```\n",
@@ -3452,6 +3458,12 @@
 			"Default": false,
 			"Status": ""
 		},
+		{
+			"Name": "ignoredError",
+			"Doc": "`\"ignoredError\"` inlay hints for implicitly discarded errors:\n```go\n\tf.Close() // ignore error\n```\nThis check inserts an `// ignore error` hint following any\nstatement that is a function call whose error result is\nimplicitly ignored.\n\nTo suppress the hint, write an actual comment containing\n\"ignore error\" following the call statement, or explictly\nassign the result to a blank variable. A handful of common\nfunctions such as `fmt.Println` are excluded from the\ncheck.\n",
+			"Default": false,
+			"Status": ""
+		},
 		{
 			"Name": "parameterNames",
 			"Doc": "`\"parameterNames\"` controls inlay hints for parameter names:\n```go\n\tparseInt(/* str: */ \"123\", /* radix: */ 8)\n```\n",

gopls/internal/golang/inlay_hint.go

@@ -5,20 +5,24 @@
 package golang
 
 import (
+	"cmp"
 	"context"
 	"fmt"
 	"go/ast"
 	"go/constant"
 	"go/token"
 	"go/types"
+	"slices"
 	"strings"
 
 	"golang.org/x/tools/go/ast/inspector"
+	"golang.org/x/tools/go/types/typeutil"
 	"golang.org/x/tools/gopls/internal/cache"
 	"golang.org/x/tools/gopls/internal/cache/parsego"
 	"golang.org/x/tools/gopls/internal/file"
 	"golang.org/x/tools/gopls/internal/protocol"
 	"golang.org/x/tools/gopls/internal/settings"
+	"golang.org/x/tools/internal/analysisinternal"
 	"golang.org/x/tools/internal/event"
 	"golang.org/x/tools/internal/typeparams"
 	"golang.org/x/tools/internal/typesinternal"
@@ -84,6 +88,7 @@ var allInlayHints = map[settings.InlayHint]inlayHintFunc{
 	settings.CompositeLiteralTypes:      compositeLiteralTypes,
 	settings.CompositeLiteralFieldNames: compositeLiteralFields,
 	settings.FunctionTypeParameters:     funcTypeParams,
+	settings.IgnoredError:               ignoredError,
 }
 
 func parameterNames(info *types.Info, pgf *parsego.File, qual types.Qualifier, cur inspector.Cursor, add func(protocol.InlayHint)) {
@@ -126,14 +131,69 @@ func parameterNames(info *types.Info, pgf *parsego.File, qual types.Qualifier, c
 			}
 			add(protocol.InlayHint{
 				Position:     start,
-				Label:        buildLabel(label + ":"),
+				Label:        labelPart(label + ":"),
 				Kind:         protocol.Parameter,
 				PaddingRight: true,
 			})
 		}
 	}
 }
 
+func ignoredError(info *types.Info, pgf *parsego.File, qual types.Qualifier, cur inspector.Cursor, add func(protocol.InlayHint)) {
+outer:
+	for curCall := range cur.Preorder((*ast.ExprStmt)(nil)) {
+		stmt := curCall.Node().(*ast.ExprStmt)
+		call, ok := stmt.X.(*ast.CallExpr)
+		if !ok {
+			continue // not a call stmt
+		}
+
+		// Check that type of result (or last component) is error.
+		tv, ok := info.Types[call]
+		if !ok {
+			continue // no type info
+		}
+		t := tv.Type
+		if res, ok := t.(*types.Tuple); ok && res.Len() > 1 {
+			t = res.At(res.Len() - 1).Type()
+		}
+		if !types.Identical(t, errorType) {
+			continue
+		}
+
+		// Suppress some common false positives.
+		obj := typeutil.Callee(info, call)
+		if analysisinternal.IsFunctionNamed(obj, "fmt", "Print", "Printf", "Println", "Fprint", "Fprintf", "Fprintln") ||
+			analysisinternal.IsMethodNamed(obj, "bytes", "Buffer", "Write", "WriteByte", "WriteRune", "WriteString") ||
+			analysisinternal.IsMethodNamed(obj, "strings", "Builder", "Write", "WriteByte", "WriteRune", "WriteString") {
+			continue
+		}
+
+		// Suppress if closely following comment contains "// ignore error".
+		comments := pgf.File.Comments
+		compare := func(cg *ast.CommentGroup, pos token.Pos) int {
+			return cmp.Compare(cg.Pos(), pos)
+		}
+		i, _ := slices.BinarySearchFunc(comments, stmt.End(), compare)
+		if i >= 0 && i < len(comments) {
+			cg := comments[i]
+			if cg.Pos() < stmt.End()+3 && strings.Contains(cg.Text(), "ignore error") {
+				continue outer // suppress
+			}
+		}
+
+		// Provide a hint.
+		pos, err := pgf.PosPosition(stmt.End())
+		if err != nil {
+			continue
+		}
+		add(protocol.InlayHint{
+			Position: pos,
+			Label:    labelPart(" // ignore error"),
+		})
+	}
+}
+
 func funcTypeParams(info *types.Info, pgf *parsego.File, qual types.Qualifier, cur inspector.Cursor, add func(protocol.InlayHint)) {
 	for curCall := range cur.Preorder((*ast.CallExpr)(nil)) {
 		call := curCall.Node().(*ast.CallExpr)
@@ -158,7 +218,7 @@ func funcTypeParams(info *types.Info, pgf *parsego.File, qual types.Qualifier, c
 		}
 		add(protocol.InlayHint{
 			Position: start,
-			Label:    buildLabel("[" + strings.Join(args, ", ") + "]"),
+			Label:    labelPart("[" + strings.Join(args, ", ") + "]"),
 			Kind:     protocol.Type,
 		})
 	}
@@ -215,7 +275,7 @@ func variableType(info *types.Info, pgf *parsego.File, qual types.Qualifier, e a
 	}
 	add(protocol.InlayHint{
 		Position:    end,
-		Label:       buildLabel(types.TypeString(typ, qual)),
+		Label:       labelPart(types.TypeString(typ, qual)),
 		Kind:        protocol.Type,
 		PaddingLeft: true,
 	})
@@ -265,7 +325,7 @@ func constantValues(info *types.Info, pgf *parsego.File, qual types.Qualifier, c
 			}
 			add(protocol.InlayHint{
 				Position:    end,
-				Label:       buildLabel("= " + strings.Join(values, ", ")),
+				Label:       labelPart("= " + strings.Join(values, ", ")),
 				PaddingLeft: true,
 			})
 		}
@@ -301,7 +361,7 @@ func compositeLiteralFields(info *types.Info, pgf *parsego.File, qual types.Qual
 				}
 				hints = append(hints, protocol.InlayHint{
 					Position:     start,
-					Label:        buildLabel(strct.Field(i).Name() + ":"),
+					Label:        labelPart(strct.Field(i).Name() + ":"),
 					Kind:         protocol.Parameter,
 					PaddingRight: true,
 				})
@@ -342,19 +402,16 @@ func compositeLiteralTypes(info *types.Info, pgf *parsego.File, qual types.Quali
 		}
 		add(protocol.InlayHint{
 			Position: start,
-			Label:    buildLabel(fmt.Sprintf("%s%s", prefix, types.TypeString(typ, qual))),
+			Label:    labelPart(fmt.Sprintf("%s%s", prefix, types.TypeString(typ, qual))),
 			Kind:     protocol.Type,
 		})
 	}
 }
 
-func buildLabel(s string) []protocol.InlayHintLabelPart {
+func labelPart(s string) []protocol.InlayHintLabelPart {
 	const maxLabelLength = 28
-	label := protocol.InlayHintLabelPart{
-		Value: s,
-	}
 	if len(s) > maxLabelLength+len("...") {
-		label.Value = s[:maxLabelLength] + "..."
+		s = s[:maxLabelLength] + "..."
 	}
-	return []protocol.InlayHintLabelPart{label}
+	return []protocol.InlayHintLabelPart{{Value: s}}
 }

gopls/internal/settings/default.go

@@ -103,7 +103,9 @@ func DefaultOptions(overrides ...func(*Options)) *Options {
 						DiagnosticsTrigger:        DiagnosticsOnEdit,
 						AnalysisProgressReporting: true,
 					},
-					InlayHintOptions: InlayHintOptions{},
+					InlayHintOptions: InlayHintOptions{
+						Hints: map[InlayHint]bool{},
+					},
 					DocumentationOptions: DocumentationOptions{
 						HoverKind:    FullDocumentation,
 						LinkTarget:   "pkg.go.dev",
@@ -150,5 +152,6 @@ func DefaultOptions(overrides ...func(*Options)) *Options {
 			override(options)
 		}
 	}
+
 	return options
 }

gopls/internal/settings/settings.go

@@ -598,6 +598,21 @@ const (
 	// 	myFoo/*[int, string]*/(1, "hello")
 	// ```
 	FunctionTypeParameters InlayHint = "functionTypeParameters"
+
+	// IgnoredError inlay hints for implicitly discarded errors:
+	// ```go
+	// 	f.Close() // ignore error
+	// ```
+	// This check inserts an `// ignore error` hint following any
+	// statement that is a function call whose error result is
+	// implicitly ignored.
+	//
+	// To suppress the hint, write an actual comment containing
+	// "ignore error" following the call statement, or explictly
+	// assign the result to a blank variable. A handful of common
+	// functions such as `fmt.Println` are excluded from the
+	// check.
+	IgnoredError InlayHint = "ignoredError"
 )
 
 type NavigationOptions struct {

gopls/internal/test/marker/testdata/inlayhints/ignored-error.txt

@@ -0,0 +1,61 @@
+Test of "ignore error" inlay hint (#73930).
+
+- f.Close() generates a hint, except when followed by
+  an "// ignore error" comment, or in a "_ = f.Close()" stmt.
+- fmt.Println() is exempted.
+
+-- settings.json --
+{"hints": {"ignoredError": true}}
+
+-- p/p.go --
+package p //@inlayhints(out)
+
+import ( "os"; "fmt" )
+
+func _(f *os.File) {
+	f.WriteString("hello")
+	f.Close()
+}
+
+func _(f *os.File) {
+	f.Close() // irrelevant comment
+}
+
+func _(f *os.File) {
+	f.Close() // ignore error
+}
+
+func _(f *os.File) {
+	_ = f.Close()
+}
+
+func _() {
+	fmt.Println()
+}
+
+-- @out --
+package p //@inlayhints(out)
+
+import ( "os"; "fmt" )
+
+func _(f *os.File) {
+	f.WriteString("hello")< // ignore error>
+	f.Close()< // ignore error>
+}
+
+func _(f *os.File) {
+	f.Close()< // ignore error> // irrelevant comment
+}
+
+func _(f *os.File) {
+	f.Close() // ignore error
+}
+
+func _(f *os.File) {
+	_ = f.Close()
+}
+
+func _() {
+	fmt.Println()
+}
+