google · TristonianJones · May 6, 2020 · Apr 22, 2020 · Apr 22, 2020 · Apr 22, 2020
diff --git a/checker/checker.go b/checker/checker.go
@@ -23,6 +23,7 @@ import (
 	"github.com/golang/protobuf/proto"
 	"github.com/google/cel-go/checker/decls"
 	"github.com/google/cel-go/common"
+	"github.com/google/cel-go/common/packages"
 	"github.com/google/cel-go/common/types/ref"
 
 	exprpb "google.golang.org/genproto/googleapis/api/expr/v1alpha1"
@@ -144,31 +145,41 @@ func (c *checker) checkNullLiteral(e *exprpb.Expr) {
 
 func (c *checker) checkIdent(e *exprpb.Expr) {
 	identExpr := e.GetIdentExpr()
-	if ident := c.env.LookupIdent(identExpr.Name); ident != nil {
+	// Check to see if the identifier is declared.
+	if ident := c.env.LookupIdent(identExpr.GetName()); ident != nil {
 		c.setType(e, ident.GetIdent().Type)
-		c.setReference(e, newIdentReference(ident.Name, ident.GetIdent().Value))
+		c.setReference(e, newIdentReference(ident.GetName(), ident.GetIdent().Value))
+		// Overwrite the identifier with its fully qualified name.
+		identExpr.Name = ident.GetName()
 		return
 	}
 
 	c.setType(e, decls.Error)
 	c.errors.undeclaredReference(
-		c.location(e), c.env.packager.Package(), identExpr.Name)
+		c.location(e), c.env.packager.Package(), identExpr.GetName())
 }
 
 func (c *checker) checkSelect(e *exprpb.Expr) {
 	sel := e.GetSelectExpr()
 	// Before traversing down the tree, try to interpret as qualified name.
-	qname, found := toQualifiedName(e)
+	qname, found := packages.ToQualifiedName(e)
 	if found {
 		ident := c.env.LookupIdent(qname)
 		if ident != nil {
 			if sel.TestOnly {
 				c.errors.expressionDoesNotSelectField(c.location(e))
 				c.setType(e, decls.Bool)
-			} else {
-				c.setType(e, ident.GetIdent().Type)
-				c.setReference(e,
-					newIdentReference(ident.Name, ident.GetIdent().Value))
+				return
+			}
+			// Rewrite the node to be a variable reference to the resolved fully-qualified
+			// variable name.
+			c.setType(e, ident.GetIdent().Type)
+			c.setReference(e, newIdentReference(ident.GetName(), ident.GetIdent().Value))
+			identName := ident.GetName()
+			e.ExprKind = &exprpb.Expr_IdentExpr{
+				IdentExpr: &exprpb.Expr_Ident{
+					Name: identName,
+				},
 			}
 			return
 		}
@@ -217,50 +228,82 @@ func (c *checker) checkSelect(e *exprpb.Expr) {
 }
 
 func (c *checker) checkCall(e *exprpb.Expr) {
+	// Note: similar logic exists within the `interpreter/planner.go`. If making changes here
+	// please consider the impact on planner.go and consolidate implementations or mirror code
+	// as appropriate.
 	call := e.GetCallExpr()
+	target := call.GetTarget()
+	args := call.GetArgs()
+	fnName := call.GetFunction()
+
 	// Traverse arguments.
-	for _, arg := range call.Args {
+	for _, arg := range args {
 		c.check(arg)
 	}
 
-	var resolution *overloadResolution
-
-	if call.Target == nil {
-		// Regular static call with simple name.
-		if fn := c.env.LookupFunction(call.Function); fn != nil {
-			resolution = c.resolveOverload(c.location(e), fn, nil, call.Args)
-		} else {
+	// Regular static call with simple name.
+	if target == nil {
+		// Check for the existence of the function.
+		fn := c.env.LookupFunction(fnName)
+		if fn == nil {
 			c.errors.undeclaredReference(
-				c.location(e), c.env.packager.Package(), call.Function)
-		}
-	} else {
-		// Check whether the target is actually a qualified name for a static function.
-		if qname, found := toQualifiedName(call.Target); found {
-			fn := c.env.LookupFunction(qname + "." + call.Function)
-			if fn != nil {
-				resolution = c.resolveOverload(c.location(e), fn, nil, call.Args)
-			}
+				c.location(e), c.env.packager.Package(), fnName)
+			c.setType(e, decls.Error)
+			return
 		}
+		// Overwrite the function name with its fully qualified resolved name.
+		call.Function = fn.GetName()
+		// Check to see whether the overload resolves.
+		c.resolveOverloadOrError(c.location(e), e, fn, nil, args)
+		return
+	}
 
-		if resolution == nil {
-			// Regular instance call.
-			c.check(call.Target)
-
-			if fn := c.env.LookupFunction(call.Function); fn != nil {
-				resolution = c.resolveOverload(c.location(e), fn, call.Target, call.Args)
-			} else {
-				c.errors.undeclaredReference(
-					c.location(e), c.env.packager.Package(), call.Function)
-			}
+	// If a receiver 'target' is present, it may either be a receiver function, or a namespaced
+	// function, but not both. Given a.b.c() either a.b.c is a function or c is a function with
+	// target a.b.
+	//
+	// Check whether the target is a namespaced function name.
+	qualifiedPrefix, maybeQualified := packages.ToQualifiedName(target)
+	if maybeQualified {
+		maybeQualifiedName := qualifiedPrefix + "." + fnName
+		fn := c.env.LookupFunction(maybeQualifiedName)
+		if fn != nil {
+			// The function name is namespaced and so preserving the target operand would
+			// be an inaccurate representation of the desired evaluation behavior.
+			// Overwrite with fully-qualified resolved function name sans receiver target.
+			call.Target = nil
+			call.Function = fn.GetName()
+			c.resolveOverloadOrError(c.location(e), e, fn, nil, args)
+			return
 		}
 	}
 
-	if resolution != nil {
-		c.setType(e, resolution.Type)
-		c.setReference(e, resolution.Reference)
-	} else {
+	// Regular instance call.
+	c.check(call.Target)
+	fn := c.env.LookupFunction(fnName)
+	// Function found, attempt overload resolution.
+	if fn != nil {
+		c.resolveOverloadOrError(c.location(e), e, fn, target, args)
+		return
+	}
+	// Function name not declared, record error.
+	c.errors.undeclaredReference(c.location(e), c.env.packager.Package(), fnName)
+}
+
+func (c *checker) resolveOverloadOrError(
+	loc common.Location,
+	e *exprpb.Expr,
+	fn *exprpb.Decl, target *exprpb.Expr, args []*exprpb.Expr) {
+	// Attempt to resolve the overload.
+	resolution := c.resolveOverload(loc, fn, target, args)
+	// No such overload, error noted in the resolveOverload call, type recorded here.
+	if resolution == nil {
 		c.setType(e, decls.Error)
+		return
 	}
+	// Overload found.
+	c.setType(e, resolution.Type)
+	c.setReference(e, resolution.Reference)
 }
 
 func (c *checker) resolveOverload(
@@ -315,7 +358,7 @@ func (c *checker) resolveOverload(
 	}
 
 	if resultType == nil {
-		c.errors.noMatchingOverload(loc, fn.Name, argTypes, target != nil)
+		c.errors.noMatchingOverload(loc, fn.GetName(), argTypes, target != nil)
 		resultType = decls.Error
 		return nil
 	}
@@ -376,8 +419,9 @@ func (c *checker) checkCreateMessage(e *exprpb.Expr) {
 			c.location(e), c.env.packager.Package(), msgVal.MessageName)
 		return
 	}
-
-	c.setReference(e, newIdentReference(decl.Name, nil))
+	// Ensure the type name is fully qualified in the AST.
+	msgVal.MessageName = decl.GetName()
+	c.setReference(e, newIdentReference(decl.GetName(), nil))
 	ident := decl.GetIdent()
 	identKind := kindOf(ident.Type)
 	if identKind != kindError {
@@ -590,20 +634,3 @@ func newIdentReference(name string, value *exprpb.Constant) *exprpb.Reference {
 func newFunctionReference(overloads ...string) *exprpb.Reference {
 	return &exprpb.Reference{OverloadId: overloads}
 }
-
-// Attempt to interpret an expression as a qualified name. This traverses select and getIdent
-// expression and returns the name they constitute, or null if the expression cannot be
-// interpreted like this.
-func toQualifiedName(e *exprpb.Expr) (string, bool) {
-	switch e.ExprKind.(type) {
-	case *exprpb.Expr_IdentExpr:
-		i := e.GetIdentExpr()
-		return i.Name, true
-	case *exprpb.Expr_SelectExpr:
-		s := e.GetSelectExpr()
-		if qname, found := toQualifiedName(s.Operand); found {
-			return qname + "." + s.Field, true
-		}
-	}
-	return "", false
-}
diff --git a/checker/checker_test.go b/checker/checker_test.go
@@ -222,9 +222,10 @@ ERROR: <input>:1:1: undeclared reference to 'foo' (in container '')
 		I:         `TestAllTypes{single_int32: 1, single_int64: 2}`,
 		Container: "google.expr.proto3.test",
 		R: `
-	TestAllTypes{single_int32 : 1~int, single_int64 : 2~int}
-	  ~google.expr.proto3.test.TestAllTypes
-	    ^google.expr.proto3.test.TestAllTypes`,
+		google.expr.proto3.test.TestAllTypes{
+			single_int32 : 1~int,
+			single_int64 : 2~int
+		}~google.expr.proto3.test.TestAllTypes^google.expr.proto3.test.TestAllTypes`,
 		Type: decls.NewObjectType("google.expr.proto3.test.TestAllTypes"),
 	},
 	{
@@ -396,7 +397,7 @@ _!=_(_-_(_+_(1~double, _*_(2~double, 3~double)~double^multiply_double)
 	{
 		I:         `TestAllTypes.NestedEnum.BAR != 99`,
 		Container: "google.expr.proto3.test",
-		R: `_!=_(TestAllTypes.NestedEnum.BAR
+		R: `_!=_(google.expr.proto3.test.TestAllTypes.NestedEnum.BAR
 	     ~int^google.expr.proto3.test.TestAllTypes.NestedEnum.BAR,
 	    99~int)
 	~bool^not_equals`,
@@ -1005,7 +1006,7 @@ ERROR: <input>:1:6: found no matching overload for '_&&_' applied to '(bool, int
 
 	{
 		I: `.google.expr.proto3.test.TestAllTypes`,
-		R: `	.google.expr.proto3.test.TestAllTypes
+		R: `google.expr.proto3.test.TestAllTypes
 	~type(google.expr.proto3.test.TestAllTypes)
 	^google.expr.proto3.test.TestAllTypes`,
 		Type: decls.NewTypeType(
@@ -1016,7 +1017,7 @@ ERROR: <input>:1:6: found no matching overload for '_&&_' applied to '(bool, int
 		I:         `test.TestAllTypes`,
 		Container: "google.expr.proto3",
 		R: `
-	test.TestAllTypes
+	google.expr.proto3.test.TestAllTypes
 	~type(google.expr.proto3.test.TestAllTypes)
 	^google.expr.proto3.test.TestAllTypes
 		`,
@@ -1088,7 +1089,7 @@ ERROR: <input>:1:5: undeclared reference to 'x' (in container '')
 				decls.NewIdent("container.x", decls.NewObjectType("google.expr.proto3.test.TestAllTypes"), nil),
 			},
 		},
-		R:    `x~google.expr.proto3.test.TestAllTypes^container.x`,
+		R:    `container.x~google.expr.proto3.test.TestAllTypes^container.x`,
 		Type: decls.NewObjectType("google.expr.proto3.test.TestAllTypes"),
 	},
 
@@ -1504,7 +1505,10 @@ _&&_(_==_(list~type(list(dyn))^list,
 	{
 		I:         `TestAllTypes{}.repeated_nested_message`,
 		Container: "google.expr.proto2.test",
-		R:         `TestAllTypes{}~google.expr.proto2.test.TestAllTypes^google.expr.proto2.test.TestAllTypes.repeated_nested_message~list(google.expr.proto2.test.TestAllTypes.NestedMessage)`,
+		R: `
+		google.expr.proto2.test.TestAllTypes{}~google.expr.proto2.test.TestAllTypes^
+		google.expr.proto2.test.TestAllTypes.repeated_nested_message
+		~list(google.expr.proto2.test.TestAllTypes.NestedMessage)`,
 		Type: decls.NewListType(
 			decls.NewObjectType(
 				"google.expr.proto2.test.TestAllTypes.NestedMessage",
@@ -1514,13 +1518,51 @@ _&&_(_==_(list~type(list(dyn))^list,
 	{
 		I:         `TestAllTypes{}.repeated_nested_message`,
 		Container: "google.expr.proto3.test",
-		R:         `TestAllTypes{}~google.expr.proto3.test.TestAllTypes^google.expr.proto3.test.TestAllTypes.repeated_nested_message~list(google.expr.proto3.test.TestAllTypes.NestedMessage)`,
+		R: `
+		google.expr.proto3.test.TestAllTypes{}~google.expr.proto3.test.TestAllTypes^
+		google.expr.proto3.test.TestAllTypes.repeated_nested_message
+		~list(google.expr.proto3.test.TestAllTypes.NestedMessage)`,
 		Type: decls.NewListType(
 			decls.NewObjectType(
 				"google.expr.proto3.test.TestAllTypes.NestedMessage",
 			),
 		),
 	},
+	{
+		I: `base64.encode('hello')`,
+		Env: env{
+			functions: []*exprpb.Decl{
+				decls.NewFunction("base64.encode",
+					decls.NewOverload(
+						"base64_encode_string",
+						[]*exprpb.Type{decls.String},
+						decls.String)),
+			},
+		},
+		R: `
+		base64.encode(
+			"hello"~string
+		)~string^base64_encode_string`,
+		Type: decls.String,
+	},
+	{
+		I:         `encode('hello')`,
+		Container: `base64`,
+		Env: env{
+			functions: []*exprpb.Decl{
+				decls.NewFunction("base64.encode",
+					decls.NewOverload(
+						"base64_encode_string",
+						[]*exprpb.Type{decls.String},
+						decls.String)),
+			},
+		},
+		R: `
+		base64.encode(
+			"hello"~string
+		)~string^base64_encode_string`,
+		Type: decls.String,
+	},
 }
 
 var testEnvs = map[string]env{

diff --git a/common/packages/BUILD.bazel b/common/packages/BUILD.bazel
@@ -11,4 +11,7 @@ go_library(
     srcs = [
         "packager.go",
     ],
+     deps = [
+        "@org_golang_google_genproto//googleapis/api/expr/v1alpha1:go_default_library",
+     ],
 )
diff --git a/common/packages/packager.go b/common/packages/packager.go
@@ -19,6 +19,8 @@ package packages
 
 import (
 	"strings"
+
+	exprpb "google.golang.org/genproto/googleapis/api/expr/v1alpha1"
 )
 
 // Packager helps interpret qualified names.
@@ -80,3 +82,19 @@ func (p *defaultPackage) ResolveCandidateNames(name string) []string {
 	}
 	return append(candidates, name)
 }
+
+// ToQualifiedName converts an expression AST into a qualified name if possible, with a boolean
+// 'found' value that indicates if the conversion is successful.
+func ToQualifiedName(e *exprpb.Expr) (string, bool) {
+	switch e.ExprKind.(type) {
+	case *exprpb.Expr_IdentExpr:
+		id := e.GetIdentExpr()
+		return id.Name, true
+	case *exprpb.Expr_SelectExpr:
+		sel := e.GetSelectExpr()
+		if qual, found := ToQualifiedName(sel.Operand); found {
+			return qual + "." + sel.Field, true
+		}
+	}
+	return "", false
+}