cel-go icon indicating copy to clipboard operation
cel-go copied to clipboard

Migrate to a native AST and Expr representation

Open TristonianJones opened this issue 2 years ago • 5 comments

Migrate to a native AST and Expr representation.

The original cel-go implementation was heavily based on protobuf definitions for types, declarations, and expressions. While this was expedient, it has also been cumbersome. With PR #568, the types and declarations were replaced by Go-native structs. This issue tracks the migration of the Expr, SourceInfo, and AST objects.

The motives for making this change are many:

  • Performance
  • Remove (most) hard dependencies on protobuf
  • Allow for better interop between serialization formats and type systems

TristonianJones avatar Aug 06 '23 18:08 TristonianJones

As part of the optimizer work, it's clear that the split between cel.Ast and ast.AST is confusing. The two structures exist separately to ease migration, but should be merged before this issue can be considered closed.

TristonianJones avatar Aug 29 '23 06:08 TristonianJones

Apologies if this is not the right place to ask this. Is the plan to unify the two AST data types into one in a future release?

I am a bit confused by how we are supposed to use the new native types right now. Most of the methods to work with parsed expressions take arguments of type ast.Expr. However, the return type of Compile and Check methods of cel.Env is cel.Ast which doesn't have a method to obtain the underlying ast.Expr object. I am currently forced to do something along the lines of the following:

celAST, _ := env.Compile(exprString)
checkedExpr, _ := cel.AstToCheckedExpr(celAST)
astAST, _ := ast.ToAST(checkedExpr)
expr := astAST.Expr() // finally have an `ast.Expr` that I can use with `ast.PreOrderVisit` etc.

Is there a better way to do this with the current version of CEL (0.18.1) or should I wait for the AST types to be unified?

charithe avatar Sep 27 '23 10:09 charithe

@TristonianJones apologies for pinging you directly. I am stuck on the above issue of not being able to access the underlying ast.Expr of the cel.Ast without jumping through a lot of hoops. Would you be open to a PR that introduces two functions to obtain the Expr and SourceInfo of an Ast object like the following or adding them as methods to cel.Ast?

diff --git a/cel/io.go b/cel/io.go
index 3133fb9..50bb878 100644
--- a/cel/io.go
+++ b/cel/io.go
@@ -101,22 +101,36 @@ func AstToString(a *Ast) (string, error) {
 	return parser.Unparse(a.impl.Expr(), a.impl.SourceInfo())
 }
 
+// AstToExpr returns the underlying expr of this Ast
+func AstToExpr(a *Ast) ast.Expr {
+	return a.impl.Expr()
+}
+
+// AstToSourceInfo returns the underlying source information of this Ast
+func AstToSourceInfo(a *Ast) *ast.SourceInfo {
+	return a.impl.SourceInfo()
+} 

charithe avatar Nov 01 '23 13:11 charithe

@charithe I added a NativeRep() call on the cel.Ast to convert to an ast.AST as part of #853 ; hopefully, this helps.

TristonianJones avatar Nov 10 '23 00:11 TristonianJones

@charithe I added a NativeRep() call on the cel.Ast to convert to an ast.AST as part of #853 ; hopefully, this helps.

@TristonianJones Thank you so much! A little more performant for straight iterating than creating a new optimizer. Appreciate you and all of the work on cel-go!

tonyhb avatar Dec 02 '23 20:12 tonyhb