Simplify functions and distinguish methods by hudlow · Pull Request #244 · bufbuild/cel-es

hudlow · 2025-12-19T19:42:25Z

NOTE THIS IS A BREAKING CHANGE AND NEEDS TO BE CONSIDERED CAREFULLY

As I discussed with @timostamm and @srikrsna-buf, this PR eliminates the "overload" abstraction from the cel-es codebase. Instead, multiple functions (and methods) can have the same name, and each function is responsible for whether it matches a name and/or a set of arguments.

This way, the _&&_ and _||_ functions can still match on any set of arguments and handle short-circuiting in a conformant way — but other functions rely on standard argument-matching logic.

This also lays the groundwork for matching functions via overload_id (which will be necessary if evaluating a checked expression) and matching functions via parameter types instead of argument values.

This PR does introduce unique, generated overload IDs in a different way than #243, but it doesn't attempt to match the IDs to the cel-go implementation.

Unfortunately, the scope grew larger than I intended. If it's necessary, I can try to decompose the work further.

srikrsna-buf

Overall I am quite happy with the simplification of removing overloads and fixing the method vs function problem. Left some comments, still reviewing the rest.

srikrsna-buf · 2025-12-23T12:03:27Z

+  narrowedByName(name: string): FuncRegistry;
+  narrowedByArgs(...args: [CelResult[]] | [CelResult, CelResult[]]): FuncRegistry;
+  // TODO: narrowedByParams(...args: [CelType[]] | [CelType, CelType[]]): FuncRegistry;


This means we will create a new FuncRegistry for each lookup. Maybe we can avoid the allocation if we club the args and name and just return a Callable | undefined?

I simplified function registry, renamed it back to a dispatcher (to avoid the proto registry confusion), and added a lookup cache.

srikrsna-buf · 2025-12-23T12:05:10Z

-  find(name: string) {
-    return this.functions.get(name);
-  }
+  withFallback(registry: FuncRegistry): FuncRegistry;


User's will also see this I think we can expose a simpler interface.

I adjusted this to withFallbacks(Callable[]) — let me know what you think. The idea is to make the ordering explicit.

hudlow · 2025-12-23T22:07:57Z

+  celFunc(olc.SIZE,           [MAP],                  INT,  (x)     =>  BigInt(x.size)),
+
+  celFunc(opc.IN,             [DYN, LIST],            BOOL,             inList),
+  celFunc(opc.IN,             [DYN, MAP],             BOOL, (n, h)  =>  h.has(n as CelMapIndex)),


@srikrsna-buf CelMapIndex is used here.

srikrsna-buf · 2026-01-02T17:47:36Z

+
+  causes(value: unknown, exprId?: bigint | number): CelError;


I think we can use a function instead for this that is internal like the deleted celErrorMerge which is not exported from the package.

srikrsna-buf · 2026-01-02T17:51:28Z

  }
 }
+
+function isOfType<T extends CelType>(


Maybe we can move this to type.ts?

srikrsna-buf · 2026-01-02T18:01:33Z

-export interface Dispatcher {
-  find(name: string): CallDispatch | undefined;
-}
+export interface Callable<R extends CelType = CelType> {


Because the interface is public via celFunc and celMethod I think it will be useful to add the doc comments back

srikrsna-buf · 2026-01-02T18:03:24Z

-export interface Dispatcher {
-  find(name: string): CallDispatch | undefined;
-}
+export interface Callable<R extends CelType = CelType> {


I am not sure making the result generic is useful. We can simplify if we use CelType directly.

srikrsna-buf · 2026-01-02T18:05:36Z


-  get parameters() {
-    return this._parameters;
+export function celFunc<const P extends TypeTuple, const R extends CelType>(


Doc comments for celFunc

srikrsna-buf · 2026-01-02T18:28:45Z

+
+  if (errors.length) return errors[0].causes(errors.slice(1));
+
+  return results as CelValue[];


I think we should return unwrapped here

srikrsna-buf · 2026-01-02T18:29:11Z

+  const unwrapped = results.reduce((u, r, i) => {
+    return u.concat([
+      types ? unwrapResult(r, types[i], i + 1) : unwrapResult(r),
+    ]);
+  }, [] as CelResult[]);
+
+  const errors = unwrapped.filter((r) => isCelError(r));


Nit: Using a regular loop we can do it in one go

srikrsna-buf · 2026-01-02T18:34:39Z

+  }
+
+  call(id: number, args: CelResult[]): CelResultFromType<R> {
+    const target = unwrapResult(args[0], this._target);


There could be a subtle bug here unwrapResult expects a CelResult which can never be undefined but here the args can be an empty list and args[0] will return undefined. I think we should replicate matchArgs to be safe

Great catch!

srikrsna-buf · 2026-01-02T18:44:05Z

+  readonly #callables: Callable[];
+  readonly #nameCache: Map<string, Dispatcher | undefined> = new Map();
+  readonly #overloadIdCache: Map<string, Callable | undefined> = new Map();


We deliberately don't use JS private fields to support older runtimes. This is anyway an internal class we can just use TS private fields

srikrsna-buf · 2026-01-02T18:46:32Z

  }
  eval(ctx: Activation): CelResult {
-    const vals = coerceToValues(this.values.map((x) => x.eval(ctx)));
+    const vals = unwrapResultTuple(this.values.map((x) => x.eval(ctx)));


We don't want to unwrap here because we want the Any unwrap to be lazy

CLAassistant · 2026-01-07T17:57:40Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

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.}

Dan Hudlow added 2 commits December 19, 2025 13:31

Simplify functions and distinguish methods

66f13bb

Revert time function changes that failed in Node 20.x

cb0c89b

srikrsna-buf reviewed Dec 23, 2025

View reviewed changes

Dan Hudlow added 5 commits December 23, 2025 10:33

Merge remote-tracking branch 'origin/main' into hudlow/noverloads

80c1f50

Revert rename of registry to protoRegistry

922890e

Simplify types per review feedback

447a5d8

Improve formatting of function tables

2211622

revert unnecessary rename

284559f

hudlow commented Dec 23, 2025

View reviewed changes

Dan Hudlow added 2 commits December 23, 2025 17:26

Revert unneeded changes for value conversion

dc1a8fa

Make interface members readonly per review comment

622bba6

srikrsna-buf reviewed Jan 2, 2026

View reviewed changes

Implement feedback for #244 (function simplification) (#252)

ab0c2e5

srikrsna-buf closed this Jan 16, 2026

srikrsna-buf deleted the hudlow/noverloads branch January 19, 2026 15:58


		if (errors.length) return errors[0].causes(errors.slice(1));

		return results as CelValue[];

Uh oh!

Conversation

hudlow commented Dec 19, 2025

Uh oh!

srikrsna-buf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CLAassistant commented Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

CLAassistant commented Jan 7, 2026 •

edited

Loading