-
Notifications
You must be signed in to change notification settings - Fork 253
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
feat: add std::warn::warn
#7723
base: master
Are you sure you want to change the base?
Conversation
@@ -597,6 +600,28 @@ impl Elaborator<'_> { | |||
} | |||
} | |||
|
|||
// It's an error to call to the built-in function "warn" if we are not in a comptime context | |||
fn check_call_to_warn_builtin_at_runtime( |
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.
Is this not covered by how we disallow calling comptime functions from a runtime context?
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, you are right. However, that check happens during monomorphization (not sure why!) and before that it panics when it can't find the built-in in SSA generation. That said, I guess I'll introduce this built-in in SSA that does nothing to let monomorphization produce this error.
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.
Hmm... but monomorphization happens before SSA, I don't know why it goes to SSA with errors 🤔
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.
Aah... it's because built-in functions don't go through the monomorphizer's queue.
Would it be okay to move this error from monomorphization to where I put it? (generalize it to all comptime functions, not just warn
).
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'm stopping for the day so we can discuss next week. You can workaround this with a non-builtin wrapper function which is comptime - this will ensure that the error is thrown 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.
Good idea! And that explains why we have many wrappers like that.
But... it didn't work. The reason is that the warning is then produced in the std crate, and we don't show warnings if they aren't from the crate being compiled.
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.
Tbh that means that this feature isn't going to work for Nico's usage anyway
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.
⚠️ 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: 4625137 | Previous: df20280 | Ratio |
---|---|---|---|
private-kernel-reset |
8.754 s |
6.796 s |
1.29 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
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.
⚠️ 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: 4625137 | Previous: df20280 | Ratio |
---|---|---|---|
AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob |
53 s |
44 s |
1.20 |
AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types |
64 s |
53 s |
1.21 |
noir-lang_noir_bigcurve_ |
277 s |
224 s |
1.24 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
Description
Problem
Resolves #7714
Summary
There's some special code here to produce an error if
warn
is used at runtime. Eventually we could control this with another attribute, maybe, though I don't know if we'll have many functions that can only be called at compile-time...Additional Context
Documentation
Check one:
PR Checklist
cargo fmt
on default settings.