Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate to a native AST and Expr representation #789

Closed
TristonianJones opened this issue Aug 6, 2023 · 5 comments
Closed

Migrate to a native AST and Expr representation #789

TristonianJones opened this issue Aug 6, 2023 · 5 comments
Assignees

Comments

@TristonianJones
Copy link
Collaborator

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
Copy link
Collaborator Author

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.

@charithe
Copy link

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
Copy link

charithe commented Nov 1, 2023

@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()
+} 

@TristonianJones
Copy link
Collaborator Author

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

charithe added a commit to charithe/cerbos that referenced this issue Nov 13, 2023
CEL 0.18.2 [added a method](google/cel-go#789 (comment)) to get the native AST object from the AST wrapper.

Signed-off-by: Charith Ellawala <charith@cerbos.dev>
charithe added a commit to charithe/cerbos that referenced this issue Nov 16, 2023
CEL 0.18.2 [added a method](google/cel-go#789 (comment)) to get the native AST object from the AST wrapper.

Signed-off-by: Charith Ellawala <charith@cerbos.dev>
charithe added a commit to cerbos/cerbos that referenced this issue Nov 16, 2023
CEL 0.18.2 [added a
method](google/cel-go#789 (comment))
to get the native AST object from the AST wrapper.

Signed-off-by: Charith Ellawala <charith@cerbos.dev>

Signed-off-by: Charith Ellawala <charith@cerbos.dev>
@tonyhb
Copy link

tonyhb commented Dec 2, 2023

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants