From d45f67fee20b8ecd7eacf1747bd60e0d8f4c4835 Mon Sep 17 00:00:00 2001 From: Tristan Swadell Date: Sat, 6 Jan 2024 20:52:43 -0800 Subject: [PATCH] Fix the optional type name and type identifier resolution (#870) --- cel/cel_test.go | 4 +++ checker/checker_test.go | 55 +++++++++++++++++++++++++++----------- checker/decls/decls.go | 2 +- checker/env.go | 8 ++++++ checker/types.go | 2 +- common/types/optional.go | 2 +- common/types/types.go | 2 +- common/types/types_test.go | 2 +- 8 files changed, 57 insertions(+), 20 deletions(-) diff --git a/cel/cel_test.go b/cel/cel_test.go index 97b81ef4..f386b13b 100644 --- a/cel/cel_test.go +++ b/cel/cel_test.go @@ -2365,6 +2365,10 @@ func TestOptionalValuesEval(t *testing.T) { }, out: true, }, + { + expr: `type(optional.none()) == optional_type`, + out: true, + }, { // return the value of m.c['dashed-index'], no magic in the optional.of() call. expr: `optional.ofNonZeroValue('').or(optional.of(m.c['dashed-index'])).orValue('default value')`, diff --git a/checker/checker_test.go b/checker/checker_test.go index da69bd89..b3090477 100644 --- a/checker/checker_test.go +++ b/checker/checker_test.go @@ -2044,7 +2044,26 @@ _&&_(_==_(list~type(list(dyn))^list, out: `_?._( a~map(string, string)^a, "b" - )~optional(string)^select_optional_field`, + )~optional_type(string)^select_optional_field`, + }, + { + in: `type(a.?b) == optional_type`, + env: testEnv{ + optionalSyntax: true, + idents: []*decls.VariableDecl{ + decls.NewVariable("a", types.NewMapType(types.StringType, types.StringType)), + }, + }, + outType: types.BoolType, + out: `_==_( + type( + _?._( + a~map(string, string)^a, + "b" + )~optional_type(string)^select_optional_field + )~type(optional_type(string))^type, + optional_type~type(optional_type)^optional_type + )~bool^equals`, }, { in: `a.b`, @@ -2054,7 +2073,7 @@ _&&_(_==_(list~type(list(dyn))^list, }, }, outType: types.NewOptionalType(types.StringType), - out: `a~optional(map(string, string))^a.b~optional(string)`, + out: `a~optional_type(map(string, string))^a.b~optional_type(string)`, }, { in: `a.dynamic`, @@ -2064,7 +2083,7 @@ _&&_(_==_(list~type(list(dyn))^list, }, }, outType: types.NewOptionalType(types.DynType), - out: `a~optional(dyn)^a.dynamic~optional(dyn)`, + out: `a~optional_type(dyn)^a.dynamic~optional_type(dyn)`, }, { in: `has(a.dynamic)`, @@ -2074,7 +2093,7 @@ _&&_(_==_(list~type(list(dyn))^list, }, }, outType: types.BoolType, - out: `a~optional(dyn)^a.dynamic~test-only~~bool`, + out: `a~optional_type(dyn)^a.dynamic~test-only~~bool`, }, { in: `has(a.?b.c)`, @@ -2086,9 +2105,9 @@ _&&_(_==_(list~type(list(dyn))^list, }, outType: types.BoolType, out: `_?._( - a~optional(map(string, dyn))^a, + a~optional_type(map(string, dyn))^a, "b" - )~optional(dyn)^select_optional_field.c~test-only~~bool`, + )~optional_type(dyn)^select_optional_field.c~test-only~~bool`, }, { in: `{?'key': {'a': 'b'}.?value}`, @@ -2100,7 +2119,7 @@ _&&_(_==_(list~type(list(dyn))^list, "a"~string:"b"~string }~map(string, string), "value" - )~optional(string)^select_optional_field + )~optional_type(string)^select_optional_field }~map(string, string)`, }, { @@ -2113,7 +2132,7 @@ _&&_(_==_(list~type(list(dyn))^list, "a"~string:"b"~string }~map(string, string), "value" - )~optional(string)^select_optional_field + )~optional_type(string)^select_optional_field }~map(string, string).key~string`, }, { @@ -2126,13 +2145,13 @@ _&&_(_==_(list~type(list(dyn))^list, }, outType: types.NewMapType(types.StringType, types.StringType), out: `{ - ?"nested"~string:a~optional(map(string, string))^a.b~optional(string) + ?"nested"~string:a~optional_type(map(string, string))^a.b~optional_type(string) }~map(string, string)`, }, { in: `{?'key': 'hi'}`, env: testEnv{optionalSyntax: true}, - err: `ERROR: :1:10: expected type 'optional(string)' but found 'string' + err: `ERROR: :1:10: expected type 'optional_type(string)' but found 'string' | {?'key': 'hi'} | .........^`, }, @@ -2147,15 +2166,15 @@ _&&_(_==_(list~type(list(dyn))^list, }, outType: types.NewListType(types.StringType), out: `[ - a~optional(string)^a, - b~optional(string)^b, + a~optional_type(string)^a, + b~optional_type(string)^b, "world"~string ]~list(string)`, }, { in: `[?'value']`, env: testEnv{optionalSyntax: true}, - err: `ERROR: :1:3: expected type 'optional(string)' but found 'string' + err: `ERROR: :1:3: expected type 'optional_type(string)' but found 'string' | [?'value'] | ..^`, }, @@ -2167,7 +2186,7 @@ _&&_(_==_(list~type(list(dyn))^list, ?single_int32:_?._( {}~map(dyn, int), "i" - )~optional(int)^select_optional_field + )~optional_type(int)^select_optional_field }~google.expr.proto2.test.TestAllTypes^google.expr.proto2.test.TestAllTypes`, outType: types.NewObjectType( "google.expr.proto2.test.TestAllTypes", @@ -2177,7 +2196,7 @@ _&&_(_==_(list~type(list(dyn))^list, in: `TestAllTypes{?single_int32: 1}`, container: "google.expr.proto2.test", env: testEnv{optionalSyntax: true}, - err: `ERROR: :1:29: expected type 'optional(int)' but found 'int' + err: `ERROR: :1:29: expected type 'optional_type(int)' but found 'int' | TestAllTypes{?single_int32: 1} | ............................^`, }, @@ -2307,6 +2326,12 @@ func TestCheck(t *testing.T) { } reg, err := types.NewRegistry(&proto2pb.TestAllTypes{}, &proto3pb.TestAllTypes{}) + if tc.env.optionalSyntax { + err = reg.RegisterType(types.OptionalType) + if err != nil { + t.Fatalf("reg.RegisterType(optional_type) failed: %v", err) + } + } if err != nil { t.Fatalf("types.NewRegistry() failed: %v", err) } diff --git a/checker/decls/decls.go b/checker/decls/decls.go index 0d91bef5..c0e5de46 100644 --- a/checker/decls/decls.go +++ b/checker/decls/decls.go @@ -67,7 +67,7 @@ func NewAbstractType(name string, paramTypes ...*exprpb.Type) *exprpb.Type { // NewOptionalType constructs an abstract type indicating that the parameterized type // may be contained within the object. func NewOptionalType(paramType *exprpb.Type) *exprpb.Type { - return NewAbstractType("optional", paramType) + return NewAbstractType("optional_type", paramType) } // NewFunctionType creates a function invocation contract, typically only used diff --git a/checker/env.go b/checker/env.go index 70682b17..d5ac0501 100644 --- a/checker/env.go +++ b/checker/env.go @@ -146,6 +146,14 @@ func (e *Env) LookupIdent(name string) *decls.VariableDecl { return decl } + if i, found := e.provider.FindIdent(candidate); found { + if t, ok := i.(*types.Type); ok { + decl := decls.NewVariable(candidate, types.NewTypeTypeWithParam(t)) + e.declarations.AddIdent(decl) + return decl + } + } + // Next try to import this as an enum value by splitting the name in a type prefix and // the enum inside. if enumValue := e.provider.EnumValue(candidate); enumValue.Type() != types.ErrType { diff --git a/checker/types.go b/checker/types.go index ed51beed..1aad3cce 100644 --- a/checker/types.go +++ b/checker/types.go @@ -41,7 +41,7 @@ func isError(t *types.Type) bool { func isOptional(t *types.Type) bool { if t.Kind() == types.OpaqueKind { - return t.TypeName() == "optional" + return t.TypeName() == "optional_type" } return false } diff --git a/common/types/optional.go b/common/types/optional.go index a9f30aed..97845a74 100644 --- a/common/types/optional.go +++ b/common/types/optional.go @@ -24,7 +24,7 @@ import ( var ( // OptionalType indicates the runtime type of an optional value. - OptionalType = NewOpaqueType("optional") + OptionalType = NewOpaqueType("optional_type") // OptionalNone is a sentinel value which is used to indicate an empty optional value. OptionalNone = &Optional{} diff --git a/common/types/types.go b/common/types/types.go index aef5c693..6c3d5f71 100644 --- a/common/types/types.go +++ b/common/types/types.go @@ -513,7 +513,7 @@ func NewNullableType(wrapped *Type) *Type { // NewOptionalType creates an abstract parameterized type instance corresponding to CEL's notion of optional. func NewOptionalType(param *Type) *Type { - return NewOpaqueType("optional", param) + return NewOpaqueType("optional_type", param) } // NewOpaqueType creates an abstract parameterized type with a given name. diff --git a/common/types/types_test.go b/common/types/types_test.go index 4a0e3709..a94fc29a 100644 --- a/common/types/types_test.go +++ b/common/types/types_test.go @@ -59,7 +59,7 @@ func TestTypeString(t *testing.T) { }, { in: NewOptionalType(NewListType(StringType)), - out: "optional(list(string))", + out: "optional_type(list(string))", }, { in: NewObjectType("my.type.Message"),