From 11b9b5bbc58bfd3aa947de477a7e39c58600841c Mon Sep 17 00:00:00 2001 From: sonalmahajan15 Date: Thu, 1 Feb 2024 15:29:52 -0800 Subject: [PATCH 1/5] support for map ident --- assertion/function/assertiontree/backprop.go | 14 +++ .../assertiontree/parse_expr_producer.go | 3 +- config/const.go | 2 +- testdata/src/go.uber.org/consts/consts.go | 13 ++- .../inference/deepnil-with-inference.go | 23 +++-- testdata/src/go.uber.org/maps/maps.go | 92 +++++++++++++++++++ util/util.go | 13 +++ 7 files changed, 142 insertions(+), 18 deletions(-) diff --git a/assertion/function/assertiontree/backprop.go b/assertion/function/assertiontree/backprop.go index 832e655..3c5962b 100644 --- a/assertion/function/assertiontree/backprop.go +++ b/assertion/function/assertiontree/backprop.go @@ -815,6 +815,8 @@ func computePostOrder(blocks []*cfg.Block) []int { // to the function, the set of assertions that must hold to avoid possible nil flow errors. func BackpropAcrossFunc(ctx context.Context, pass *analysis.Pass, decl *ast.FuncDecl, functionContext FunctionContext, graph *cfg.CFG) ([]annotation.FullTrigger, error) { + util.WriteToErrorLog(fmt.Sprintf("START ANALYZING %s.%s", pass.Pkg.Path(), decl.Name.Name)) + start := time.Now() // We transform the CFG to have it reflect the implicit control flow that happens // inside short-circuiting boolean expressions. graph, richCheckBlocks, exprNonceMap := preprocess(graph, functionContext) @@ -979,6 +981,18 @@ func BackpropAcrossFunc(ctx context.Context, pass *analysis.Pass, decl *ast.Func currRootAssertionNode, nextRootAssertionNode = nextRootAssertionNode, nil } + triggerLen := 0 + if currRootAssertionNode != nil { + triggerLen = len(currRootAssertionNode.triggers) + } + + end := time.Now() + // fmt.Println("pkg.func,time in sec,iterations,triggers") + s := fmt.Sprintf("%s.%s,%s,%d,%d", pass.Pkg.Path(), decl.Name.Name, end.Sub(start), roundCount, triggerLen) + _ = s + // util.WriteToErrorLog(s) + // fmt.Println(s) + // Return the generated full triggers at the entry block; we're done! if currRootAssertionNode == nil { return nil, nil diff --git a/assertion/function/assertiontree/parse_expr_producer.go b/assertion/function/assertiontree/parse_expr_producer.go index 58cfe6a..5abcf46 100644 --- a/assertion/function/assertiontree/parse_expr_producer.go +++ b/assertion/function/assertiontree/parse_expr_producer.go @@ -327,7 +327,8 @@ func (r *RootAssertionNode) ParseExprAsProducer(expr ast.Expr, doNotTrack bool) } if recv != nil { // receiver is trackable - if r.isStable(expr.Index) { + _, isIndexIdent := expr.Index.(*ast.Ident) + if r.isStable(expr.Index) || (util.TypeIsDeeplyMap(util.TypeOf(r.Pass(), expr.X)) && isIndexIdent) { // receiver is trackable and index is stable, so return an augmented path return append(recv, &indexAssertionNode{ index: expr.Index, diff --git a/config/const.go b/config/const.go index 85bfe3d..4e6f650 100644 --- a/config/const.go +++ b/config/const.go @@ -23,7 +23,7 @@ package config // In practice, a value of StableRoundLimit >= 2 has shown to provide sound analysis, capturing most false negatives. // After experimentation, we observed that using StableRoundLimit = 5 with NilAway yields similar analysis time compared // to lower values, making it a good compromise for precise results. -const StableRoundLimit = 5 +const StableRoundLimit = 2 // ErrorOnNilableMapRead configures whether reading from nil maps should be considered an error. // Since Go does not panic on this, right now we do not interpret it as one, but this could be 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..534a595 100644 --- a/testdata/src/go.uber.org/maps/maps.go +++ b/testdata/src/go.uber.org/maps/maps.go @@ -668,3 +668,95 @@ 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 80: + // 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 + } + } +} diff --git a/util/util.go b/util/util.go index f9ab42b..87238ef 100644 --- a/util/util.go +++ b/util/util.go @@ -20,6 +20,8 @@ import ( "go/ast" "go/token" "go/types" + "log" + "os" "regexp" "strings" @@ -471,3 +473,14 @@ func truncatePosition(position token.Position) token.Position { func PosToLocation(pos token.Pos, pass *analysis.Pass) token.Position { return truncatePosition(pass.Fset.Position(pos)) } + +func WriteToErrorLog(s string) { + f, err := os.OpenFile("/tmp/nilaway_log", os.O_RDWR|os.O_CREATE|os.O_APPEND, 0644) + if err != nil { + log.Fatalf("error opening file: %v", err) + } + defer f.Close() + + log.SetOutput(f) + log.Println(s) +} From dbd49fb197018f6516035eebf841d1cb5c4d88ec Mon Sep 17 00:00:00 2001 From: sonalmahajan15 Date: Fri, 29 Mar 2024 17:57:17 -0700 Subject: [PATCH 2/5] some changes --- assertion/function/assertiontree/backprop.go | 14 -------------- config/const.go | 2 +- testdata/src/go.uber.org/maps/maps.go | 8 +++++++- util/util.go | 13 ------------- 4 files changed, 8 insertions(+), 29 deletions(-) diff --git a/assertion/function/assertiontree/backprop.go b/assertion/function/assertiontree/backprop.go index 3c5962b..832e655 100644 --- a/assertion/function/assertiontree/backprop.go +++ b/assertion/function/assertiontree/backprop.go @@ -815,8 +815,6 @@ func computePostOrder(blocks []*cfg.Block) []int { // to the function, the set of assertions that must hold to avoid possible nil flow errors. func BackpropAcrossFunc(ctx context.Context, pass *analysis.Pass, decl *ast.FuncDecl, functionContext FunctionContext, graph *cfg.CFG) ([]annotation.FullTrigger, error) { - util.WriteToErrorLog(fmt.Sprintf("START ANALYZING %s.%s", pass.Pkg.Path(), decl.Name.Name)) - start := time.Now() // We transform the CFG to have it reflect the implicit control flow that happens // inside short-circuiting boolean expressions. graph, richCheckBlocks, exprNonceMap := preprocess(graph, functionContext) @@ -981,18 +979,6 @@ func BackpropAcrossFunc(ctx context.Context, pass *analysis.Pass, decl *ast.Func currRootAssertionNode, nextRootAssertionNode = nextRootAssertionNode, nil } - triggerLen := 0 - if currRootAssertionNode != nil { - triggerLen = len(currRootAssertionNode.triggers) - } - - end := time.Now() - // fmt.Println("pkg.func,time in sec,iterations,triggers") - s := fmt.Sprintf("%s.%s,%s,%d,%d", pass.Pkg.Path(), decl.Name.Name, end.Sub(start), roundCount, triggerLen) - _ = s - // util.WriteToErrorLog(s) - // fmt.Println(s) - // Return the generated full triggers at the entry block; we're done! if currRootAssertionNode == nil { return nil, nil diff --git a/config/const.go b/config/const.go index 4e6f650..85bfe3d 100644 --- a/config/const.go +++ b/config/const.go @@ -23,7 +23,7 @@ package config // In practice, a value of StableRoundLimit >= 2 has shown to provide sound analysis, capturing most false negatives. // After experimentation, we observed that using StableRoundLimit = 5 with NilAway yields similar analysis time compared // to lower values, making it a good compromise for precise results. -const StableRoundLimit = 2 +const StableRoundLimit = 5 // ErrorOnNilableMapRead configures whether reading from nil maps should be considered an error. // Since Go does not panic on this, right now we do not interpret it as one, but this could be diff --git a/testdata/src/go.uber.org/maps/maps.go b/testdata/src/go.uber.org/maps/maps.go index 534a595..9af2662 100644 --- a/testdata/src/go.uber.org/maps/maps.go +++ b/testdata/src/go.uber.org/maps/maps.go @@ -750,7 +750,7 @@ func testNonLiteralMapAccess(mp map[int]*int, i, j int) { print(*mp[localVar]) } - case 80: + 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 @@ -758,5 +758,11 @@ func testNonLiteralMapAccess(mp map[int]*int, i, j int) { i = 100 print(*mp[i]) // TODO: report error here } + + case 12: + if _, ok := mp[i]; !ok { + mp[i] = new(int) + } + print(*mp[i]) } } diff --git a/util/util.go b/util/util.go index 87238ef..f9ab42b 100644 --- a/util/util.go +++ b/util/util.go @@ -20,8 +20,6 @@ import ( "go/ast" "go/token" "go/types" - "log" - "os" "regexp" "strings" @@ -473,14 +471,3 @@ func truncatePosition(position token.Position) token.Position { func PosToLocation(pos token.Pos, pass *analysis.Pass) token.Position { return truncatePosition(pass.Fset.Position(pos)) } - -func WriteToErrorLog(s string) { - f, err := os.OpenFile("/tmp/nilaway_log", os.O_RDWR|os.O_CREATE|os.O_APPEND, 0644) - if err != nil { - log.Fatalf("error opening file: %v", err) - } - defer f.Close() - - log.SetOutput(f) - log.Println(s) -} From b2bcd7e0efa5345d7c656e9bc850a4255e3f6d6e Mon Sep 17 00:00:00 2001 From: sonalmahajan15 Date: Wed, 3 Apr 2024 14:55:40 -0700 Subject: [PATCH 3/5] another solution --- .../assertiontree/parse_expr_producer.go | 3 +- assertion/function/assertiontree/util.go | 106 ++++++++++++++++-- 2 files changed, 100 insertions(+), 9 deletions(-) diff --git a/assertion/function/assertiontree/parse_expr_producer.go b/assertion/function/assertiontree/parse_expr_producer.go index 5abcf46..58cfe6a 100644 --- a/assertion/function/assertiontree/parse_expr_producer.go +++ b/assertion/function/assertiontree/parse_expr_producer.go @@ -327,8 +327,7 @@ func (r *RootAssertionNode) ParseExprAsProducer(expr ast.Expr, doNotTrack bool) } if recv != nil { // receiver is trackable - _, isIndexIdent := expr.Index.(*ast.Ident) - if r.isStable(expr.Index) || (util.TypeIsDeeplyMap(util.TypeOf(r.Pass(), expr.X)) && isIndexIdent) { + if r.isStable(expr.Index) { // receiver is trackable and index is stable, so return an augmented path return append(recv, &indexAssertionNode{ index: expr.Index, diff --git a/assertion/function/assertiontree/util.go b/assertion/function/assertiontree/util.go index 524322e..fc48ca0 100644 --- a/assertion/function/assertiontree/util.go +++ b/assertion/function/assertiontree/util.go @@ -255,7 +255,35 @@ 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 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,12 +399,76 @@ 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, - }) +// Between two stable expressions, check if we expect them to produce the same value +// precondition: isStable(left) && isStable(right), then checks if left and right are equal +func (r *RootAssertionNode) eqStableTemp(left, right ast.Expr) bool { + right = astutil.Unparen(right) + switch left := astutil.Unparen(left).(type) { + case *ast.BasicLit: + if right, ok := right.(*ast.BasicLit); ok { + return left.Value == right.Value + } + return false + case *ast.BinaryExpr: + if right, ok := right.(*ast.BinaryExpr); ok { + return left.Op == right.Op && + r.eqStableTemp(left.X, right.X) && r.eqStableTemp(left.Y, right.Y) + } + return false + case *ast.UnaryExpr: + if right, ok := right.(*ast.UnaryExpr); ok { + return left.Op == right.Op && r.eqStableTemp(left.X, right.X) + } + return false + case *ast.CallExpr: + if right, ok := right.(*ast.CallExpr); ok { + if len(left.Args) != len(right.Args) { + return false + } + for i := range left.Args { + if !r.eqStableTemp(left.Args[i], right.Args[i]) { + return false + } + } + return r.eqStableTemp(left.Fun, right.Fun) + } + return false + case *ast.Ident: + if right, ok := right.(*ast.Ident); ok { + // if the two identifiers are special values, just check them for string equality + if (r.isNil(left) && r.isNil(right)) || + (r.isBuiltIn(left) && r.isBuiltIn(right)) || + (r.isConst(left) && (r.isConst(right))) || + (r.isPkgName(left) && r.isPkgName(right)) { + return left.Name == right.Name + } + rightVarObj, rightOk := r.ObjectOf(right).(*types.Var) + leftVarObj, leftOk := r.ObjectOf(left).(*types.Var) + + if !rightOk || !leftOk { + return false // here, we have eliminated all of the cases in which + // non-variable identifiers can be equal, so if either side is a + // non-variable then the sides are not equal + } + // if they are variables, check them for declaration equality + return leftVarObj == rightVarObj + } + return false + case *ast.SelectorExpr: + if right, ok := right.(*ast.SelectorExpr); ok { + if !r.eqStableTemp(left.Sel, right.Sel) { + return false + } + return r.eqStableTemp(left.X, right.X) + } + return false + case *ast.IndexExpr: + if right, ok := right.(*ast.IndexExpr); ok { + return r.eqStableTemp(left.X, right.X) && r.eqStableTemp(left.Index, right.Index) + } + return false + default: + return false } } From cfb21ae9768e33b95bcc35347b107315552f5a15 Mon Sep 17 00:00:00 2001 From: sonalmahajan15 Date: Wed, 3 Apr 2024 15:10:45 -0700 Subject: [PATCH 4/5] one more solution --- assertion/function/assertiontree/util.go | 116 ++++++----------------- 1 file changed, 30 insertions(+), 86 deletions(-) diff --git a/assertion/function/assertiontree/util.go b/assertion/function/assertiontree/util.go index fc48ca0..f547f60 100644 --- a/assertion/function/assertiontree/util.go +++ b/assertion/function/assertiontree/util.go @@ -22,6 +22,7 @@ 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" ) @@ -266,23 +267,39 @@ func AddNilCheck(pass *analysis.Pass, expr ast.Expr) (trueCheck, falseCheck Root // 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 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, - } - } + 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, + // } + // } + // } + // } + // } + // } } } @@ -399,79 +416,6 @@ func AddNilCheck(pass *analysis.Pass, expr ast.Expr) (trueCheck, falseCheck Root return noop, noop, true } -// Between two stable expressions, check if we expect them to produce the same value -// precondition: isStable(left) && isStable(right), then checks if left and right are equal -func (r *RootAssertionNode) eqStableTemp(left, right ast.Expr) bool { - right = astutil.Unparen(right) - switch left := astutil.Unparen(left).(type) { - case *ast.BasicLit: - if right, ok := right.(*ast.BasicLit); ok { - return left.Value == right.Value - } - return false - case *ast.BinaryExpr: - if right, ok := right.(*ast.BinaryExpr); ok { - return left.Op == right.Op && - r.eqStableTemp(left.X, right.X) && r.eqStableTemp(left.Y, right.Y) - } - return false - case *ast.UnaryExpr: - if right, ok := right.(*ast.UnaryExpr); ok { - return left.Op == right.Op && r.eqStableTemp(left.X, right.X) - } - return false - case *ast.CallExpr: - if right, ok := right.(*ast.CallExpr); ok { - if len(left.Args) != len(right.Args) { - return false - } - for i := range left.Args { - if !r.eqStableTemp(left.Args[i], right.Args[i]) { - return false - } - } - return r.eqStableTemp(left.Fun, right.Fun) - } - return false - case *ast.Ident: - if right, ok := right.(*ast.Ident); ok { - // if the two identifiers are special values, just check them for string equality - if (r.isNil(left) && r.isNil(right)) || - (r.isBuiltIn(left) && r.isBuiltIn(right)) || - (r.isConst(left) && (r.isConst(right))) || - (r.isPkgName(left) && r.isPkgName(right)) { - return left.Name == right.Name - } - rightVarObj, rightOk := r.ObjectOf(right).(*types.Var) - leftVarObj, leftOk := r.ObjectOf(left).(*types.Var) - - if !rightOk || !leftOk { - return false // here, we have eliminated all of the cases in which - // non-variable identifiers can be equal, so if either side is a - // non-variable then the sides are not equal - } - // if they are variables, check them for declaration equality - return leftVarObj == rightVarObj - } - return false - case *ast.SelectorExpr: - if right, ok := right.(*ast.SelectorExpr); ok { - if !r.eqStableTemp(left.Sel, right.Sel) { - return false - } - return r.eqStableTemp(left.X, right.X) - } - return false - case *ast.IndexExpr: - if right, ok := right.(*ast.IndexExpr); ok { - return r.eqStableTemp(left.X, right.X) && r.eqStableTemp(left.Index, right.Index) - } - return false - default: - return false - } -} - // CopyNode computes a deep code of an AssertionNode // precondition: node is not nil func CopyNode(node AssertionNode) AssertionNode { From 4cc27e976071973079a97c6e2bda7453d60390d2 Mon Sep 17 00:00:00 2001 From: sonalmahajan15 Date: Wed, 3 Apr 2024 15:25:22 -0700 Subject: [PATCH 5/5] one more solution --- assertion/function/assertiontree/backprop.go | 7 +++---- .../function/assertiontree/preprocess_blocks.go | 4 +++- assertion/function/assertiontree/util.go | 12 +++++++----- 3 files changed, 13 insertions(+), 10 deletions(-) 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 f547f60..a9d2ae9 100644 --- a/assertion/function/assertiontree/util.go +++ b/assertion/function/assertiontree/util.go @@ -29,13 +29,15 @@ import ( // 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]`