-
Notifications
You must be signed in to change notification settings - Fork 370
fix: check lambda runtime context also in comptime #11280
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
base: master
Are you sure you want to change the base?
Changes from all commits
7443722
45aeed8
0bc3378
0b1254a
cdbf64c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -707,7 +707,15 @@ impl<'context> Elaborator<'context> { | |
| /// True if we're currently within a constrained function or lambda. | ||
| /// Defaults to `true` if the current function is unknown. | ||
| 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); | ||
| if in_unconstrained_lambda { | ||
| return false; | ||
| } | ||
|
|
||
| // 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() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes it could be because of this. |
||
| return false; | ||
| } | ||
|
|
||
|
|
@@ -719,9 +727,7 @@ impl<'context> Elaborator<'context> { | |
| } | ||
| }); | ||
|
|
||
| let in_unconstrained_lambda = self.lambda_stack.last().is_some_and(|ctx| ctx.unconstrained); | ||
|
|
||
| !in_unconstrained_function && !in_unconstrained_lambda | ||
| !in_unconstrained_function | ||
| } | ||
|
|
||
| /// Register a use of the given unstable feature. Errors if the feature has not | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -460,14 +460,29 @@ fn user_defined_verify_proof_with_type_is_allowed_in_brillig() { | |
| assert_no_errors(src); | ||
| } | ||
|
|
||
| /// Globals are evaluated in a `comptime` context so they can call unconstrained functions without `unsafe` blocks | ||
| #[test] | ||
| fn call_unconstrained_function_in_lambda_in_global() { | ||
| fn cannot_call_unconstrained_function_in_lambda_in_global() { | ||
| let src = r#" | ||
| pub global foo: fn() = || bar(); | ||
guipublic marked this conversation as resolved.
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 @asterite I see you came to a similar conclusion as well #10385 (comment). |
||
| ^^^^^ Call to unconstrained function from constrained function is unsafe and must be in an unconstrained function or unsafe block | ||
|
|
||
| unconstrained fn bar() {} | ||
|
|
||
| fn main() {} | ||
| "#; | ||
| check_errors(src); | ||
| } | ||
|
|
||
| #[test] | ||
| fn call_unconstrained_function_in_lambda_in_global() { | ||
| let src = r#" | ||
| pub global foo: fn() = || { | ||
| // Safety: showing an unconstrained function call for testing | ||
| unsafe { bar() } | ||
| }; | ||
| unconstrained fn bar() {} | ||
|
|
||
| fn main() {} | ||
| "#; | ||
| assert_no_errors(src); | ||
| } | ||
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 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.