Alan Donovan (Gerrit)

unread,

Dec 16, 2025, 12:42:19 PM (14 hours ago) Dec 16

to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

Alan Donovan has uploaded the change for review

Commit message

go/analysis/passes/scannererr: report failure to check bufio.Scanner.Err

This CL adds an analyzer that checks for calls to bufio.NewScanner
followed by a Scanner.Scan loop that do not involve a call to
Scanner.Err.

Fixes golang/go#17747

Change-Id: I938c0d5b15cac7fc914899763c2c2715e5b38bf3

Change diff

diff --git a/go/analysis/passes/hostport/main.go b/go/analysis/passes/hostport/main.go
index 99f7a09..abac866 100644
--- a/go/analysis/passes/hostport/main.go
+++ b/go/analysis/passes/hostport/main.go
@@ -7,8 +7,8 @@
 package main
 
 import (
+	"golang.org/x/tools/go/analysis/passes/hostport"
 	"golang.org/x/tools/go/analysis/singlechecker"
-	"golang.org/x/tools/gopls/internal/analysis/hostport"
 )
 
 func main() { singlechecker.Main(hostport.Analyzer) }
diff --git a/go/analysis/passes/scannererr/main.go b/go/analysis/passes/scannererr/main.go
new file mode 100644
index 0000000..65a7909
--- /dev/null
+++ b/go/analysis/passes/scannererr/main.go
@@ -0,0 +1,14 @@
+// Copyright 2026 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+//go:build ignore
+
+package main
+
+import (
+	"golang.org/x/tools/go/analysis/passes/scannererr"
+	"golang.org/x/tools/go/analysis/singlechecker"
+)
+
+func main() { singlechecker.Main(scannererr.Analyzer) }
diff --git a/go/analysis/passes/scannererr/scannererr.go b/go/analysis/passes/scannererr/scannererr.go
new file mode 100644
index 0000000..5965152
--- /dev/null
+++ b/go/analysis/passes/scannererr/scannererr.go
@@ -0,0 +1,116 @@
+// Copyright 2025 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Package scannererr defines an analyzer for uses of bufio.Scanner
+// in which the user has forgotten to check Scanner.Err.
+package scannererr
+
+import (
+	"fmt"
+	"go/ast"
+	"go/token"
+	"go/types"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+	"golang.org/x/tools/go/ast/edge"
+	typeindexanalyzer "golang.org/x/tools/internal/analysis/typeindex"
+	"golang.org/x/tools/internal/astutil"
+	"golang.org/x/tools/internal/moreiters"
+	"golang.org/x/tools/internal/typesinternal/typeindex"
+)
+
+const doc = `scannererr: report failure to check bufio.Scanner.Err
+
+This analyzer reports uses of bufio.Scanner in which the result of
+NewScanner is assigned to a local variable that is then used in a loop
+that calls Scanner.Scan, but lacks a final check of Scanner.Err.
+
+For example:
+
+	sc := bufio.NewScanner(os.Stdin) // error: "bufio.Scanner sc is used in Scan loop without final check of sc.Err()"
+	for sc.Scan() {
+		line := sc.Text()
+		use(line)
+	}
+	/* ...no use of sc.Err()... */
+
+To avoid false positives, the analyzer is silent if the scanner is
+passed into or out of the function or assigned somewhere other than a
+local variable.
+`
+
+var Analyzer = &analysis.Analyzer{
+	Name:     "scannererr",
+	Doc:      doc,
+	URL:      "https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/scannererr",
+	Requires: []*analysis.Analyzer{inspect.Analyzer, typeindexanalyzer.Analyzer},
+	Run:      run,
+}
+
+func run(pass *analysis.Pass) (any, error) {
+	var (
+		index = pass.ResultOf[typeindexanalyzer.Analyzer].(*typeindex.Index)
+		info  = pass.TypesInfo
+	)
+
+next:
+	for curCall := range index.Calls(index.Object("bufio", "NewScanner")) {
+		// TODO(adonovan): factor this common pattern.
+		var lhs ast.Expr
+		switch ek, idx := curCall.ParentEdge(); ek {
+		case edge.ValueSpec_Values:
+			curName := curCall.Parent().ChildAt(edge.ValueSpec_Names, idx)
+			lhs = curName.Node().(*ast.Ident)
+		case edge.AssignStmt_Rhs:
+			curLhs := curCall.Parent().ChildAt(edge.AssignStmt_Lhs, idx)
+			lhs = curLhs.Node().(ast.Expr)
+		}
+		id, ok := lhs.(*ast.Ident)
+		if !ok {
+			continue
+		}
+		v, ok := info.Defs[id].(*types.Var)
+		if !ok {
+			continue
+		}
+
+		scanLoop := token.NoPos // position of sc.Scan() call within a loop
+
+		// Check all uses of the var "sc".
+		for curUse := range index.Uses(v) {
+			// If the var sc is used in a context other than sc.Method(...),
+			// assume conservatively that it may escape, and reject this candidate.
+			if !astutil.IsChildOf(curUse, edge.SelectorExpr_X) ||
+				!astutil.IsChildOf(curUse.Parent(), edge.CallExpr_Fun) {
+				continue next
+			}
+
+			methodName := curUse.Parent().Node().(*ast.SelectorExpr).Sel.Name
+			switch methodName {
+			case "Err":
+				// If the sc.Err method is called anywhere, reject this candidate.
+				continue next
+			case "Scan":
+				// The Scan call must be in a loop that intervenes the declaration of sc.
+				if curLoop, ok := moreiters.First(curUse.Enclosing((*ast.RangeStmt)(nil), (*ast.ForStmt)(nil))); ok {
+					if curLoop.Node().Pos() > v.Pos() {
+						scanLoop = curUse.Node().Pos()
+					}
+				}
+			}
+		}
+
+		if scanLoop.IsValid() {
+			pass.Report(analysis.Diagnostic{
+				Pos: curCall.Node().Pos(),
+				End: curCall.Node().End(),
+				Message: fmt.Sprintf("bufio.Scanner %q is used in Scan loop at line %d without final check of %s.Err()",
+					v.Name(), pass.Fset.Position(scanLoop).Line, v.Name()),
+			})
+		}
+	}
+
+	return nil, nil
+}
diff --git a/go/analysis/passes/scannererr/scannererr_test.go b/go/analysis/passes/scannererr/scannererr_test.go
new file mode 100644
index 0000000..f64255a
--- /dev/null
+++ b/go/analysis/passes/scannererr/scannererr_test.go
@@ -0,0 +1,17 @@
+// Copyright 2026 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package scannererr_test
+
+import (
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+	"golang.org/x/tools/go/analysis/passes/scannererr"
+)
+
+func Test(t *testing.T) {
+	testdata := analysistest.TestData()
+	analysistest.RunWithSuggestedFixes(t, testdata, scannererr.Analyzer, "a")
+}
diff --git a/go/analysis/passes/scannererr/testdata/src/a/a.go b/go/analysis/passes/scannererr/testdata/src/a/a.go
new file mode 100644
index 0000000..b0424f4
--- /dev/null
+++ b/go/analysis/passes/scannererr/testdata/src/a/a.go
@@ -0,0 +1,50 @@
+package a
+
+import "bufio"
+
+func missingErr() {
+	sc := bufio.NewScanner(nil) // want `bufio.Scanner "sc" is used in Scan loop at line 7 without final check of sc.Err\(\)`
+	for sc.Scan() {             // L7
+		println(sc.Text())
+	}
+}
+
+func missingErr2() {
+	sc := bufio.NewScanner(nil) // want `bufio.Scanner "sc" is used in Scan loop at line 15 without final check of sc.Err\(\)`
+	for {
+		if !sc.Scan() { // L15
+			break
+		}
+		println(sc.Text())
+	}
+}
+
+func nopeErrIsChecked() {
+	sc := bufio.NewScanner(nil)
+	for sc.Scan() {
+	}
+	if err := sc.Err(); err != nil {
+		panic(err)
+	}
+}
+
+func nopeErrIsCalled() {
+	sc := bufio.NewScanner(nil)
+	for sc.Scan() {
+		println(sc.Text())
+	}
+	_ = sc.Err() // ignore error
+}
+
+func nopeScannerIsParam(sc *bufio.Scanner) {
+	for sc.Scan() {
+	}
+}
+
+func nopeScannerEscapes(sc *bufio.Scanner) {
+	for sc.Scan() {
+	}
+	arbitraryEffects(sc)
+}
+
+func arbitraryEffects(any)

Change information

Files:

M go/analysis/passes/hostport/main.go
A go/analysis/passes/scannererr/main.go
A go/analysis/passes/scannererr/scannererr.go
A go/analysis/passes/scannererr/scannererr_test.go
A go/analysis/passes/scannererr/testdata/src/a/a.go

Change size: M

Delta: 5 files changed, 198 insertions(+), 1 deletion(-)

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

satisfied_requirement

open

diffy

Madeline Kalil (Gerrit)

unread,

Dec 16, 2025, 4:21:18 PM (10 hours ago) Dec 16

to Alan Donovan, goph...@pubsubhelper.golang.org, Go LUCI, golang-co...@googlegroups.com

Attention needed from Alan Donovan

Madeline Kalil added 2 comments

File go/analysis/passes/scannererr/scannererr.go

Line 93, Patchset 1 (Latest): // If the sc.Err method is called anywhere, reject this candidate.

Madeline Kalil . unresolved

What if sc.Err() is called inside a conditional statement so it is possibly not executed? I also understand if we don't want to handle that case to keep the vet check more conservative

Line 99, Patchset 1 (Latest): scanLoop = curUse.Node().Pos()

Madeline Kalil . unresolved

scanLoop will be the last use of sc.Scan() in the enclosing block. Does anyone use sc.Scan() in two different loops and should the analyzer consider this - maybe there's a call to error after the first use of Scan() but not after the second?

Open in Gerrit

Related details

Attention is currently required from:

Alan Donovan

Submit Requirements:

Code-Review

No-Unresolved-Comments

Review-Enforcement

TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

satisfied_requirement

open

diffy

Alan Donovan (Gerrit)

unread,

Dec 16, 2025, 4:31:43 PM (10 hours ago) Dec 16

to goph...@pubsubhelper.golang.org, Go LUCI, Madeline Kalil, golang-co...@googlegroups.com

Attention needed from Madeline Kalil

Alan Donovan added 2 comments

File go/analysis/passes/scannererr/scannererr.go

Line 93, Patchset 1 (Latest): // If the sc.Err method is called anywhere, reject this candidate.

Madeline Kalil . resolved

What if sc.Err() is called inside a conditional statement so it is possibly not executed? I also understand if we don't want to handle that case to keep the vet check more conservative

Alan Donovan

The Scanner API makes it easy to write apparently working code without ever thinking about errors; that's the problem we're trying to flag. So if the user demonstrates that they are aware of Err, it doesn't really matter how they use it.

Line 99, Patchset 1 (Latest): scanLoop = curUse.Node().Pos()

Madeline Kalil . resolved

scanLoop will be the last use of sc.Scan() in the enclosing block. Does anyone use sc.Scan() in two different loops and should the analyzer consider this - maybe there's a call to error after the first use of Scan() but not after the second?

Alan Donovan

Again, the relative location of Err and Scan isn't very important; we're just trying to find cases where NewScanner is called, and used at least once in a loop, and no error is checked. If there are two loops, or if they check Err within the loop, that's ok. If they don't call Scan at all, we have to admit we have no idea what's going on and be silent.

Open in Gerrit

Related details

Attention is currently required from:

Madeline Kalil

Submit Requirements:

Code-Review

No-Unresolved-Comments

Review-Enforcement
TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

satisfied_requirement

open

diffy

Alan Donovan (Gerrit)

unread,

Dec 16, 2025, 4:36:33 PM (10 hours ago) Dec 16

to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

Attention needed from Madeline Kalil

Alan Donovan uploaded new patchset

Alan Donovan uploaded patch set #2 to this change.

Following approvals got outdated and were removed:

TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI

Open in Gerrit

Related details

Attention is currently required from:

Madeline Kalil

Submit Requirements:

Code-Review
No-Unresolved-Comments
Review-Enforcement

TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

satisfied_requirement

open

diffy

Alan Donovan (Gerrit)

unread,

Dec 16, 2025, 4:36:36 PM (10 hours ago) Dec 16

to goph...@pubsubhelper.golang.org, Go LUCI, Madeline Kalil, golang-co...@googlegroups.com

Attention needed from Madeline Kalil

Alan Donovan added 1 comment

File go/analysis/passes/scannererr/scannererr.go

Line 99, Patchset 1: scanLoop = curUse.Node().Pos()

Madeline Kalil . resolved

scanLoop will be the last use of sc.Scan() in the enclosing block. Does anyone use sc.Scan() in two different loops and should the analyzer consider this - maybe there's a call to error after the first use of Scan() but not after the second?

Alan Donovan

Again, the relative location of Err and Scan isn't very important; we're just trying to find cases where NewScanner is called, and used at least once in a loop, and no error is checked. If there are two loops, or if they check Err within the loop, that's ok. If they don't call Scan at all, we have to admit we have no idea what's going on and be silent.

Alan Donovan

I've updated the documentation above to make this clearer.

Open in Gerrit

Related details

Attention is currently required from:

Madeline Kalil

Submit Requirements:

Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

satisfied_requirement

open

diffy

Madeline Kalil (Gerrit)

unread,

Dec 16, 2025, 4:43:05 PM (10 hours ago) Dec 16

to Alan Donovan, goph...@pubsubhelper.golang.org, Go LUCI, golang-co...@googlegroups.com

Attention needed from Alan Donovan

Madeline Kalil voted Code-Review+2

Code-Review

+2

Open in Gerrit

Related details

Attention is currently required from:

Alan Donovan

Submit Requirements:

Code-Review
No-Unresolved-Comments
Review-Enforcement

TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Reply all

Reply to author

Forward

[tools] go/analysis/passes/scannererr: report failure to check bufio.Scanner.Err

Alan Donovan (Gerrit)

Alan Donovan has uploaded the change for review

Commit message

Change diff

Change information

Related details

Madeline Kalil (Gerrit)

Madeline Kalil added 2 comments

Related details

Alan Donovan (Gerrit)

Alan Donovan added 2 comments

Related details

Alan Donovan (Gerrit)

Alan Donovan uploaded new patchset

Related details

Alan Donovan (Gerrit)

Alan Donovan added 1 comment

Related details

Madeline Kalil (Gerrit)

Madeline Kalil voted Code-Review+2

Related details