fix: check lambda runtime context also in comptime #11280
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: cdbf64c | Previous: b8d4eea | Ratio |
|---|---|---|---|
test_report_zkpassport_noir-ecdsa_ |
2 s |
1 s |
2 |
test_report_zkpassport_noir_rsa_ |
2 s |
1 s |
2 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: 7443722 | Previous: e48a0d8 | Ratio |
|---|---|---|---|
rollup-block-root-single-tx |
0.003 s |
0.002 s |
1.50 |
rollup-root |
0.005 s |
0.004 s |
1.25 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
|
|
||
| // Check if we're in a comptime context (and not in a lambda that will run at runtime). | ||
| // Lambdas in global initializers are elaborated in comptime context but execute at runtime. | ||
| if self.in_comptime_context() && self.lambda_stack.is_empty() { |
There was a problem hiding this comment.
How about when we have a lambda stack is not empty? It looks like this valid code will error now:
unconstrained fn bar() {}
fn main() {
comptime {
let _ = || bar();
}
}Could we add a test case for this under the runtime unit tests as well.
It looks like we will need to track whether we are resolving a global more explicitly.
There was a problem hiding this comment.
I have not investigated whether this is the exact cause but it looks like we are getting a lot more constrained/unconstrained boundary errors https://github.com/noir-lang/noir/actions/runs/21208049227/job/61009656909?pr=11280. The crux of the problem just looks to be that there are comptime contexts aside globals.
There was a problem hiding this comment.
Yes it could be because of this.
Your example fails because it ask for unsafe { bar()}, but then it works with it.
I think it is fine to require it, although in this specific case it is not useful. The compiler is currently not smart enough to see when unsafe can be avoided.
However, it seems that a lot Aztec code will need to be updated...
| fn in_constrained_function(&self) -> bool { | ||
| if self.in_comptime_context() { | ||
| // Check if any lambda in the nesting chain is unconstrained. | ||
| let in_unconstrained_lambda = self.lambda_stack.iter().any(|ctx| ctx.unconstrained); |
There was a problem hiding this comment.
It was not enough to just look at the last member of the lambda stack? Would be nice to have a unit test that displays a failure when that check is not enough.
| #[test] | ||
| fn cannot_call_unconstrained_function_in_lambda_in_global() { | ||
| let src = r#" | ||
| pub global foo: fn() = || bar(); |
There was a problem hiding this comment.
I am now unsure whether we need to prevent this type of code in the first place.
This code is equivalent to the following (which passes w/o errors):
unconstrained fn bar() {}
comptime fn baz() {
bar()
}
pub global foo_global: fn() = || baz();
fn main() {
comptime {
let _ = baz();
}
}Yet in the snippet above we do not need any unsafe blocks as baz acts as a comptime fn wrapper around bar. It makes sense that the call to bar would not need an unsafe block as we only check from constrained -> unconstrained calls in our type checking (in fact the error message looks like it could be updated). comptime should not need unsafe blocks to called unconstrained functions and as globals are evaluated in comptime that should apply to them as well.
@asterite I see you came to a similar conclusion as well #10385 (comment).
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Brillig Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: cdbf64c | Previous: b8d4eea | Ratio |
|---|---|---|---|
rollup-tx-merge |
0.002 s |
0.001 s |
2 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Compilation Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: cdbf64c | Previous: b8d4eea | Ratio |
|---|---|---|---|
rollup-tx-base-private |
25.86 s |
19.2 s |
1.35 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
Description
Problem
Resolves #10385
Summary
Use the lambda runtime context for the 'constrain-ness'.
Also fix in case of nested lambdas by checking for any unconstrained lambda.
Additional Context
User Documentation
Check one:
PR Checklist
cargo fmton default settings.