Separate the trustedfunc framework from assertiontree package (#248)

This PR separates the `trustedfunc` framework out of the `assertiontree`
package for better organization.

Additionally, we have also removed the `BuiltinAppend` and `BuiltinNew`
inside `trustedfunc` since they are just constant strings. Instead, we
added those _function objects_ to `util` package and the compare the
function objects instead (which is more robust). This will guard us
against cases where the code shadows the builtin function `new` and
`append` (although these are rare).

assertion/function/assertiontree/backprop_util.go

@@ -23,6 +23,7 @@ import (
 	"go/types"
 
 	"go.uber.org/nilaway/annotation"
+	"go.uber.org/nilaway/assertion/function/trustedfunc"
 	"go.uber.org/nilaway/util"
 	"go.uber.org/nilaway/util/asthelper"
 	"golang.org/x/tools/go/analysis"
@@ -244,7 +245,7 @@ func isErrorReturnNil(rootNode *RootAssertionNode, errRet ast.Expr) bool {
 // isErrorReturnNonnil returns true if the error return is guaranteed to be nonnil, false otherwise
 func isErrorReturnNonnil(rootNode *RootAssertionNode, errRet ast.Expr) bool {
 	t := rootNode.Pass().TypesInfo.TypeOf(errRet)
-	if _, ok := AsTrustedFuncAction(errRet, rootNode.Pass()); ok || util.TypeAsDeeplyStruct(t) != nil {
+	if _, ok := trustedfunc.As(errRet, rootNode.Pass()); ok || util.TypeAsDeeplyStruct(t) != nil {
 		return true
 	}
 
@@ -574,12 +575,11 @@ func exprAsAssignmentConsumer(rootNode *RootAssertionNode, expr ast.Node, exprRH
 
 	switch expr := expr.(type) {
 	case *ast.Ident:
-		funcObj := rootNode.FuncObj()
+		// This block checks if the rhs of the assignment is the builtin append function for slices.
 		varObj := rootNode.ObjectOf(expr).(*types.Var)
-		// This block checks if the rhs of the assignment is the builtin append function for slices
 		if call, ok := exprRHS.(*ast.CallExpr); ok && util.TypeIsSlice(varObj.Type()) {
-			if fun, ok := call.Fun.(*ast.Ident); ok && fun.Name == BuiltinAppend {
-				if annotation.VarIsParam(funcObj, varObj) {
+			if fun, ok := call.Fun.(*ast.Ident); ok && rootNode.ObjectOf(fun) == util.BuiltinAppend {
+				if annotation.VarIsParam(rootNode.FuncObj(), varObj) {
 					// If there is a deep assignment to a slice using append method
 					return handleDeepAssignmentToExpr(expr)
 				}

assertion/function/assertiontree/parse_expr_producer.go

@@ -21,6 +21,7 @@ import (
 
 	"go.uber.org/nilaway/annotation"
 	"go.uber.org/nilaway/assertion/function/producer"
+	"go.uber.org/nilaway/assertion/function/trustedfunc"
 	"go.uber.org/nilaway/util"
 )
 
@@ -239,7 +240,7 @@ func (r *RootAssertionNode) ParseExprAsProducer(expr ast.Expr, doNotTrack bool)
 			return true
 		}
 
-		if ret, ok := AsTrustedFuncAction(expr, r.Pass()); ok {
+		if ret, ok := trustedfunc.As(expr, r.Pass()); ok {
 			if prod, ok := ret.(*annotation.ProduceTrigger); ok {
 				return nil, []producer.ParsedProducer{producer.ShallowParsedProducer{Producer: prod}}
 			}
@@ -254,7 +255,7 @@ func (r *RootAssertionNode) ParseExprAsProducer(expr ast.Expr, doNotTrack bool)
 				// only two arguments and the first argument is the same as the lhs of assignment.
 				// Since in Go it is allowed to have only one argument in the append method, we need
 				// to have a check to make sure that len(expr.Args) > 1
-				if fun.Name == BuiltinAppend && len(expr.Args) > 1 {
+				if r.ObjectOf(fun) == util.BuiltinAppend && len(expr.Args) > 1 {
 					// TODO: handle the correlation of return type of append with its first argument .
 					// TODO: iterate over the arguments of the append call if it has more than two args
 					rec, producers := r.ParseExprAsProducer(expr.Args[1], false)
@@ -268,7 +269,7 @@ func (r *RootAssertionNode) ParseExprAsProducer(expr ast.Expr, doNotTrack bool)
 				// uninitialized with a `new(S)`.
 				// TODO: below logic won't be required once we standardize the calls by replacing `new(S)` with `&S{}`
 				//  in the preprocessing phase after  is implemented.
-				if r.functionContext.functionConfig.EnableStructInitCheck && fun.Name == BuiltinNew {
+				if r.functionContext.functionConfig.EnableStructInitCheck && r.ObjectOf(fun) == util.BuiltinNew {
 					rproducer := r.parseStructCreateExprAsProducer(expr.Args[0], nil)
 					if rproducer != nil {
 						return nil, []producer.ParsedProducer{rproducer}

assertion/function/assertiontree/preprocess_blocks.go

@@ -19,6 +19,7 @@ import (
 	"go/ast"
 	"go/token"
 
+	"go.uber.org/nilaway/assertion/function/trustedfunc"
 	"go.uber.org/nilaway/util"
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/cfg"
@@ -140,7 +141,7 @@ func splitBlockOnTrustedFuncs(graph *cfg.CFG, thisBlock, failureBlock *cfg.Block
 		if call, ok = expr.X.(*ast.CallExpr); !ok {
 			continue
 		}
-		if retExpr, ok = AsTrustedFuncAction(call, pass); !ok {
+		if retExpr, ok = trustedfunc.As(call, pass); !ok {
 			continue
 		}
 		if trustedCond, ok = retExpr.(ast.Expr); !ok {

assertion/function/assertiontree/trusted_func.go → assertion/function/trustedfunc/trustedfunc.go

@@ -12,7 +12,11 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package assertiontree
+// Package trustedfunc implements a trusted function framework where it hooks into different parts
+// of NilAway to provide additional context for certain function calls. This is useful for
+// well-known standard or 3rd party libraries where we can encode certain knowledge about them (
+// e.g., `assert.Nil(t, x)` implies `x == nil`) and use that to provide better analysis.
+package trustedfunc
 
 import (
 	"go/ast"
@@ -28,10 +32,11 @@ import (
 
 // NOTE: in the future, when we implement  to add contracts, this trusted func mechanism can possibly be replaced with that one.
 
-// AsTrustedFuncAction checks a function call AST node to see if it is one of the trusted functions, and if it is then runs
-// the corresponding action and returns that as the output along with a bool indicating success or failure.
-// For example, a binary expression `x != nil` is returned for trusted function `assert.NotNil(t, x)`, while a `TrustedFuncNonnil` producer is returned for `errors.New(s)`
-func AsTrustedFuncAction(expr ast.Expr, p *analysis.Pass) (any, bool) {
+// As checks a function call AST node to see if it is one of the trusted functions, and if it is
+// then runs the corresponding action and returns that as the output along with a bool indicating
+// success or failure. For example, a binary expression `x != nil` is returned for trusted function
+// `assert.NotNil(t, x)`, while a `TrustedFuncNonnil` producer is returned for `errors.New(s)`
+func As(expr ast.Expr, p *analysis.Pass) (any, bool) {
 	if call, ok := expr.(*ast.CallExpr); ok {
 		for f, a := range trustedFuncs {
 			if f.match(call, p) {
@@ -470,9 +475,3 @@ var trustedFuncs = map[trustedFuncSig]trustedFuncAction{
 		funcNameRegex:  regexp.MustCompile(`^(Empty(f)?|NotEmpty(f)?)$`),
 	}: {action: requireZeroComparators, argIndex: 0},
 }
-
-// BuiltinAppend is used to check the builtin append method for slice
-const BuiltinAppend = "append"
-
-// BuiltinNew is used to check the builtin `new` function
-const BuiltinNew = "new"

testdata/src/go.uber.org/slices/slices.go

@@ -807,3 +807,9 @@ func testAppendNilableForGlobalVar() {
 	a = append(a, nil) //want "literal `nil` assigned into global variable `a`"
 	print(*a[0])       //want "literal `nil` sliced into"
 }
+
+func testShadowAppend() {
+	// Shadow the builtin append function that returns the same slice without modifications.
+	var append = func(s []*int, x ...*int) []*int { return s }
+	a = append(a, nil) // Safe here because the shadowed append does not touch the elements.
+}

util/util.go

@@ -36,6 +36,12 @@ var BoolType = types.Universe.Lookup("bool").Type()
 // BuiltinLen is the builtin "len" function object.
 var BuiltinLen = types.Universe.Lookup("len")
 
+// BuiltinAppend is the builtin "append" function object.
+var BuiltinAppend = types.Universe.Lookup("append")
+
+// BuiltinNew is the builtin "new" function object.
+var BuiltinNew = types.Universe.Lookup("new")
+
 // TypeIsDeep checks if a type is an expression that directly admits a deep nilability annotation - deep
 // nilability annotations on all other types are ignored
 func TypeIsDeep(t types.Type) bool {