Add type checker #237
Add type checker #237
Conversation
| const P extends readonly CelType[], | ||
| const R extends CelType, | ||
| >( | ||
| id: string, |
There was a problem hiding this comment.
@jafaircl why was it necessary to add the ID back? We had it previously, but didn't find it useful.
There was a problem hiding this comment.
It’s used when resolving overloads to determine if the overload should be disabled (i.e. cross type numeric comparisons or user defined disabled overloads). See the resolveOverload method on line 522 in checker.ts. It’s also needed for reference info which is part of the cel spec (https://github.com/google/cel-spec/blob/121e265b0c5e1d4c7d5c140b33d6048fec754c77/proto/cel/expr/checked.proto#L387)
There was a problem hiding this comment.
The latter may be unavoidable, but I was already inclined to question the need for configurability on cross-type numeric comparisons. The spec itself is unambiguous, and I assume the Google implementations only provide it for historical reasons. What do you think?
There was a problem hiding this comment.
I think there may be valid reasons in some environments to disable cross type numeric comparisons. Since it's part of the spec, I'm inclined to believe there's a reason it hasn't been removed or marked as deprecated. Maybe that's historical. But, it could also be used again in the future. Since every other reference implementation includes it, I would be hesitant to remove it.
There was a problem hiding this comment.
I removed the id as it was not used at the time. Because we have all the types, can we derive the id?
| * The environment defines the functions and types that are available | ||
| * during CEL expression checking. | ||
| */ | ||
| export interface CelCheckerEnv { |
There was a problem hiding this comment.
I recognize this may be painful, so feel free to push back (and I'd like to hear @timostamm and/or @srikrsna-buf's thoughts) but I'd like to avoid having multiple "environment" implementations. cel-go is really hard to work with, in part because for any given noun there are seven different abstractions all related to each other in some convoluted way.
For cel-es if we could follow the rule that "an environment is an environment" I think we'd be better for it.
The ontological relationships of "declarations," "scopes," "groups," "namespaces," and "registries" is also going to be a very steep learning curve for the uninitiated. Let's try to narrow the terminology and use the terms very crisply if possible.
| /** | ||
| * Idents available in this environment. | ||
| */ | ||
| idents?: CelIdent[]; |
| /** | ||
| * Whether to allow cross-type numeric comparisons. | ||
| */ | ||
| crossTypeNumericComparisons?: boolean; |
There was a problem hiding this comment.
As mentioned on the other thread, I'm not convinced we need this flexibility.
| /** | ||
| * AddIdents configures the checker with a list of variable declarations. | ||
| * | ||
| * If there are overlapping declarations, the method will error. | ||
| */ | ||
| addIdents(idents: CelIdent[]): void { | ||
| let errMsgs: string[] = []; | ||
| for (const ident of idents) { | ||
| const errMsg = this.#addIdent(ident); | ||
| if (errMsg) { | ||
| errMsgs.push(errMsg); | ||
| } | ||
| } | ||
| if (errMsgs.length > 0) { | ||
| throw new Error(errMsgs.join("\n")); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * AddFunctions configures the checker with a list of function declarations. | ||
| * | ||
| * If there are overlapping declarations, the method will error. | ||
| */ | ||
| addFunctions(funcs: CelFunc[]): void { | ||
| let errMsgs: string[] = []; | ||
| for (const fn of funcs) { | ||
| errMsgs = errMsgs.concat(this.#setFunction(fn)); | ||
| } | ||
| if (errMsgs.length > 0) { | ||
| throw new Error(errMsgs.join("\n")); | ||
| } | ||
| } |
There was a problem hiding this comment.
Let's eliminate mutation if possible, and require variables and functions to be provided when an environment or scope is created.
| * LookupIdent returns an identifier in the Env. | ||
| * Returns undefined if no such identifier is found in the Env. | ||
| */ | ||
| lookupIdent(name: string): CelIdent | undefined { |
There was a problem hiding this comment.
The compound word lookup is a noun, only — this should be lookUpIdent (or lookUpVariable)
| /** | ||
| * Scopes represents nested Decl sets where the Scopes value contains a Groups containing all | ||
| * identifiers in scope and an optional parent representing outer scopes. | ||
| * Each Groups value is a mapping of names to Decls in the ident and function namespaces. | ||
| * Lookups are performed such that bindings in inner scopes shadow those in outer scopes. | ||
| */ | ||
| export class Scopes { | ||
| constructor( | ||
| public readonly scopes = new Group(), | ||
| public readonly parent?: Scopes, | ||
| ) { | ||
| this.scopes = scopes; | ||
| this.parent = parent; | ||
| } |
There was a problem hiding this comment.
Can we call this a Scope, singular, and eliminate the Group abstraction altogether? Also, can we eliminate mutation from this class?
| /** | ||
| * A CEL ident definition. | ||
| */ | ||
| export interface CelIdent { |
packages/cel/src/ident.ts
Outdated
| public readonly doc?: string, | ||
| ) {} | ||
|
|
||
| declarationIsEquivalent(other: CelIdent): boolean { |
There was a problem hiding this comment.
Eliminate the term "declaration" here — it's not clearly defined.
|
@jafaircl Thanks for all the hard work on this — I tried to give some useful feedback, but please let me know if anything isn't making sense to you. Or, if you'd like me to take a swing at any of my suggestions. |
|
@jafaircl For visibility, here's the POC TypeScript API I wrote: import { CelResult } from "./error.ts";
import { CelType, CelInput, CelScalar } from "./type.ts";
interface Variable<N extends string = string, T extends CelType = CelType> {
readonly name: N;
readonly type: T;
}
type CelVariableTuple<T extends readonly Variable[]> =
T extends readonly [
Variable<infer Name extends string, infer Type extends CelType>,
...infer Rest extends Variable[],
]
? [Variable<Name, Type>, ...CelVariableTuple<Rest>]
: Variable[] extends T ? any[] : [];
type VariableInputEntry<V extends Variable> = {
[key in V['name']]: CelInput<V['type']>
}
type CelInputMap<T extends CelVariableTuple<readonly Variable[]>> =
T extends readonly [
infer V extends Variable,
...infer Rest extends Variable[],
] ? VariableInputEntry<V> & CelInputMap<Rest> : {};
function plan<
const T extends CelVariableTuple<readonly Variable[]>,
>(variables: T): ((bindings: CelInputMap<T>) => CelResult) {
return () => variables.length; // fake program
}
const program = plan([
{ name: "foo", type: CelScalar.STRING },
{ name: "bar", type: CelScalar.INT },
]);
program({ foo: "string", bar: 1n });EDIT: @srikrsna-buf suggests changing the declarations to an object to match the bindings, which makes this a lot simpler: import { CelScalar, type CelType, mapType, type CelInput } from "./type.js";
export type BindingDeclaration = {
[key: string]: CelType;
};
export type Binding<T extends BindingDeclaration> = {
[P in keyof T]: CelInput<T[P]>;
};
function plan<const T extends BindingDeclaration>(
binding: T,
): (params: Binding<T>) => void {
return () => {};
}
const program = plan({
foo: CelScalar.STRING,
bar: CelScalar.INT,
});
program({ foo: "string", bar: 1n }); |
|
Dan Hudlow seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
Since there have been so many breaking changes, this would have to be completely rewritten. I'm not even sure if some of the changes you have made are compatible with the way all other type checking implementations work. Surely something in this PR is salvageable. It's just not immediately clear what that is. Is this something you as a team would prefer to do? I don't want to waste time if it's something you are going to work on internally or not something you are interested in implementing anymore. |
We are very much interested in adding a type checker. We are not working on it internally at this point, but will be happy to review and accept a contribution. I understand if you don't want to work on this anymore, but the recent changes were done in response to this PR to trim this down to just the type checker implementation. If you do decide to pick this up again, I think it is best if we start by agreeing on how the public API is going to end up looking and on a general approach to the implementation. Then, implementing this over several PRs with unit tests (when relevant) will make it significantly easier to review and integrate.
The intent was to make it more idiomatic for the TS/JS ecosystem. We are already quite different from other implementations where we don't always box values and function implementations are type-safe. If, however, there is a limitation in the current implementation that makes it impossible or difficult to implement a type checker, please let us know and we'd be happy to address it. Thank you for your interest and contributions so far! |
4 participants
No description provided.