Sonalmahajan15/fix non literal access in maps

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

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

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 {

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 ""

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)

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])
+	}
+}