Skip to content

Commit 934834a

Browse files
yuxincsyuxincs
authored
Merge fca4e01 into 3c8ea06 
2 parents 3c8ea06 + fca4e01 commit 934834a

3 files changed

Lines changed: 194 additions & 26 deletions

File tree

‎assertion/function/preprocess/cfg.go‎

Lines changed: 67 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -55,15 +55,30 @@ func (p *Preprocessor) CFG(graph *cfg.CFG, funcDecl *ast.FuncDecl) *cfg.CFG {
5555
failureBlock := &cfg.Block{Index: int32(len(graph.Blocks))}
5656
graph.Blocks = append(graph.Blocks, failureBlock)
5757

58-
// Perform the (series of) CFG transformations.
58+
// Perform a series of CFG transformations here (for hooks and canonicalization). The order of
59+
// these transformations matters due to canonicalization. Some transformations may expect the
60+
// CFG to be in canonical form, and some transformations may change the CFG structure in a way
61+
// that it needs to be re-canonicalized.
62+
63+
// split blocks do not require the CFG to be in canonical form, and it may modify the CFG
64+
// structure in a way that it needs to be re-canonicalized. Here, we cleverly bundles the two
65+
// operations together such that we only need to run canonicalization once.
5966
for _, block := range graph.Blocks {
6067
if block.Live {
6168
p.splitBlockOnTrustedFuncs(graph, block, failureBlock)
6269
}
6370
}
6471
for _, block := range graph.Blocks {
6572
if block.Live {
66-
p.restructureConditional(graph, block)
73+
p.canonicalizeConditional(graph, block)
74+
}
75+
}
76+
// Replacing conditionals in the CFG requires the CFG to be in canonical form (such that it
77+
// does not have to handle "trustedFunc() && trustedFunc()"), and it will canonicalize the
78+
// modified block by itself.
79+
for _, block := range graph.Blocks {
80+
if block.Live {
81+
p.replaceConditional(graph, block)
6782
}
6883
}
6984

@@ -119,6 +134,10 @@ func copyGraph(graph *cfg.CFG) *cfg.CFG {
119134
return newGraph
120135
}
121136

137+
// splitBlockOnTrustedFuncs splits the CFG block into two parts upon seeing a trusted function
138+
// from the hook framework (e.g., "require.Nil(t, arg)" to "if arg == nil { <all code after> }".
139+
// This does not expect the CFG to be in canonical form, and it may change the CFG structure in a
140+
// way that it needs to be re-canonicalized.
122141
func (p *Preprocessor) splitBlockOnTrustedFuncs(graph *cfg.CFG, thisBlock, failureBlock *cfg.Block) {
123142
for i, node := range thisBlock.Nodes {
124143
expr, ok := node.(*ast.ExprStmt)
@@ -153,47 +172,69 @@ func (p *Preprocessor) splitBlockOnTrustedFuncs(graph *cfg.CFG, thisBlock, failu
153172
}
154173
}
155174

156-
func (p *Preprocessor) restructureConditional(graph *cfg.CFG, thisBlock *cfg.Block) {
157-
// We only restructure non-empty branching blocks.
158-
if len(thisBlock.Nodes) == 0 || len(thisBlock.Succs) != 2 {
175+
// replaceConditional calls the hook functions and replaces the conditional expressions in the CFG
176+
// with the returned equivalent expression for analysis.
177+
//
178+
// This function expects the CFG to be in canonical form to fully function (otherwise it may miss
179+
// cases like "trustedFunc() && trustedFunc()").
180+
//
181+
// It also calls canonicalizeConditional to canonicalize the transformed block such that the CFG
182+
// is still canonical.
183+
func (p *Preprocessor) replaceConditional(graph *cfg.CFG, block *cfg.Block) {
184+
// We only replace conditionals on branching blocks.
185+
if len(block.Nodes) == 0 || len(block.Succs) != 2 {
159186
return
160187
}
161-
cond, ok := thisBlock.Nodes[len(thisBlock.Nodes)-1].(ast.Expr)
188+
call, ok := block.Nodes[len(block.Nodes)-1].(*ast.CallExpr)
162189
if !ok {
163190
return
164191
}
192+
replaced := hook.ReplaceConditional(p.pass, call)
193+
if replaced == nil {
194+
return
195+
}
165196

166-
// places a new given node into the last position of this block
167-
replaceCond := func(node ast.Node) {
168-
thisBlock.Nodes[len(thisBlock.Nodes)-1] = node
197+
block.Nodes[len(block.Nodes)-1] = replaced
198+
// The returned expression may be a binary expression, so we need to canonicalize the CFG again
199+
// after such replacement.
200+
p.canonicalizeConditional(graph, block)
201+
}
202+
203+
// canonicalizeConditional canonicalizes the conditional CFG structures to make it easier to reason
204+
// about control flows later. For example, it rewrites
205+
// `if !cond {T} {F}` to `if cond {F} {T}` (swap successors), and rewrites
206+
// `if cond1 && cond2 {T} {F}` to `if cond1 {if cond2 {T} else {F}}{F}` (nesting).
207+
func (p *Preprocessor) canonicalizeConditional(graph *cfg.CFG, thisBlock *cfg.Block) {
208+
// We only restructure non-empty branching blocks.
209+
if len(thisBlock.Nodes) == 0 || len(thisBlock.Succs) != 2 {
210+
return
169211
}
170212

171213
trueBranch := thisBlock.Succs[0] // type *cfg.Block
172214
falseBranch := thisBlock.Succs[1] // type *cfg.Block
173215

174-
replaceTrueBranch := func(block *cfg.Block) {
175-
thisBlock.Succs[0] = block
176-
}
177-
replaceFalseBranch := func(block *cfg.Block) {
178-
thisBlock.Succs[1] = block
179-
}
216+
// A few helper functions to make the code more readable.
217+
replaceCond := func(node ast.Node) { thisBlock.Nodes[len(thisBlock.Nodes)-1] = node } // The conditional expr is the last node in the block.
218+
replaceTrueBranch := func(block *cfg.Block) { thisBlock.Succs[0] = block }
219+
replaceFalseBranch := func(block *cfg.Block) { thisBlock.Succs[1] = block }
220+
swapTrueFalseBranches := func() { replaceTrueBranch(falseBranch); replaceFalseBranch(trueBranch) }
180221

181-
swapTrueFalseBranches := func() {
182-
replaceTrueBranch(falseBranch)
183-
replaceFalseBranch(trueBranch)
222+
cond, ok := thisBlock.Nodes[len(thisBlock.Nodes)-1].(ast.Expr)
223+
if !ok {
224+
return
184225
}
185226

186227
switch cond := cond.(type) {
187228
case *ast.ParenExpr:
188229
// if a parenexpr, strip and restart - this is done with recursion to account for ((((x)))) case
189230
replaceCond(cond.X)
190-
p.restructureConditional(graph, thisBlock) // recur within parens
231+
p.canonicalizeConditional(graph, thisBlock) // recur within parens
191232
case *ast.UnaryExpr:
192233
if cond.Op == token.NOT {
193234
// swap successors - i.e. swap true and false branches
194235
swapTrueFalseBranches()
195236
replaceCond(cond.X)
196-
p.restructureConditional(graph, thisBlock) // recur within NOT
237+
p.canonicalizeConditional(graph, thisBlock) // recur within NOT
197238
}
198239
case *ast.BinaryExpr:
199240
// Logical AND and Logical OR actually require the exact same short circuiting behavior
@@ -214,8 +255,8 @@ func (p *Preprocessor) restructureConditional(graph *cfg.CFG, thisBlock *cfg.Blo
214255
replaceFalseBranch(newBlock)
215256
}
216257
graph.Blocks = append(graph.Blocks, newBlock)
217-
p.restructureConditional(graph, thisBlock)
218-
p.restructureConditional(graph, newBlock)
258+
p.canonicalizeConditional(graph, thisBlock)
259+
p.canonicalizeConditional(graph, newBlock)
219260
}
220261

221262
// Standardize binary expressions to be of the form `expr OP literal` by swapping `x` and `y`, if `x` is a literal.
@@ -277,8 +318,8 @@ func (p *Preprocessor) restructureConditional(graph *cfg.CFG, thisBlock *cfg.Blo
277318
Op: token.NOT,
278319
X: x,
279320
}
280-
replaceCond(newCond) // replaces `ok != true` with `!ok`
281-
p.restructureConditional(graph, thisBlock) // recur to swap true and false branches for the unary expr `!ok`
321+
replaceCond(newCond) // replaces `ok != true` with `!ok`
322+
p.canonicalizeConditional(graph, thisBlock) // recur to swap true and false branches for the unary expr `!ok`
282323
}
283324

284325
case token.EQL:
@@ -292,8 +333,8 @@ func (p *Preprocessor) restructureConditional(graph *cfg.CFG, thisBlock *cfg.Blo
292333
Op: token.NOT,
293334
X: x,
294335
}
295-
replaceCond(newCond) // replaces `ok == false` with `!ok`
296-
p.restructureConditional(graph, thisBlock) // recur to swap true and false branches for the unary expr `!ok`
336+
replaceCond(newCond) // replaces `ok == false` with `!ok`
337+
p.canonicalizeConditional(graph, thisBlock) // recur to swap true and false branches for the unary expr `!ok`
297338
}
298339
}
299340
}

‎hook/replace_conditional.go‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
// Copyright (c) 2024 Uber Technologies, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package hook
16+
17+
import (
18+
"go/ast"
19+
"go/token"
20+
"regexp"
21+
22+
"golang.org/x/tools/go/analysis"
23+
)
24+
25+
// ReplaceConditional replaces a call to a matched function with the returned expression. This is
26+
// useful for modeling stdlib and 3rd party functions that return a single boolean value, which
27+
// implies nilability of the arguments. For example, `errors.As(err, &target)` implies
28+
// `target != nil`, so it can be replaced with `target != nil`.
29+
//
30+
// If the call does not match any known function, nil is returned.
31+
func ReplaceConditional(pass *analysis.Pass, call *ast.CallExpr) ast.Expr {
32+
for sig, act := range _replaceConditionals {
33+
if sig.match(pass, call) {
34+
return act(pass, call)
35+
}
36+
}
37+
return nil
38+
}
39+
40+
type replaceConditionalAction func(pass *analysis.Pass, call *ast.CallExpr) ast.Expr
41+
42+
// _errorAsAction replaces a call to `errors.As(err, &target)` with an equivalent expression
43+
// `errors.As(err, &target) && target != nil`. Keeping the `errors.As(err, &target)` is important
44+
// since `err` may contain complex expressions that may have nilness issues.
45+
//
46+
// Note that technically `target` can still be nil even if `errors.As(err, &target)` is true. For
47+
// example, if err is a typed nil (e.g., `var err *exec.ExitError`), then `errors.As` would
48+
// actually find a match, but `target` would be set to the typed nil value, resulting in a `nil`
49+
// target. However, in practice this should rarely happen such that even the official documentation
50+
// assumes the target is non-nil after such check [1]. So here we make this assumption as well.
51+
//
52+
// [1] https://pkg.go.dev/errors#As
53+
var _errorAsAction replaceConditionalAction = func(_ *analysis.Pass, call *ast.CallExpr) ast.Expr {
54+
if len(call.Args) != 2 {
55+
return nil
56+
}
57+
unaryExpr, ok := call.Args[1].(*ast.UnaryExpr)
58+
if !ok {
59+
return nil
60+
}
61+
if unaryExpr.Op != token.AND {
62+
return nil
63+
}
64+
return &ast.BinaryExpr{
65+
X: call,
66+
Op: token.LAND,
67+
OpPos: call.Pos(),
68+
Y: newNilBinaryExpr(unaryExpr.X, token.NEQ),
69+
}
70+
}
71+
72+
var _replaceConditionals = map[trustedFuncSig]replaceConditionalAction{
73+
{
74+
kind: _func,
75+
enclosingRegex: regexp.MustCompile(`^errors$`),
76+
funcNameRegex: regexp.MustCompile(`^As$`),
77+
}: _errorAsAction,
78+
}

‎testdata/src/go.uber.org/testing/trustedfuncs.go‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,9 @@ This package aims to test any nilaway behavior specific to accomdating tests, su
2020
package testing
2121

2222
import (
23+
"errors"
24+
"os/exec"
25+
2326
"go.uber.org/testing/github.com/stretchr/testify/assert"
2427
"go.uber.org/testing/github.com/stretchr/testify/require"
2528
"go.uber.org/testing/github.com/stretchr/testify/suite"
@@ -954,3 +957,49 @@ func testEmpty(t *testing.T, i int, a []int, mp map[int]*int) interface{} {
954957

955958
return 0
956959
}
960+
961+
// nilable(err)
962+
func errorsAs(err error, num string, dummy bool) {
963+
switch num {
964+
case "simple":
965+
var exitErr *exec.ExitError
966+
if errors.As(err, &exitErr) {
967+
print(*exitErr)
968+
}
969+
print(*exitErr) //want "unassigned variable `exitErr` dereferenced"
970+
case "not in if block":
971+
var exitErr *exec.ExitError
972+
// Not checking the result of `errors.As` would not guard the variable.
973+
errors.As(err, &exitErr)
974+
print(*exitErr) //want "unassigned variable `exitErr` dereferenced"
975+
case "two errors connected by AND":
976+
var exitErr, anotherErr *exec.ExitError
977+
if errors.As(err, &exitErr) && errors.As(err, &anotherErr) {
978+
print(*exitErr)
979+
print(*anotherErr)
980+
}
981+
case "errors.As with other conditionals connected by AND":
982+
var exitErr *exec.ExitError
983+
if errors.As(err, &exitErr) && dummy {
984+
print(*exitErr)
985+
}
986+
case "errors.As with other conditionals connected by OR":
987+
var exitErr *exec.ExitError
988+
if errors.As(err, &exitErr) || dummy {
989+
print(*exitErr) //want "unassigned variable `exitErr` dereferenced"
990+
}
991+
case "two errors connected by OR":
992+
var exitErr, anotherErr *exec.ExitError
993+
if errors.As(err, &exitErr) || errors.As(err, &anotherErr) {
994+
// We do not know the nilability of either.
995+
print(*exitErr) //want "unassigned variable `exitErr` dereferenced"
996+
print(*anotherErr) //want "unassigned variable `anotherErr` dereferenced"
997+
}
998+
case "nil dereference in first argument":
999+
var exitErr *exec.ExitError
1000+
var nilError *error
1001+
if errors.As(*nilError, &exitErr) { //want "unassigned variable `nilError` dereferenced"
1002+
print(*exitErr) // But this is fine!
1003+
}
1004+
}
1005+
}

0 commit comments

Comments
 (0)