From 58bf46fa425631c94a74c16bec8c6ada904ccf30 Mon Sep 17 00:00:00 2001 From: Tom van der Woerdt Date: Thu, 11 Jun 2026 19:06:12 -0400 Subject: [PATCH 1/3] Match trusted functions called via dot-imports Trusted function matching previously required the call to be a selector expression (`assert.Error(...)`), so functions of dot-imported packages (`Error(...)` with `import . "..."`) were never matched. Resolve the called identifier for both call forms via `asthelper.FuncIdentFromCallExpr` in both `matchCall` and `generateComparators`. Soundness is unchanged: the identifier must still resolve to a function object whose package path matches the trusted signature, so local functions and function values never match. --- hook/hook.go | 17 +++++- hook/split_blocks_on.go | 7 ++- .../src/go.uber.org/trustedfunc/dotimport.go | 55 +++++++++++++++++++ 3 files changed, 73 insertions(+), 6 deletions(-) create mode 100644 testdata/src/go.uber.org/trustedfunc/dotimport.go diff --git a/hook/hook.go b/hook/hook.go index 1bf2ee29..62e32bfe 100644 --- a/hook/hook.go +++ b/hook/hook.go @@ -25,6 +25,7 @@ import ( "regexp" "go.uber.org/nilaway/util/analysishelper" + "go.uber.org/nilaway/util/asthelper" "go.uber.org/nilaway/util/typeshelper" ) @@ -54,18 +55,24 @@ type trustedSig struct { // - _func: match enclosing "". E.g., for `assert.Error(err)`, path = github.com/stretchr/testify/assert // - _method: match ".". E.g., for `u.Require().Error(err)`, path = github.com/stretchr/testify/require.Assertions // +// Functions of dot-imported packages are also matched: for `Error(err)` with +// `import . "github.com/stretchr/testify/assert"`, the called identifier still resolves to the +// function object in the imported package, so the path matching is identical. Function values +// (e.g., `f := assert.Error; f(t, err)`) resolve to variables rather than functions and are +// hence never matched. +// // Trusted package-level variables (kind _var) are matched separately via matchSel, since they are // read as bare selectors rather than calls. func (t *trustedSig) matchCall(pass *analysishelper.EnhancedPass, call *ast.CallExpr) bool { if t.kind != _func && t.kind != _method { return false } - sel, ok := call.Fun.(*ast.SelectorExpr) - if !ok || !t.nameRegex.MatchString(sel.Sel.Name) { + ident := asthelper.FuncIdentFromCallExpr(call) + if ident == nil || !token.IsExported(ident.Name) || !t.nameRegex.MatchString(ident.Name) { return false } - funcObj, ok := pass.TypesInfo.ObjectOf(sel.Sel).(*types.Func) + funcObj, ok := pass.TypesInfo.ObjectOf(ident).(*types.Func) if !ok || funcObj.Pkg() == nil { return false } @@ -99,6 +106,10 @@ func (t *trustedSig) matchCall(pass *analysishelper.EnhancedPass, call *ast.Call // This is intentionally independent of how the variable is later used: a read like `os.Stdout`, // `os.Stdout.Write(...)` (as a method receiver), or `os.Args[0]` (as an index operand) all parse the // bare selector as a producer, so all are covered here without involving matchCall. +// +// Unlike matchCall, dot-imported variable reads (a bare `Stdout` under `import . "os"`) are +// intentionally out of scope: they parse as bare identifiers, which are routed elsewhere by the +// assertion tree and never reach this matcher. func (t *trustedSig) matchSel(pass *analysishelper.EnhancedPass, sel *ast.SelectorExpr) bool { if t.kind != _var || !t.nameRegex.MatchString(sel.Sel.Name) { return false diff --git a/hook/split_blocks_on.go b/hook/split_blocks_on.go index 27dc61e4..67418f82 100644 --- a/hook/split_blocks_on.go +++ b/hook/split_blocks_on.go @@ -22,6 +22,7 @@ import ( "regexp" "go.uber.org/nilaway/util/analysishelper" + "go.uber.org/nilaway/util/asthelper" "go.uber.org/nilaway/util/typeshelper" ) @@ -196,11 +197,11 @@ var requireZeroComparators splitBlockOnAction = func(pass *analysishelper.Enhanc // generateComparators generates comparators based on the semantics of the function. func generateComparators(call *ast.CallExpr, actualExpr ast.Expr, actualExprIndex int, expectedVal expectedValue) ast.Expr { - sel, ok := call.Fun.(*ast.SelectorExpr) - if !ok { + ident := asthelper.FuncIdentFromCallExpr(call) + if ident == nil { return nil } - funcName := sel.Sel.Name + funcName := ident.Name // Now, based on the semantics of the function, we can create artificial nonnil checks for // the following cases. diff --git a/testdata/src/go.uber.org/trustedfunc/dotimport.go b/testdata/src/go.uber.org/trustedfunc/dotimport.go new file mode 100644 index 00000000..4b69597f --- /dev/null +++ b/testdata/src/go.uber.org/trustedfunc/dotimport.go @@ -0,0 +1,55 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package trustedfunc + +import ( + "testing" + + . "stubs/github.com/stretchr/testify/require" +) + +// testDotImport tests that trusted functions of a dot-imported package (called as bare +// identifiers rather than selector expressions) are matched and narrow just like qualified calls. +// +// nilable(x) +func testDotImport(t *testing.T, x any) any { + switch 0 { + case 1: + y, err := errs() + NoError(t, err) + return y + case 2: + _, err := errs() + Error(t, err) + takesNonnil(err) + case 3: + True(t, x != nil) + return x + case 4: + // `Equal` routes through generateComparators, which extracts the called name; this + // exercises that path for bare-identifier calls. + y, err := errs() + Equal(t, nil, err) + return y + case 5: + // Function values are NOT matched (the identifier resolves to a variable rather than the + // trusted function), so no narrowing happens. + _, err := errs() + f := Error + f(t, err) + takesNonnil(err) //want "passed" + } + return 0 +} From a88c3753984e3bf6c1c63666866f903e21cff5c9 Mon Sep 17 00:00:00 2001 From: Tom van der Woerdt Date: Thu, 11 Jun 2026 19:06:17 -0400 Subject: [PATCH 2/3] Model testify, gotest.tools, and goconvey assertions as trusted functions Extend the SplitBlockOn hook with more assertion functions whose success implies nilability facts about their arguments: - testify `ErrorContains(f)`/`EqualError(f)` (function and method forms): these can only pass with a non-nil error, same as `Error`. - gotest.tools assert (both /v3 and the legacy v1/v2 import path): `NilError`, `Error`, and `ErrorContains`, plus `Assert` via a new `boolOrErrorExpr` action handling its bool and error argument forms. - goconvey `So(actual, assertion, ...)`: a new `soExpr` action resolves the assertion argument to its package-level object and models the nilability-relevant `Should*` assertions. The idiomatic dot-imported call form relies on the dot-import matching from the previous commit. Also hoist the argIndex bounds check from the individual actions into SplitBlockOn, and document that the failure branch of a split is modeled as terminating execution. --- hook/split_blocks_on.go | 120 +++++++++++++++++- .../src/go.uber.org/trustedfunc/goconvey.go | 75 +++++++++++ .../go.uber.org/trustedfunc/gotesttools.go | 84 ++++++++++++ .../go.uber.org/trustedfunc/trustedfuncs.go | 46 +++++++ .../smartystreets/goconvey/convey/convey.go | 39 ++++++ .../stretchr/testify/assert/assert.go | 10 ++ .../stretchr/testify/require/require.go | 25 ++++ .../src/stubs/gotest.tools/assert/assert.go | 29 +++++ .../stubs/gotest.tools/v3/assert/assert.go | 40 ++++++ 9 files changed, 466 insertions(+), 2 deletions(-) create mode 100644 testdata/src/go.uber.org/trustedfunc/goconvey.go create mode 100644 testdata/src/go.uber.org/trustedfunc/gotesttools.go create mode 100644 testdata/src/stubs/github.com/smartystreets/goconvey/convey/convey.go create mode 100644 testdata/src/stubs/gotest.tools/assert/assert.go create mode 100644 testdata/src/stubs/gotest.tools/v3/assert/assert.go diff --git a/hook/split_blocks_on.go b/hook/split_blocks_on.go index 67418f82..1ac25d8a 100644 --- a/hook/split_blocks_on.go +++ b/hook/split_blocks_on.go @@ -81,6 +81,93 @@ var negatedSelfExpr splitBlockOnAction = func(_ *analysishelper.EnhancedPass, ca } } +// boolOrErrorExpr handles assertion arguments declared as `interface{}`, e.g., gotest.tools' +// `assert.Assert(t, comparison BoolOrComparison)`, whose argument may be: +// - a boolean expression, e.g., `assert.Assert(t, x != nil)`: behaves like selfExpr; +// - an error value, e.g., `assert.Assert(t, err)`: behaves like nilBinaryExpr, since a nil error +// means success while a non-nil error fails the assertion. Only interface types (`error` +// itself or interfaces embedding it) qualify: a nil value of such a type stays nil when passed +// as `interface{}`, whereas a concrete error type would be wrapped in a non-nil interface and +// always fail the assertion (even a typed-nil pointer); +// - anything else (e.g., a `cmp.Comparison` closure): no narrowing is applied. +var boolOrErrorExpr splitBlockOnAction = func(pass *analysishelper.EnhancedPass, call *ast.CallExpr, argIndex int) ast.Expr { + if argIndex < 0 || argIndex >= len(call.Args) { + return nil + } + if isBoolExpr(pass, call.Args[argIndex]) { + return selfExpr(pass, call, argIndex) + } + t := pass.TypesInfo.TypeOf(call.Args[argIndex]) + if t == nil { + return nil + } + if _, ok := t.Underlying().(*types.Interface); ok && typeshelper.ImplementsError(t) { + return nilBinaryExpr(pass, call, argIndex) + } + return nil +} + +// _goconveyAssertions matches the package paths where goconvey's `Should*` assertions are +// defined: the `convey` package itself (which re-exports them as package-level variables, e.g., +// `var ShouldBeNil = assertions.ShouldBeNil`) and the underlying assertions package (both its +// current `smarty` and historical `smartystreets` homes, for users importing it directly). +var _goconveyAssertions = regexp.MustCompile(`^(stubs/)?(github\.com/smartystreets/goconvey/convey|github\.com/smarty(streets)?/assertions)$`) + +// goconveySoExpr handles goconvey's `So(actual, assertion, expected...)`, where the narrowing fact is +// determined by the assertion argument rather than the called function: e.g., +// `So(err, ShouldBeNil)` implies `err == nil` afterwards. The assertion argument is resolved to +// its package-level object (a var re-exported by `convey`, or a function of the assertions +// package), and only the nilability-relevant assertions are modeled; any other assertion (or a +// locally-defined custom one) yields no narrowing. +var goconveySoExpr splitBlockOnAction = func(pass *analysishelper.EnhancedPass, call *ast.CallExpr, argIndex int) ast.Expr { + // The assertion argument sits right after the actual expression. + if argIndex+1 >= len(call.Args) { + return nil + } + var ident *ast.Ident + switch assert := call.Args[argIndex+1].(type) { + case *ast.Ident: + ident = assert + case *ast.SelectorExpr: + ident = assert.Sel + default: + return nil + } + obj := pass.TypesInfo.ObjectOf(ident) + if obj == nil || obj.Pkg() == nil || !_goconveyAssertions.MatchString(obj.Pkg().Path()) { + return nil + } + + // For the boolean assertions, the actual argument is declared `interface{}`; only narrow + // when it is statically a boolean expression (anything else fails the assertion at runtime + // anyway). + switch obj.Name() { + case "ShouldBeNil": + return nilBinaryExpr(pass, call, argIndex) + case "ShouldNotBeNil", "ShouldBeError": + return nonnilBinaryExpr(pass, call, argIndex) + case "ShouldBeTrue": + if isBoolExpr(pass, call.Args[argIndex]) { + return selfExpr(pass, call, argIndex) + } + case "ShouldBeFalse": + if isBoolExpr(pass, call.Args[argIndex]) { + return negatedSelfExpr(pass, call, argIndex) + } + } + return nil +} + +// isBoolExpr reports whether the expression is statically of boolean type. +func isBoolExpr(pass *analysishelper.EnhancedPass, expr ast.Expr) bool { + t := pass.TypesInfo.TypeOf(expr) + if t == nil { + return false + } + basic, ok := t.Underlying().(*types.Basic) + return ok && basic.Kind() == types.Bool +} + // The constant (enum) values below represent the possible values of an expected expression in a comparison // E.g., `Equal(1, len(s))`, where `1` is the expected expression and is assigned the value `_greaterThanZero`. // E.g., `Equal(nil, err)`, where `nil` is the expected expression and is assigned the value `_nil`. @@ -300,7 +387,7 @@ var _splitBlockOn = map[trustedSig]struct { { kind: _method, enclosingRegex: regexp.MustCompile(`^(stubs/)?github\.com/stretchr/testify/(suite\.Suite|assert\.Assertions|require\.Assertions)$`), - nameRegex: regexp.MustCompile(`^(NotNil(f)?|Error(f)?)$`), + nameRegex: regexp.MustCompile(`^(NotNil(f)?|Error(f)?|ErrorContains(f)?|EqualError(f)?)$`), }: {action: nonnilBinaryExpr, argIndex: 0}, { kind: _method, @@ -332,7 +419,7 @@ var _splitBlockOn = map[trustedSig]struct { { kind: _func, enclosingRegex: regexp.MustCompile(`^(stubs/)?github\.com/stretchr/testify/(assert|require)$`), - nameRegex: regexp.MustCompile(`^(NotNil(f)?|Error(f)?)$`), + nameRegex: regexp.MustCompile(`^(NotNil(f)?|Error(f)?|ErrorContains(f)?|EqualError(f)?)$`), }: {action: nonnilBinaryExpr, argIndex: 1}, { kind: _func, @@ -364,4 +451,33 @@ var _splitBlockOn = map[trustedSig]struct { enclosingRegex: regexp.MustCompile(`^(stubs/)?github\.com/stretchr/testify/(suite\.Suite|assert\.Assertions|require\.Assertions)$`), nameRegex: regexp.MustCompile(`^(Empty(f)?|NotEmpty(f)?)$`), }: {action: requireZeroComparators, argIndex: 0}, + + // `gotest.tools/v3/assert`, as well as its legacy v1/v2 form `gotest.tools/assert` with + // identical semantics. Note that `ErrorIs` is deliberately NOT modeled with nonnil narrowing: + // `errors.Is(nil, nil)` is true, so `assert.ErrorIs(t, err, nil)` can pass with a nil error. + { + kind: _func, + enclosingRegex: regexp.MustCompile(`^(stubs/)?gotest\.tools(/v3)?/assert$`), + nameRegex: regexp.MustCompile(`^NilError$`), + }: {action: nilBinaryExpr, argIndex: 1}, + { + kind: _func, + enclosingRegex: regexp.MustCompile(`^(stubs/)?gotest\.tools(/v3)?/assert$`), + nameRegex: regexp.MustCompile(`^(Error|ErrorContains)$`), + }: {action: nonnilBinaryExpr, argIndex: 1}, + { + kind: _func, + enclosingRegex: regexp.MustCompile(`^(stubs/)?gotest\.tools(/v3)?/assert$`), + nameRegex: regexp.MustCompile(`^Assert$`), + }: {action: boolOrErrorExpr, argIndex: 1}, + + // `github.com/smartystreets/goconvey/convey`, which is typically dot-imported. Under the + // default `FailureHalts` mode, a failed `So` panics and is recovered by the `Convey` runner, + // halting the enclosing scope; the opt-in `FailureContinues` mode is over-approximated the + // same way as testify's non-fatal `assert` (see the comment on this table). + { + kind: _func, + enclosingRegex: regexp.MustCompile(`^(stubs/)?github\.com/smartystreets/goconvey/convey$`), + nameRegex: regexp.MustCompile(`^So$`), + }: {action: goconveySoExpr, argIndex: 0}, } diff --git a/testdata/src/go.uber.org/trustedfunc/goconvey.go b/testdata/src/go.uber.org/trustedfunc/goconvey.go new file mode 100644 index 00000000..63fae086 --- /dev/null +++ b/testdata/src/go.uber.org/trustedfunc/goconvey.go @@ -0,0 +1,75 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package trustedfunc + +import ( + "testing" + + . "stubs/github.com/smartystreets/goconvey/convey" +) + +// testGoConvey tests goconvey's `So(actual, assertion, expected...)`, whose narrowing fact is +// determined by the assertion argument rather than the called function. The package is +// dot-imported, as is idiomatic for goconvey. `So` is called at function level here because +// NilAway only analyzes `Convey` closure bodies under experimental anonymous-function support. +// +// nilable(x) +func testGoConvey(t *testing.T, x any) any { + switch 0 { + case 1: + y, err := errs() + So(err, ShouldBeNil) + return y + case 2: + // The narrowing direction is nil (not nonnil): err is nil after a passing `ShouldBeNil`. + _, err := errs() + So(err, ShouldBeNil) + takesNonnil(err) //want "passed" + case 3: + _, err := errs() + So(err, ShouldNotBeNil) + takesNonnil(err) + case 4: + _, err := errs() + So(err, ShouldBeError) + takesNonnil(err) + case 5: + So(x != nil, ShouldBeTrue) + return x + case 6: + So(x == nil, ShouldBeFalse) + return x + case 7: + // Unmodeled assertions have no narrowing effect. + _, err := errs() + So(err, ShouldEqual, nil) + takesNonnil(err) //want "passed" + case 8: + // A non-boolean actual with a boolean assertion gets no narrowing (and, importantly, + // does not crash the analysis). + So(x, ShouldBeTrue) + return x //want "returned" + case 9: + // The idiomatic form: `So` inside a `Convey` closure. The closure body is only analyzed + // under experimental anonymous-function support, so no narrowing is asserted here; this + // guards the integration against crashes. + Convey("narrows inside a closure", t, func() { + y, err := errs() + So(err, ShouldBeNil) + consume(y) + }) + } + return 0 +} diff --git a/testdata/src/go.uber.org/trustedfunc/gotesttools.go b/testdata/src/go.uber.org/trustedfunc/gotesttools.go new file mode 100644 index 00000000..647d7085 --- /dev/null +++ b/testdata/src/go.uber.org/trustedfunc/gotesttools.go @@ -0,0 +1,84 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package trustedfunc + +import ( + "testing" + + gtassertv2 "stubs/gotest.tools/assert" + gtassert "stubs/gotest.tools/v3/assert" +) + +// nilable(x) +func testGoTestTools(t *testing.T, x any) any { + switch 0.0 { + case 1.0: + y, err := errs() + gtassert.NilError(t, err) + return y + case 1.1: + y, err := errs() + consume(err) + return y //want "returned" + case 2.0: + y, err := errs() + gtassert.Error(t, err, "oops") + return y //want "returned" + case 2.1: + _, err := errs() + gtassert.Error(t, err, "oops") + takesNonnil(err) + case 2.2: + _, err := errs() + gtassert.ErrorContains(t, err, "oops") + takesNonnil(err) + case 3.0: + gtassert.Assert(t, x != nil) + return x + case 3.1: + // `Assert` with a non-boolean argument (e.g., a `cmp.Comparison`) should have no + // narrowing effect (and, importantly, not crash the analysis). + gtassert.Assert(t, x) + return x //want "returned" + case 3.2: + // `Assert` with an error-typed argument passes iff the error is nil, just like `NilError`. + y, err := errs() + gtassert.Assert(t, err) + return y + case 3.3: + // The error form narrows the error to nil (not nonnil): a passing `Assert(t, err)` means + // `err == nil`, so passing it to a nonnil-expecting function must still be reported. + _, err := errs() + gtassert.Assert(t, err) + takesNonnil(err) //want "passed" + case 4.0: + // `ErrorIs` is intentionally unmodeled (see `_splitBlockOn` in the hook package for the + // rationale), so `err` must still be considered nilable after the call. + _, err := errs() + gtassert.ErrorIs(t, err, nil) + takesNonnil(err) //want "passed" + case 5.0: + // The legacy v1/v2 import path `gotest.tools/assert` (without `/v3`) has identical + // semantics and must be matched by the same trusted function entries. + y, err := errs() + gtassertv2.NilError(t, err) + return y + case 5.1: + _, err := errs() + gtassertv2.Error(t, err, "oops") + takesNonnil(err) + } + return 0 +} diff --git a/testdata/src/go.uber.org/trustedfunc/trustedfuncs.go b/testdata/src/go.uber.org/trustedfunc/trustedfuncs.go index ad0326d7..fc5d4e29 100644 --- a/testdata/src/go.uber.org/trustedfunc/trustedfuncs.go +++ b/testdata/src/go.uber.org/trustedfunc/trustedfuncs.go @@ -94,6 +94,22 @@ func testRequire(t *testing.T, x any, z any, m map[any]any) interface{} { y, err := errs() require.Errorf(t, err, "mymsg: %s", "arg") return y //want "returned" + case 4.4: + y, err := errs() + require.ErrorContains(t, err, "oops") + return y //want "returned" + case 4.5: + y, err := errs() + require.ErrorContainsf(t, err, "oops", "mymsg: %s", "arg") + return y //want "returned" + case 4.6: + y, err := errs() + require.EqualError(t, err, "oops") + return y //want "returned" + case 4.7: + y, err := errs() + require.EqualErrorf(t, err, "oops", "mymsg: %s", "arg") + return y //want "returned" case 5: require.True(t, x != nil) return x @@ -291,6 +307,36 @@ func testBackToBack(t *testing.T) { takesNonnil(x) } +// testErrorNonnilNarrowing tests that assertions which can only pass with a non-nil error (e.g., +// `ErrorContains`, `EqualError`) narrow the error argument itself to be nonnil afterwards. +func testErrorNonnilNarrowing(t *testing.T, r *require.Assertions, a *assert.Assertions) { + switch 0 { + case 1: + _, err := errs() + takesNonnil(err) //want "passed" + case 2: + _, err := errs() + require.ErrorContains(t, err, "oops") + takesNonnil(err) + case 3: + _, err := errs() + require.EqualError(t, err, "oops") + takesNonnil(err) + case 4: + _, err := errs() + assert.ErrorContains(t, err, "oops") + takesNonnil(err) + case 5: + _, err := errs() + r.ErrorContainsf(err, "oops", "mymsg: %s", "arg") + takesNonnil(err) + case 6: + _, err := errs() + a.EqualErrorf(err, "oops", "mymsg: %s", "arg") + takesNonnil(err) + } +} + // test for embedded testify package `suite` at depth 1 type testSetupEmbeddedDepth1 struct { suite.Suite diff --git a/testdata/src/stubs/github.com/smartystreets/goconvey/convey/convey.go b/testdata/src/stubs/github.com/smartystreets/goconvey/convey/convey.go new file mode 100644 index 00000000..d90c7f4b --- /dev/null +++ b/testdata/src/stubs/github.com/smartystreets/goconvey/convey/convey.go @@ -0,0 +1,39 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// +package convey + +// these stubs simulate the real goconvey `convey` package because we can't import it in tests + +type assertion func(actual interface{}, expected ...interface{}) string + +func alwaysPass(_ interface{}, _ ...interface{}) string { return "" } + +// In the real package, the `Should*` assertions are package-level variables re-exported from the +// assertions package (e.g., `var ShouldBeNil = assertions.ShouldBeNil`); they are declared as +// variables here as well so that the resolution logic is exercised against vars, not funcs. +var ( + ShouldBeNil assertion = alwaysPass + ShouldNotBeNil assertion = alwaysPass + ShouldBeError assertion = alwaysPass + ShouldBeTrue assertion = alwaysPass + ShouldBeFalse assertion = alwaysPass + ShouldEqual assertion = alwaysPass +) + +// nilable(actual, expected) +func So(actual interface{}, assert assertion, expected ...interface{}) {} + +func Convey(items ...interface{}) {} diff --git a/testdata/src/stubs/github.com/stretchr/testify/assert/assert.go b/testdata/src/stubs/github.com/stretchr/testify/assert/assert.go index 3bd23049..c439a9ee 100644 --- a/testdata/src/stubs/github.com/stretchr/testify/assert/assert.go +++ b/testdata/src/stubs/github.com/stretchr/testify/assert/assert.go @@ -49,6 +49,11 @@ func Error(t TestingT, object interface{}, msgAndArgs ...interface{}) bool { ret // nilable(object) func Errorf(t TestingT, object interface{}, msg string, args ...interface{}) bool { return true } +// nilable(theError) +func ErrorContains(t TestingT, theError error, contains string, msgAndArgs ...interface{}) bool { + return true +} + func True(t TestingT, value bool, msgAndArgs ...interface{}) bool { return true } func Truef(t TestingT, value bool, msg string, args ...interface{}) bool { return true } @@ -136,6 +141,11 @@ func (*Assertions) Error(object interface{}, msgAndArgs ...interface{}) bool { r // nilable(object) func (*Assertions) Errorf(object interface{}, msg string, args ...interface{}) bool { return true } +// nilable(theError) +func (*Assertions) EqualErrorf(theError error, errString string, msg string, args ...interface{}) bool { + return true +} + func (*Assertions) True(value bool, msgAndArgs ...interface{}) bool { return true } func (*Assertions) Truef(value bool, msg string, args ...interface{}) bool { return true } diff --git a/testdata/src/stubs/github.com/stretchr/testify/require/require.go b/testdata/src/stubs/github.com/stretchr/testify/require/require.go index cf810b89..88312fc8 100644 --- a/testdata/src/stubs/github.com/stretchr/testify/require/require.go +++ b/testdata/src/stubs/github.com/stretchr/testify/require/require.go @@ -51,6 +51,26 @@ func Error(t TestingT, object interface{}, msgAndArgs ...interface{}) bool { ret // nilable(object) func Errorf(t TestingT, object interface{}, msg string, args ...interface{}) bool { return true } +// nilable(theError) +func ErrorContains(t TestingT, theError error, contains string, msgAndArgs ...interface{}) bool { + return true +} + +// nilable(theError) +func ErrorContainsf(t TestingT, theError error, contains string, msg string, args ...interface{}) bool { + return true +} + +// nilable(theError) +func EqualError(t TestingT, theError error, errString string, msgAndArgs ...interface{}) bool { + return true +} + +// nilable(theError) +func EqualErrorf(t TestingT, theError error, errString string, msg string, args ...interface{}) bool { + return true +} + func True(t TestingT, value bool, msgAndArgs ...interface{}) bool { return true } func Truef(t TestingT, value bool, msg string, args ...interface{}) bool { return true } @@ -126,6 +146,11 @@ func (*Assertions) Error(object interface{}, msgAndArgs ...interface{}) bool { r // nilable(object) func (*Assertions) Errorf(object interface{}, msg string, args ...interface{}) bool { return true } +// nilable(theError) +func (*Assertions) ErrorContainsf(theError error, contains string, msg string, args ...interface{}) bool { + return true +} + func (*Assertions) True(value bool, msgAndArgs ...interface{}) bool { return true } func (*Assertions) Truef(value bool, msg string, args ...interface{}) bool { return true } diff --git a/testdata/src/stubs/gotest.tools/assert/assert.go b/testdata/src/stubs/gotest.tools/assert/assert.go new file mode 100644 index 00000000..4ded7b4a --- /dev/null +++ b/testdata/src/stubs/gotest.tools/assert/assert.go @@ -0,0 +1,29 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// +package assert + +// these stubs simulate the legacy v1/v2 `gotest.tools/assert` package (the pre-`/v3` import +// path), whose assertion functions have identical semantics to `gotest.tools/v3/assert` + +type TestingT interface { + FailNow() +} + +// nilable(err) +func NilError(t TestingT, err error, msgAndArgs ...interface{}) {} + +// nilable(err) +func Error(t TestingT, err error, expected string, msgAndArgs ...interface{}) {} diff --git a/testdata/src/stubs/gotest.tools/v3/assert/assert.go b/testdata/src/stubs/gotest.tools/v3/assert/assert.go new file mode 100644 index 00000000..18deaa23 --- /dev/null +++ b/testdata/src/stubs/gotest.tools/v3/assert/assert.go @@ -0,0 +1,40 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// +package assert + +// these stubs simulate the real `gotest.tools/v3/assert` package because we can't import it in tests + +type TestingT interface { + FailNow() +} + +// BoolOrComparison can be a bool, an error (nil means success), or a `cmp.Comparison`. +type BoolOrComparison interface{} + +// nilable(comparison) +func Assert(t TestingT, comparison BoolOrComparison, msgAndArgs ...interface{}) {} + +// nilable(err) +func NilError(t TestingT, err error, msgAndArgs ...interface{}) {} + +// nilable(err) +func Error(t TestingT, err error, expected string, msgAndArgs ...interface{}) {} + +// nilable(err) +func ErrorContains(t TestingT, err error, substring string, msgAndArgs ...interface{}) {} + +// nilable(err, expected) +func ErrorIs(t TestingT, err error, expected error, msgAndArgs ...interface{}) {} From 19859057e0edc3c2dccb382b61e9d3a53b6b0a4d Mon Sep 17 00:00:00 2001 From: Tom van der Woerdt Date: Thu, 11 Jun 2026 19:06:20 -0400 Subject: [PATCH 3/3] Model bool-returning testify assertions used as conditionals `if assert.NoError(t, err) { }` is a common pattern with testify's non-fatal `assert` package, but narrowing previously only applied to statement-position assertion calls (via SplitBlockOn), so NilAway reported false positives inside such guarded branches. Model these through the ReplaceConditional mechanism, following the existing `errors.As` pattern: the conditional is replaced with ` && ` (e.g., `assert.NoError(t, err) && err == nil`), where the implied expression is the same one `_splitBlockOn` derives for the call in statement position, so the per-assertion semantics are defined in a single place. --- hook/replace_conditional.go | 35 ++++++++ .../go.uber.org/trustedfunc/conditional.go | 81 +++++++++++++++++++ 2 files changed, 116 insertions(+) create mode 100644 testdata/src/go.uber.org/trustedfunc/conditional.go diff --git a/hook/replace_conditional.go b/hook/replace_conditional.go index 5434fa42..9d2b7cec 100644 --- a/hook/replace_conditional.go +++ b/hook/replace_conditional.go @@ -69,6 +69,28 @@ var _errorAsAction replaceConditionalAction = func(_ *analysishelper.EnhancedPas } } +// _assertConditionalAction replaces bool-returning testify assertions used as conditionals, e.g., +// `if assert.NoError(t, err) {...}`, with ` && ` (here: +// `assert.NoError(t, err) && err == nil`). The implied expression is the same one `_splitBlockOn` +// derives for the call in statement position, so the per-assertion semantics (including the +// asserted argument's position) are defined only there. The assertion returns true iff it passed, +// so the implied expression holds in the then-branch; the else-branch gains no information from +// the conjunction, which is conservative. Note that, unlike the statement-position modeling in +// `_splitBlockOn`, no assumption about test termination is involved, so this is sound for the +// non-fatal `assert` package as well. +var _assertConditionalAction replaceConditionalAction = func(pass *analysishelper.EnhancedPass, call *ast.CallExpr) ast.Expr { + implied := SplitBlockOn(pass, call) + if implied == nil { + return nil + } + return &ast.BinaryExpr{ + X: call, + Op: token.LAND, + OpPos: call.Pos(), + Y: implied, + } +} + var _replaceConditionals = map[trustedSig]replaceConditionalAction{ { kind: _func, @@ -80,4 +102,17 @@ var _replaceConditionals = map[trustedSig]replaceConditionalAction{ enclosingRegex: regexp.MustCompile(`^(stubs/)?github\.com/cockroachdb/errors$`), nameRegex: regexp.MustCompile(`^As$`), }: _errorAsAction, + + // Bool-returning testify assertions used as conditionals. `require` is absent since its + // functions do not return values and hence cannot appear in a conditional. + { + kind: _func, + enclosingRegex: regexp.MustCompile(`^(stubs/)?github\.com/stretchr/testify/assert$`), + nameRegex: regexp.MustCompile(`^(Nil(f)?|NotNil(f)?|NoError(f)?|Error(f)?|ErrorContains(f)?|EqualError(f)?|True(f)?|False(f)?)$`), + }: _assertConditionalAction, + { + kind: _method, + enclosingRegex: regexp.MustCompile(`^(stubs/)?github\.com/stretchr/testify/(suite\.Suite|assert\.Assertions)$`), + nameRegex: regexp.MustCompile(`^(Nil(f)?|NotNil(f)?|NoError(f)?|Error(f)?|ErrorContains(f)?|EqualError(f)?|True(f)?|False(f)?)$`), + }: _assertConditionalAction, } diff --git a/testdata/src/go.uber.org/trustedfunc/conditional.go b/testdata/src/go.uber.org/trustedfunc/conditional.go new file mode 100644 index 00000000..8a93d195 --- /dev/null +++ b/testdata/src/go.uber.org/trustedfunc/conditional.go @@ -0,0 +1,81 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package trustedfunc + +import ( + "testing" + + "stubs/github.com/stretchr/testify/assert" +) + +// testAssertConditional tests bool-returning testify assertions used as conditionals, e.g., +// `if assert.NoError(t, err) {...}`: the assertion returns true iff it passed, so the implied +// fact holds inside the then-branch (with no assumption about test termination), while the +// else-branch and the code after the conditional gain no information. +// +// nilable(x) +func testAssertConditional(t *testing.T, x any, a *assert.Assertions) any { + switch 0 { + case 1: + y, err := errs() + if assert.NoError(t, err) { + return y + } + case 2: + // No narrowing survives past the conditional. + y, err := errs() + if assert.NoError(t, err) { + consume(y) + } + return y //want "returned" + case 3: + _, err := errs() + if assert.Error(t, err) { + takesNonnil(err) + } + case 4: + if assert.NotNil(t, x) { + takesNonnil(x) + } + takesNonnil(x) //want "passed" + case 5: + // The narrowing direction for `Nil` is nil, so x must not be treated as nonnil. + if assert.Nil(t, x) { + takesNonnil(x) //want "passed" + } + case 6: + if assert.True(t, x != nil) { + return x + } + case 7: + if assert.False(t, x == nil) { + return x + } + case 8: + // Method form on `assert.Assertions`. + y, err := errs() + if a.NoError(err) { + return y + } + case 9: + // The `ok := ...; if ok` form is recognized as well. + y, err := errs() + ok := assert.NoError(t, err) + if ok { + return y + } + } + return 0 +}