google · TristonianJones · Mar 24, 2022 · Apr 30, 2020 · Apr 30, 2020 · Apr 30, 2020
diff --git a/cel/BUILD.bazel b/cel/BUILD.bazel
@@ -46,6 +46,7 @@ go_test(
     srcs = [
         "cel_example_test.go",
         "cel_test.go",
+        "env_test.go",
         "io_test.go",
     ],
     data = [
@@ -67,6 +68,7 @@ go_test(
         "//test/proto3pb:go_default_library",
         "@io_bazel_rules_go//proto/wkt:descriptor_go_proto",
         "@org_golang_google_genproto//googleapis/api/expr/v1alpha1:go_default_library",
+        "@org_golang_google_protobuf//proto:go_default_library",
         "@org_golang_google_protobuf//types/known/structpb:go_default_library",
     ],
 )
diff --git a/cel/env.go b/cel/env.go
@@ -96,6 +96,7 @@ type Env struct {
 	chk     *checker.Env
 	chkErr  error
 	chkOnce sync.Once
+	chkOpts []checker.Option
 
 	// Program options tied to the environment
 	progOpts []ProgramOption
@@ -108,8 +109,12 @@ type Env struct {
 // See the EnvOption helper functions for the options that can be used to configure the
 // environment.
 func NewEnv(opts ...EnvOption) (*Env, error) {
-	stdOpts := append([]EnvOption{StdLib()}, opts...)
-	return NewCustomEnv(stdOpts...)
+	// Extend the statically configured standard environment, disabling eager validation to ensure
+	// the cost of setup for the environment is still just as cheap as it is in v0.11.x and earlier
+	// releases. The user provided options can easily re-enable the eager validation as they are
+	// processed after this default option.
+	stdOpts := append([]EnvOption{EagerlyValidateDeclarations(false)}, opts...)
+	return stdEnv.Extend(stdOpts...)
 }
 
 // NewCustomEnv creates a custom program environment which is not automatically configured with the
@@ -150,25 +155,8 @@ func (e *Env) Check(ast *Ast) (*Ast, *Issues) {
 	pe, _ := AstToParsedExpr(ast)
 
 	// Construct the internal checker env, erroring if there is an issue adding the declarations.
-	e.chkOnce.Do(func() {
-		ce, err := checker.NewEnv(e.Container, e.provider,
-			checker.HomogeneousAggregateLiterals(
-				e.HasFeature(featureDisableDynamicAggregateLiterals)),
-			checker.CrossTypeNumericComparisons(
-				e.HasFeature(featureCrossTypeNumericComparisons)))
-		if err != nil {
-			e.chkErr = err
-			return
-		}
-		err = ce.Add(e.declarations...)
-		if err != nil {
-			e.chkErr = err
-			return
-		}
-		e.chk = ce
-	})
-	// The once call will ensure that this value is set or nil for all invocations.
-	if e.chkErr != nil {
+	err := e.initChecker()
+	if err != nil {
 		errs := common.NewErrors(ast.Source())
 		errs.ReportError(common.NoLocation, e.chkErr.Error())
 		return nil, NewIssues(errs)
@@ -231,11 +219,29 @@ func (e *Env) Extend(opts ...EnvOption) (*Env, error) {
 	if e.chkErr != nil {
 		return nil, e.chkErr
 	}
-	// Copy slices.
-	decsCopy := make([]*exprpb.Decl, len(e.declarations))
+
+	// The type-checker is configured with Declarations. The declarations may either be provided
+	// as options which have not yet been validated, or may come from a previous checker instance
+	// whose types have already been validated.
+	chkOptsCopy := make([]checker.Option, len(e.chkOpts))
+	copy(chkOptsCopy, e.chkOpts)
+
+	// Copy the declarations if needed.
+	decsCopy := []*exprpb.Decl{}
+	if e.chk != nil {
+		// If the type-checker has already been instantiated, then the e.declarations have been
+		// valdiated within the chk instance.
+		chkOptsCopy = append(chkOptsCopy, checker.ValidatedDeclarations(e.chk))
+	} else {
+		// If the type-checker has not been instantiated, ensure the unvalidated declarations are
+		// provided to the extended Env instance.
+		decsCopy = make([]*exprpb.Decl, len(e.declarations))
+		copy(decsCopy, e.declarations)
+	}
+
+	// Copy macros and program options
 	macsCopy := make([]parser.Macro, len(e.macros))
 	progOptsCopy := make([]ProgramOption, len(e.progOpts))
-	copy(decsCopy, e.declarations)
 	copy(macsCopy, e.macros)
 	copy(progOptsCopy, e.progOpts)
 
@@ -279,6 +285,7 @@ func (e *Env) Extend(opts ...EnvOption) (*Env, error) {
 		adapter:      adapter,
 		features:     featuresCopy,
 		provider:     provider,
+		chkOpts:      chkOptsCopy,
 	}
 	return ext.configure(opts)
 }
@@ -424,6 +431,8 @@ func (e *Env) configure(opts []EnvOption) (*Env, error) {
 			return nil, err
 		}
 	}
+
+	// Configure the parser.
 	prsrOpts := []parser.Option{parser.Macros(e.macros...)}
 	if e.HasFeature(featureEnableMacroCallTracking) {
 		prsrOpts = append(prsrOpts, parser.PopulateMacroCalls(true))
@@ -432,9 +441,44 @@ func (e *Env) configure(opts []EnvOption) (*Env, error) {
 	if err != nil {
 		return nil, err
 	}
+
+	// The simplest way to eagerly validate declarations on environment creation is to compile
+	// a dummy program and check for the presence of e.chkErr being non-nil.
+	if e.HasFeature(featureEagerlyValidateDeclarations) {
+		err := e.initChecker()
+		if err != nil {
+			return nil, err
+		}
+	}
+
 	return e, nil
 }
 
+func (e *Env) initChecker() error {
+	e.chkOnce.Do(func() {
+		chkOpts := []checker.Option{}
+		chkOpts = append(chkOpts, e.chkOpts...)
+		chkOpts = append(chkOpts,
+			checker.HomogeneousAggregateLiterals(
+				e.HasFeature(featureDisableDynamicAggregateLiterals)),
+			checker.CrossTypeNumericComparisons(
+				e.HasFeature(featureCrossTypeNumericComparisons)))
+
+		ce, err := checker.NewEnv(e.Container, e.provider, chkOpts...)
+		if err != nil {
+			e.chkErr = err
+			return
+		}
+		err = ce.Add(e.declarations...)
+		if err != nil {
+			e.chkErr = err
+			return
+		}
+		e.chk = ce
+	})
+	return e.chkErr
+}
+
 // Issues defines methods for inspecting the error details of parse and check calls.
 //
 // Note: in the future, non-fatal warnings and notices may be inspectable via the Issues struct.
@@ -486,3 +530,13 @@ func (i *Issues) String() string {
 	}
 	return i.errs.ToDisplayString()
 }
+
+var stdEnv *Env
+
+func init() {
+	var err error
+	stdEnv, err = NewCustomEnv(StdLib(), EagerlyValidateDeclarations(true))
+	if err != nil {
+		panic(err)
+	}
+}
diff --git a/cel/env_test.go b/cel/env_test.go
@@ -18,7 +18,12 @@ import (
 	"reflect"
 	"testing"
 
+	"github.com/google/cel-go/checker/decls"
 	"github.com/google/cel-go/common"
+	"github.com/google/cel-go/common/operators"
+	"github.com/google/cel-go/common/types"
+
+	exprpb "google.golang.org/genproto/googleapis/api/expr/v1alpha1"
 )
 
 func TestIssuesNil(t *testing.T) {
@@ -79,3 +84,104 @@ ERROR: <input>:1:2: Syntax error: mismatched input '<EOF>' expecting {'[', '{',
 		t.Errorf("iss.String() returned %v, wanted %v", iss.String(), wantIss)
 	}
 }
+
+func BenchmarkNewCustomEnvLazy(b *testing.B) {
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		_, err := NewCustomEnv(StdLib(), EagerlyValidateDeclarations(false))
+		if err != nil {
+			b.Fatalf("NewCustomEnv() failed: %v", err)
+		}
+	}
+}
+
+func BenchmarkNewCustomEnvEager(b *testing.B) {
+	for i := 0; i < b.N; i++ {
+		_, err := NewCustomEnv(StdLib(), EagerlyValidateDeclarations(true))
+		if err != nil {
+			b.Fatalf("NewCustomEnv() failed: %v", err)
+		}
+	}
+}
+
+func BenchmarkNewEnvLazy(b *testing.B) {
+	for i := 0; i < b.N; i++ {
+		_, err := NewEnv()
+		if err != nil {
+			b.Fatalf("NewEnv() failed: %v", err)
+		}
+	}
+}
+
+func BenchmarkNewEnvEager(b *testing.B) {
+	for i := 0; i < b.N; i++ {
+		env, err := NewEnv()
+		if err != nil {
+			b.Fatalf("NewEnv() failed: %v", err)
+		}
+		_, iss := env.Compile("123")
+		if iss.Err() != nil {
+			b.Fatalf("env.Compile(123) failed: %v", iss.Err())
+		}
+	}
+}
+
+func BenchmarkEnvExtendEager(b *testing.B) {
+	env, err := NewEnv()
+	if err != nil {
+		b.Fatalf("NewEnv() failed: %v", err)
+	}
+	for i := 0; i < b.N; i++ {
+		ext, err := env.Extend()
+		if err != nil {
+			b.Fatalf("env.Extend() failed: %v", err)
+		}
+		_, iss := ext.Compile("123")
+		if iss.Err() != nil {
+			b.Fatalf("env.Compile(123) failed: %v", iss.Err())
+		}
+	}
+}
+
+func BenchmarkEnvExtendEagerTypes(b *testing.B) {
+	env, err := NewEnv(EagerlyValidateDeclarations(true))
+	if err != nil {
+		b.Fatalf("NewEnv() failed: %v", err)
+	}
+	for i := 0; i < b.N; i++ {
+		ext, err := env.Extend(CustomTypeProvider(types.NewEmptyRegistry()))
+		if err != nil {
+			b.Fatalf("env.Extend() failed: %v", err)
+		}
+		_, iss := ext.Compile("123")
+		if iss.Err() != nil {
+			b.Fatalf("env.Compile(123) failed: %v", iss.Err())
+		}
+	}
+}
+
+func BenchmarkEnvExtendEagerDecls(b *testing.B) {
+	env, err := NewEnv(EagerlyValidateDeclarations(true))
+	if err != nil {
+		b.Fatalf("NewEnv() failed: %v", err)
+	}
+	for i := 0; i < b.N; i++ {
+		ext, err := env.Extend(
+			Declarations(
+				decls.NewVar("test_var", decls.String),
+				decls.NewFunction(
+					operators.In,
+					decls.NewOverload("string_in_string", []*exprpb.Type{
+						decls.String, decls.String,
+					}, decls.Bool)),
+			),
+		)
+		if err != nil {
+			b.Fatalf("env.Extend() failed: %v", err)
+		}
+		_, iss := ext.Compile("123")
+		if iss.Err() != nil {
+			b.Fatalf("env.Compile(123) failed: %v", iss.Err())
+		}
+	}
+}
diff --git a/cel/options.go b/cel/options.go
@@ -53,6 +53,10 @@ const (
 
 	// Enable the use of cross-type numeric comparisons at the type-checker.
 	featureCrossTypeNumericComparisons
+
+	// Enable eager validation of declarations to ensure that Env values created
+	// with `Extend` inherit a validated list of declarations from the parent Env.
+	featureEagerlyValidateDeclarations
 )
 
 // EnvOption is a functional interface for configuring the environment.
@@ -103,6 +107,18 @@ func Declarations(decls ...*exprpb.Decl) EnvOption {
 	}
 }
 
+// EagerlyValidateDeclarations ensures that any collisions between configured declarations are caught
+// at the time of the `NewEnv` call.
+//
+// Eagerly validating declarations is also useful for bootstrapping a base `cel.Env` value.
+// Calls to base `Env.Extend()` will be significantly faster when declarations are eagerly validated
+// as declarations will be collision-checked at most once and only incrementally by way of `Extend`
+//
+// Disabled by default as not all environments are used for type-checking.
+func EagerlyValidateDeclarations(enabled bool) EnvOption {
+	return features(featureEagerlyValidateDeclarations, enabled)
+}
+
 // HomogeneousAggregateLiterals option ensures that list and map literal entry types must agree
 // during type-checking.
 //

diff --git a/checker/checker.go b/checker/checker.go
@@ -168,11 +168,9 @@ func (c *checker) checkSelect(e *exprpb.Expr) {
 	if found {
 		ident := c.env.LookupIdent(qname)
 		if ident != nil {
-			if sel.TestOnly {
-				c.errors.expressionDoesNotSelectField(c.location(e))
-				c.setType(e, decls.Bool)
-				return
-			}
+			// We don't check for a TestOnly expression here since the `found` result is
+			// always going to be false for TestOnly expressions.
+
 			// Rewrite the node to be a variable reference to the resolved fully-qualified
 			// variable name.
 			c.setType(e, ident.GetIdent().Type)
@@ -323,33 +321,39 @@ func (c *checker) resolveOverload(
 	var resultType *exprpb.Type
 	var checkedRef *exprpb.Reference
 	for _, overload := range fn.GetFunction().Overloads {
+		// Determine whether the overload is currently considered.
+		if c.env.isOverloadDisabled(overload.GetOverloadId()) {
+			continue
+		}
+
+		// Ensure the call style for the overload matches.
 		if (target == nil && overload.IsInstanceFunction) ||
 			(target != nil && !overload.IsInstanceFunction) {
 			// not a compatible call style.
 			continue
 		}
 
 		overloadType := decls.NewFunctionType(overload.ResultType, overload.Params...)
-		if len(overload.TypeParams) > 0 {
+		if len(overload.GetTypeParams()) > 0 {
 			// Instantiate overload's type with fresh type variables.
 			substitutions := newMapping()
-			for _, typePar := range overload.TypeParams {
+			for _, typePar := range overload.GetTypeParams() {
 				substitutions.add(decls.NewTypeParamType(typePar), c.newTypeVar())
 			}
 			overloadType = substitute(substitutions, overloadType, false)
 		}
 
-		candidateArgTypes := overloadType.GetFunction().ArgTypes
+		candidateArgTypes := overloadType.GetFunction().GetArgTypes()
 		if c.isAssignableList(argTypes, candidateArgTypes) {
 			if checkedRef == nil {
-				checkedRef = newFunctionReference(overload.OverloadId)
+				checkedRef = newFunctionReference(overload.GetOverloadId())
 			} else {
-				checkedRef.OverloadId = append(checkedRef.OverloadId, overload.OverloadId)
+				checkedRef.OverloadId = append(checkedRef.OverloadId, overload.GetOverloadId())
 			}
 
 			// First matching overload, determines result type.
 			fnResultType := substitute(c.mappings,
-				overloadType.GetFunction().ResultType,
+				overloadType.GetFunction().GetResultType(),
 				false)
 			if resultType == nil {
 				resultType = fnResultType