diff --git a/assertion/function/assertiontree/backprop.go b/assertion/function/assertiontree/backprop.go index 832e655..62869c5 100644 --- a/assertion/function/assertiontree/backprop.go +++ b/assertion/function/assertiontree/backprop.go @@ -964,10 +964,9 @@ func BackpropAcrossFunc(ctx context.Context, pass *analysis.Pass, decl *ast.Func break } - checkCFGFixedPointRuntime( - fmt.Sprintf("BackpropAcrossFunc(%s) Forwards Propagation", decl.Name.Name), - roundCount, len(blocks), - ) + if exit := checkCFGFixedPointRuntime(fmt.Sprintf("BackpropAcrossFunc(%s) Forwards Propagation", decl.Name.Name), roundCount, len(blocks)); exit { + break + } // Move variables from this round to last round and create new ones for next round. // For best performance, we reuse the slices by simply swapping them and clearing the diff --git a/assertion/function/assertiontree/preprocess_blocks.go b/assertion/function/assertiontree/preprocess_blocks.go index 1993261..bc5e7f6 100644 --- a/assertion/function/assertiontree/preprocess_blocks.go +++ b/assertion/function/assertiontree/preprocess_blocks.go @@ -735,7 +735,9 @@ func propagateRichChecks(graph *cfg.CFG, richCheckBlocks [][]RichCheckEffect) [] roundCount++ - checkCFGFixedPointRuntime("RichCheckEffect Forwards Propagation", roundCount, n) + if exit := checkCFGFixedPointRuntime("RichCheckEffect Forwards Propagation", roundCount, n); exit { + break + } } // this strips duplicates from the RichCheckEffect slices diff --git a/assertion/function/assertiontree/util.go b/assertion/function/assertiontree/util.go index 524322e..a9d2ae9 100644 --- a/assertion/function/assertiontree/util.go +++ b/assertion/function/assertiontree/util.go @@ -22,19 +22,22 @@ import ( "go.uber.org/nilaway/annotation" "go.uber.org/nilaway/util" + "go.uber.org/nilaway/util/asthelper" "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/ast/astutil" ) // checkCFGFixedPointRuntime panics if a fixed point iteration loop runs beyond some upper // bounded round number, determined by the number of blocks in the CFG of the analyzed function. -func checkCFGFixedPointRuntime(passName string, currRound, numBlocks int) { +func checkCFGFixedPointRuntime(passName string, currRound, numBlocks int) bool { if maxRound := numBlocks * numBlocks * 2; currRound > maxRound { - panic(fmt.Sprintf("propagation over %d-block CFG in %q ran for "+ - "%d rounds, when maximum allowed was %d rounds.", - numBlocks, passName, currRound, maxRound), - ) + // panic(fmt.Sprintf("propagation over %d-block CFG in %q ran for "+ + // "%d rounds, when maximum allowed was %d rounds.", + // numBlocks, passName, currRound, maxRound), + // ) + return true } + return false } // GetDeclaringPath finds the path of nested AST nodes beginning with the passed interval `[start, end]` @@ -255,7 +258,51 @@ func AddNilCheck(pass *analysis.Pass, expr ast.Expr) (trueCheck, falseCheck Root } produceNegativeNilCheck := func(expr ast.Expr) RootFunc { - return produceExprByTrigger(expr, &annotation.NegativeNilCheck{ProduceTriggerNever: &annotation.ProduceTriggerNever{}}) + return func(self *RootAssertionNode) { + trigger := &annotation.NegativeNilCheck{ProduceTriggerNever: &annotation.ProduceTriggerNever{}} + + self.AddProduction(&annotation.ProduceTrigger{ + Annotation: trigger, + Expr: expr, + }) + + // Iterate over already created triggers and update the producer, if necessary. + // Here we can safely match on consumer expressions since we are looking at adding negative nil checks, + // and they rightly should be applied to only consumers with the same expression as the one in the nil check. + if _, ok := expr.(*ast.IndexExpr); ok { + for i := range self.triggers { + s1, _ := asthelper.PrintExpr(self.triggers[i].Consumer.Expr, self.Pass(), false) + s2, _ := asthelper.PrintExpr(expr, self.Pass(), false) + if s1 == s2 { + self.triggers[i] = annotation.FullTrigger{ + Producer: &annotation.ProduceTrigger{ + Annotation: trigger, + Expr: expr, + }, + Consumer: self.triggers[i].Consumer, + } + } + } + } + + // if e, ok := expr.(*ast.IndexExpr); ok { + // if _, ok := e.X.(*ast.Ident); ok { + // if _, ok := e.Index.(*ast.Ident); ok { + // for i := range self.triggers { + // if self.eqStableTemp(self.triggers[i].Consumer.Expr, expr) { + // self.triggers[i] = annotation.FullTrigger{ + // Producer: &annotation.ProduceTrigger{ + // Annotation: trigger, + // Expr: expr, + // }, + // Consumer: self.triggers[i].Consumer, + // } + // } + // } + // } + // } + // } + } } // An exprCheck is a pattern that we match on that, if successful, will give us a pair @@ -371,15 +418,6 @@ func AddNilCheck(pass *analysis.Pass, expr ast.Expr) (trueCheck, falseCheck Root return noop, noop, true } -func produceExprByTrigger(expr ast.Expr, trigger annotation.ProducingAnnotationTrigger) RootFunc { - return func(self *RootAssertionNode) { - self.AddProduction(&annotation.ProduceTrigger{ - Annotation: trigger, - Expr: expr, - }) - } -} - // CopyNode computes a deep code of an AssertionNode // precondition: node is not nil func CopyNode(node AssertionNode) AssertionNode { diff --git a/testdata/src/go.uber.org/consts/consts.go b/testdata/src/go.uber.org/consts/consts.go index 36b8cdc..7071291 100644 --- a/testdata/src/go.uber.org/consts/consts.go +++ b/testdata/src/go.uber.org/consts/consts.go @@ -39,9 +39,8 @@ func testConst(mp map[string]*string, i int) string { return *mp[lib.MyStrConst] } case 3: - // variable is not considered a stable expression, hence an error would be reported here var v = lib.MyStrConst - if mp[v] == nil || *mp[v] == "" { //want "deep read from parameter `mp`" + if mp[v] == nil || *mp[v] == "" { return "nil" } case 4: @@ -66,23 +65,23 @@ func testConst(mp map[string]*string, i int) string { var unexportedGlobalVar string = "local" -// tests for checking the behavior of indexing with a global variable. It should not be considered a stable expression. +// tests for checking the behavior of indexing with a global variable. // nonnil(mp, mp[]) func testGlobalVar(mp map[string]*string, i int) string { switch i { case 0: // locally defined unexported global variable - if mp[unexportedGlobalVar] == nil || *mp[unexportedGlobalVar] == "" { //want "dereferenced" + if mp[unexportedGlobalVar] == nil || *mp[unexportedGlobalVar] == "" { return "nil" } else { - return *mp[unexportedGlobalVar] //want "dereferenced" + return *mp[unexportedGlobalVar] } case 2: // global variable defined in another package - if mp == nil || mp[lib.MyGlobalVar] == nil || *mp[lib.MyGlobalVar] == "" { //want "dereferenced" + if mp == nil || mp[lib.MyGlobalVar] == nil || *mp[lib.MyGlobalVar] == "" { return "nil" } else { - return *mp[lib.MyGlobalVar] //want "dereferenced" + return *mp[lib.MyGlobalVar] } } return "" diff --git a/testdata/src/go.uber.org/deepnil/inference/deepnil-with-inference.go b/testdata/src/go.uber.org/deepnil/inference/deepnil-with-inference.go index b85c85b..b178560 100644 --- a/testdata/src/go.uber.org/deepnil/inference/deepnil-with-inference.go +++ b/testdata/src/go.uber.org/deepnil/inference/deepnil-with-inference.go @@ -31,6 +31,13 @@ func retNilSometimes() *int { return new(int) } +func retNilSometimes2() *int { + if dummy { + return nil + } + return new(int) +} + func testLocalDeepAssignNil(i int) { switch i { case 0: @@ -56,15 +63,13 @@ func testLocalDeepAssignNil(i int) { m := make(map[int]*int) m[i] = nil if v, ok := m[i]; ok { - _ = *v //want "deep read from local variable `m` dereferenced" + _ = *v //want "dereferenced" } - // m[i] is not recognized as a stable expression, hence an error is reported here. if m[i] != nil { - _ = *m[i] //want "deep read from local variable `m` lacking guarding" + _ = *m[i] } - // m[i] is not recognized as a stable expression, hence an error is reported here. if m[i] != nil { - _ = *m[i] //want "deep read from local variable `m` lacking guarding" + _ = *m[i] } case 3: @@ -73,7 +78,7 @@ func testLocalDeepAssignNil(i int) { if v, ok := m[i]; ok && v != nil { _ = *v } else { - _ = *v //want "deep read from local variable `m` lacking guarding" + _ = *v //want "dereferenced" } case 4: @@ -87,7 +92,7 @@ func testLocalDeepAssignNil(i int) { case 5: sl := make([]*int, 1) sl[i] = nil - _ = *sl[i] //want "deep read from local variable `sl` dereferenced" + _ = *sl[i] //want "dereferenced" case 6: sl := make([]*int, 1) @@ -96,8 +101,8 @@ func testLocalDeepAssignNil(i int) { case 7: sl := make([]*int, 1) - sl[i] = retNilSometimes() - _ = *sl[i] //want "deep read from local variable `sl` dereferenced" + sl[i] = retNilSometimes2() + _ = *sl[i] //want "dereferenced" case 8: ch := make(chan *int) diff --git a/testdata/src/go.uber.org/maps/maps.go b/testdata/src/go.uber.org/maps/maps.go index e2d75be..9af2662 100644 --- a/testdata/src/go.uber.org/maps/maps.go +++ b/testdata/src/go.uber.org/maps/maps.go @@ -668,3 +668,101 @@ func testExplicitBool(mp map[int]*int, i int) *int { } return &i } + +// tests for checking non-literal map accesses + +func retInt() int { + return 0 +} + +type S struct { + f int + g int +} + +// nonnil(mp, mp[]) +func testNonLiteralMapAccess(mp map[int]*int, i, j int) { + switch i { + case 0: + if mp[i] != nil { + print(*mp[i]) + } + + case 1: + if mp[i] == nil { + return + } + print(*mp[i]) + + case 3: + if mp[i] != nil { + i := 10 + print(*mp[i]) //want "lacking guarding" + } + + case 4: + if mp[i] != nil { + print(*mp[j]) //want "lacking guarding" + } + + case 5: + localVar := 0 + if mp[localVar] != nil { + print(mp[localVar]) + } + + case 6: + s := &S{} + if mp[s.f] != nil { + print(*mp[s.f]) + } + + case 7: + s1 := &S{} + s2 := &S{} + if mp[s1.f] != nil { + print(*mp[s2.f]) //want "lacking guarding" + } + + case 8: + s := &S{} + if mp[s.f] != nil { + print(*mp[s.g]) //want "lacking guarding" + } + + case 9: + var sl []*int + if mp[len(sl)] != nil { + print(*mp[len(sl)]) + } + + case 10: + // NilAway does not consider user-defined functions as stable, and hence reports an error here. It could be + // considered a false positive from a user perspective, but NilAway cannot guarantee the stability of the function + // without a more complex analysis. We are currently not choosing to do this since we believe this to be a rare + // case and also an anti-pattern since users should ideally create a local variable and use that instead. + if mp[retInt()] != nil { + print(*mp[retInt()]) //want "lacking guarding" + } + + localVar := retInt() + if mp[localVar] != nil { + print(*mp[localVar]) + } + + case 11: + // TODO: This case is currently a false negative since NilAway does not track the value of `i` across consecutive + // map accesses. We plan to support this in a follow-up PR. + i = 0 + if mp[i] != nil { + i = 100 + print(*mp[i]) // TODO: report error here + } + + case 12: + if _, ok := mp[i]; !ok { + mp[i] = new(int) + } + print(*mp[i]) + } +}