-
Notifications
You must be signed in to change notification settings - Fork 11
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
[WIP] feat: add support for functions #344
Conversation
7c6ef62
to
e161d68
Compare
7806077
to
a2a1583
Compare
a38aeba
to
11aaad3
Compare
parser/src/transforms/inlining.rs
Outdated
self.rewrite_expr(&mut expr)?; | ||
Ok(vec![Statement::Expr(expr)]) | ||
Statement::Expr(mut expr) => match expr { | ||
Expr::Call(call) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would revert this specific change here - when we expand a statement block, we will expand call sites, and in the process recursively expand the callee. We do want to rewrite call expressions which are introduced into the tree when inlining the callee (so that arguments are rewritten to use the expressions from the caller). I think expanding the call here would cause inlining of the callee to occur without rewriting the arguments, and result in incorrect rewriting of the callee body.
I'm working through this by running the code in my mental interpreter, so to speak, so I may be forgetting something that makes this necessary - but the comment I left just above this is what reminded me that this is already supposed to be an expression introduced as the result of inlining, and we only are interested in applying rewrites.
It may also be very possible that Expr::Call
can never be observed here - a good way to check that would be to make the match arm panic and see if you can trigger it. Without working through some tests designed to see what exactly is going on when a call is seen here, it's hard for me to say for sure whether that would be unexpected with certainty, but my gut feeling is that we don't want to attempt expanding calls here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Would we not need it when the return value is a function call to another function?
fn foo(a: felt) -> felt:
return bar(a)
Maybe there's another way to handle this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expansion is performed recursively, so when we expand the call to foo
, we will also expand it's body, and thus the call to bar
as well.
As an aside, we need to make sure we check for recursive functions calls and raise a diagnostic. We can do that during inlining by tracking what calls we're currently expanding. Each time we start expanding a call, we check if we're currently in an expansion of the callee, and if so, raise a diagnostic; if not, then we insert the callee into the "expansion in progress" set and proceed with inlining. When we finish expanding a call, we remove the callee from that set.
We should probably also do that for evaluators, since the same issue can occur there, I don't think I handled that anywhere yet.
We can handle that later though, no need to do that as part of this PR.
parser/src/transforms/inlining.rs
Outdated
} | ||
} | ||
|
||
fn expand_binary_expr(&mut self, expr: &mut BinaryExpr) -> Result<Vec<Statement>, SemanticAnalysisError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't look like you are expanding binary expressions correctly here. You are expanding the statements of the two operands, but never applying the operator to the values produced by the operand statements.
For these expressions, I would recommend checking first to see if either operand is a function call. If not, you can handle it the "simple" way, i.e. by calling rewrite_expr
straight away.
In the case where one or both operands are function calls, you'll want to do the following:
- If the left-hand operand is a non-call expression, nothing needs to be done, other than calling
rewrite_scalar_expr
on it. - If the left-hand operand is a function call, expand it to a block of statements, as you are doing here. Then, using
with_let_result
, as demonstrated inexpand_let
, replace the last expression of the block with a newlet
expression, in essence binding the "result" of the originallet
to a new generated variable so we can reference it when we emit code for the binary operator. While we're still in the call towith_let_result
, we must expand the right-hand operand as well, as it will form the body of thelet
we're generating in this step. - If the right-hand operand is a non-call expression, call
rewrite_scalar_expr
on it, and then emit the code for the binary operator. Assuming the left-hand operand was a call, and we're still in the call towith_let_result
from step 2, we now have all of the pieces to emit thelet
we started building in that step. - If the right-hand operand is a call expression, we need to perform the same process as in step 2, just on the right-hand operand this time. Assuming the left-hand operand was also a call, this will mean that we're going to have a nested call to
with_let_result
. The nested call is where we will emit the binary operator, applied to the operands we bound in the generatedlet
s. When unwinding these calls, the effect will be that the binary operator is placed at the bottom of the let tree, preceded by the statements expanded from the right-hand operand, preceded by the statements expanded from the left-hand operand.
Any code following the original binary expression, when one of the operands was a call, will necessarily need to be placed at the bottom of the resulting let-tree, but that is handled by expand_let
, we only need handle emitting that tree here.
parser/src/transforms/inlining.rs
Outdated
ScalarExpr::Call(call) => { | ||
let mut expanded = self.expand_call(call.clone())?; | ||
with_let_result(self, &mut expanded, move |_, value| { | ||
*expr = ScalarExpr::try_from(value.clone())?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work. You've got the gist of the idea, but there's an issue here: You are replacing the original scalar expression (the call), with whatever expression (after expansion) is at the end of the callee, effectively discarding the entire expansion except for that last expression - not what you want.
The purpose of this function is to take a scalar expression, and emit a block of statements corresponding to the expansion of that expression. The scalar expression is mutable so that you can "steal" values from it without cloning them, because the original scalar expression will be discarded after this function returns.
So what you want to do here is expand the call, as you are, and simply return the expansion, that's it. We don't need to do anything further with it here. It is up to callers of expand_scalar_expr
to handle the expansion appropriately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thanks. I think I got it.
parser/src/transforms/inlining.rs
Outdated
let mut stmts = Vec::new(); | ||
|
||
// Recursively expand the lhs and rhs. | ||
stmts.extend(self.expand_scalar_expr(lhs)?); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an example of a place where we need to be careful about how we handle the results of expand_scalar_expr
. In particular, we need to handle this similarly to what I described in expand_binary_expr
. Expand both operands, and then do the following:
Check the expansion of the left-hand operand to see if it terminates with a let
. If it does, we're going to use with_let_result
on it to perform a transformation like we did in expand_binary_expr
. In the call to with_let_result
, we will check if the right-hand expansion also terminates with a let, and if so, we're going to immediately call with_let_result
on it, and emit the binary operator in the body of a new let
. The, now rewritten, expansion of the right-hand operand will be used as the body of a new let
in the expansion of the left-hand statement block.
At the end, we simply return the expanded left-hand statement block (as we've moved the right-hand statement block into it).
NOTE: You won't need to call rewrite_scalar_expr
, as by expanding both operands, that function gets called already.
NOTE: If either operand expands to a block that doesn't terminate in a let
, which should only happen if there is just a single Statement::Expr
in the block, you can elide the with_let_result
for that operand, and just use that expression as the operand for the binary operator directly. Obviously, in the case where both operands are simple expressions, you can omit generating a let
entirely, just as in expand_binary_expr
parser/src/transforms/inlining.rs
Outdated
self.rewrite_scalar_expr(lhs.as_mut())?; | ||
self.rewrite_scalar_expr(rhs.as_mut())?; | ||
Ok(vec![Statement::Enforce(ScalarExpr::Binary(BinaryExpr { | ||
let lhs_statements = self.expand_scalar_expr(lhs.as_mut())?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same deal as in expand_scalar_expr
here
parser/src/transforms/inlining.rs
Outdated
// which can produce aggregates. However, when those are added, we may want to add support | ||
// for that here. This branch is set up to raise an appropriate panic if we forget to do so. | ||
Expr::Call(_) => unimplemented!("calls to functions as iterables"), | ||
// Expr::Call(call) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably should perform a transformation earlier in the compiler pipeline that rewrites comprehension expressions with calls as iterable values, into a let
tree, where the result of each call is bound to a new variable via let
, and at the bottom of the let tree we place the comprehension expression, with the calls rewritten to their corresponding let
-bound variable.
Alternatively, we could disallow calls in iterable position at the syntax level, but since we don't have any specific reason why we can't support them, that might not be desirable, aside from a little less complexity.
In either case, we would then handle Expr::Call
here just like Expr::Binary
and Expr::ListComprehension
below.
2ea222d
to
5464fc2
Compare
683d4c1
to
14ee3ba
Compare
14ee3ba
to
5945d65
Compare
5945d65
to
ad440a3
Compare
Superceded by #351 |
This PR adds support for pure functions to AirScript.
TODOs:
func(a + b)
)func([1, 2, 3, 4])
)func_a(func_b(a))
)