Support for union types

[grantr]

Describe the bug
This is sort of a bug, sort of a feature request.

I'd like to use CEL with dynamic JSON objects, so I'm using google.protobuf.Struct as the type. The Struct definition doesn't distinguish between ints and floats (by design, because JSON doesn't either). CEL thus interprets all integer values as floats. This is problematic because the user expects to compare those values with constant integers, but when they try they get the no such overload error. To write a successful expression, the user must know to convert their integer constants to floats.

Here is a minimal repro: https://gist.github.com/grantr/f9064831f358dcc57c0b3816a606764d

To Reproduce
Check which components this affects:

• parser
• checker
• interpreter

Sample expression and input that reproduces the issue:

data.foo == 5

{"foo":5}

Test setup:

e, err := cel.NewEnv(
		cel.Declarations(
			decls.NewIdent("data", decls.NewObjectType("google.protobuf.Struct"), nil),
		),
	)
	if err != nil {
		log.Fatalf("environment creation error: %s\n", err)
	}

        // This doesn't work with an integer, but it works with a float: data.foo == 5.0
	p, iss := e.Parse(`data.foo == 5`)
	if iss != nil && iss.Err() != nil {
		log.Fatalln(iss.Err())
	}
	c, iss := e.Check(p)
	if iss != nil && iss.Err() != nil {
		log.Fatalln(iss.Err())
	}

	prg, err := e.Program(c)
	if err != nil {
		log.Fatalf("program creation error: %s\n", err)
	}

	data := structpb.Struct{}
	if err := jsonpb.Unmarshal(strings.NewReader(`{"foo":5}`), &data); err != nil {
		log.Fatalf("json parse error: %s\n", err)
	}

	out, _, err := prg.Eval(map[string]interface{}{
		"data": &data,
	})
	if err != nil {
		log.Fatalf("runtime error: %s\n", err)
	}

	fmt.Printf("out: %#v\n", out)

Expected behavior
I'd expect floats and ints to be freely convertible in both directions, with floats truncated when converted to ints:

float(1) -> 1.0
int(1.2) -> 1

[TristonianJones]

The code here is working as designed, so it's not really a bug, but it probably comes as a surprise if you're used to JavaScript. One of the key elements of the CEL specification is that there is no implicit numeric type conversion. If you'd like to compare a google.protobuf.Value to a numeric type you might need to convert the type of the comparison value.

// Either of the following will work
data.foo == 1.0 && int(data.foo) == 1

One of the reasons there isn't implicit numeric conversion is because this is a common source of errors. In an effort to be safe by default, the language asks the developer to be explicit about their intent.

As a feature request, I can see why this would be appealing. Since the type-checker can't infer the type of the google.protobuf.Value statically, this case isn't flagged as an error at check time. There are other scenarios, such as a recent discussion about whether to support heterogeneous equality (currently unsupported), where the runtime may have to be more flexible than the check-time. Likewise there are wrapper types which are expected to have value or null semantics. It's possible that numeric Value should be treated as a 'maybe int or float' type. We can't support implicit conversion between strong types, but with respect to dynamic types, it might be possible. A change in this direction would need to be reviewed by the language council though.

An alternative approach which yields a much nicer user experience, would be to declare your inputs as qualified names with primitive types like Istio does: https://istio.io/docs/reference/config/policy-and-telemetry/attribute-vocabulary. If that's an option, I would strongly recommend it over using purely dynamic types.

[grantr]

Thanks @TristonianJones! Your explanation makes sense.

In an effort to be safe by default, the language asks the developer to be explicit about their intent.

Perhaps my use case is unusual: I need to give users (distinct from developers) a way to filter on CloudEvent metadata. That metadata is fully dynamic according to the spec ("This specification places no restriction on the type or semantics of the extension attributes"), so there is no opportunity to declare input types in advance.

I expect that the user persona will be confused by the need to cast their integer value to an int in the expression. From their perspective, the input value is an integer, so the value in the expression should also be an integer.

We can't support implicit conversion between strong types, but with respect to dynamic types, it might be possible. A change in this direction would need to be reviewed by the language council though.

I understand the hesitation to change the language here. The root issue seems to be in the dynamic type mechanism, not the language itself.

Perhaps an EnvOption could be added that allows dynamically typed numbers to be comparable. Then it would only affect the use cases that need this functionality.

A change in this direction would need to be reviewed by the language council though.

What's the process for that? Would a PR be useful?

[TristonianJones]

Hi Grant,

Good comments and questions.

Perhaps an EnvOption could be added

I like this suggestion, but equality should probably work one way due to the support and implementation overhead. Every type would have to support a strict and non-strict comparison which would be error-prone (for the implementer at least).

The heterogeneous equality discussion in cel-spec touches on a similar concern to the one you've raised; how to avoid principal of least surprise while also retaining a safety-first posture. We've started discussing the semantic implications in the language council, but haven't come to a conclusion yet.

What's the process for [a language change]? Would a PR be useful?

The language council is a small quorum of mostly Googlers that meets semi-regularly to discuss syntactic and semantic changes. Typically, we consider feature requests filed against https://github.com/google/cel-spec and work with the community to figure out what the requirements are in an effort to reach the ideal outcome for both CEL and the person who filed the issue. Ultimately our rubric is:

• Does the change break any existing features or user expectations?
• Does the change improve the consistency of the semantics?
• Does the change preserve the runtime compute / memory complexity of CEL?

It's pretty rare that there are new requests that affect semantics, but this is one of them. Usually, we field requests for improvements in clarity, speed, and analysis. Most of those requests can just be handled by the developers working on that particular CEL stack. For example, if you'd asked for the ability to type-check JSON as a union type rather than as a dynamic type, that's something that could be handled as a feature without the involvement of the council since it's just an improvement to the static analysis tools.

Thanks for the pointers to the events spec, I'll give it a look. I'm still a bit puzzled who the user is versus who the developer is in this use case, but I'm sure it will make more sense as I read further.

[grantr]

Thanks for the detailed explanation @TristonianJones!

I'm still a bit puzzled who the user is versus who the developer is in this use case

Perhaps this is an arbitrary distinction, and maybe a better term for what I'm calling developer is contributor. This is the person who implements event filtering using CEL. This person has read the CEL docs and code and understands the nuances of its type system.

The user is the person who "just wants to filter their events". They don't care about CEL specifically, just that there is a language that can be used to express filters. On first look, this person may not have read and digested all the CEL docs. They probably don't even know that CEL is involved. Their knowledge may be based on a simple tutorial or sample focused on Knative, not CEL. This is a delicate time because if they encounter an unexplained behavior, they are more likely to abandon the effort due to lack of knowledge about how to explain it.

Despite the above concern, this issue is realistically a low priority for Knative. It's not likely to be encountered in simple filtering scenarios because all the basic fields are strings. Dynamic fields are an advanced feature, and we can explicitly point out the caveat with dynamic number types in the corresponding Knative docs.

I'm glad to hear this is already being considered by the community. Would you like me to close this issue as a dupe of #54?

[CatalystG]

@TristonianJones You mentioned wrapper types, I'm seeing a similar "no such overload" on eval (not check) when trying to compare a google.protobuf.StringValue to a literal string. Would that be expected for the same reasons mentioned above?

[TristonianJones]

Hi @CatalystG, the missing support for the wrapper types at eval time is a bug. I simply didn't implement it yet (I'll file an issue for it for the 0.3.0 release). That said, an implementation consistent with the current spec would suffer from the same runtime surprise-factor as was presented in this bug report.

Personally, I think this is good reason for heterogeneous equality, but there are a few numeric comparison cases to work through where the result should still be error.

[TristonianJones]

CEL supports a hard-coded notion of an optional value with proto wrapper types such as google.protobuf.StringValue. The optional value may either be null or <type>. JSON is similarly a union type, but it is not supported like one.

JSON is marked as the catch-all dyn type. The dyn type is insufficient to provide feedback to the user about whether their checked expressions will evaluate safely at runtime. In this case, whether the JSON value must be type-converted to an int prior to comparison.

Initially, heterogeneous equality was considered to ensure checked-expressions would evaluate according to user expectations. This would be a significant change in semantics and would have ordering implications as well. The safer and more consistent alternative is to augment type-checking to provide stronger signals of correctness at type-check time.