Skip to content

Commit

Permalink
chore: remove malformed functions from brillig reports (noir-lang/noi…
Browse files Browse the repository at this point in the history
…r#6898)

chore: clean up gates reports script (noir-lang/noir#6896)
chore: move empty programs to `compile_success_empty` (noir-lang/noir#6891)
feat: add a warning when using unsafe blocks without safety comments (noir-lang/noir#6860)
chore: quick docs fix for #6839 (noir-lang/noir#6840)
chore: Avoid duplicate Not instructions during flattening (noir-lang/noir#6886)
chore: Use smallvec for instruction results (noir-lang/noir#6877)
chore(ci): Display times in compilation and execution reports only with seconds (noir-lang/noir#6880)
feat: flatten nested if-else statements with equivalent conditions (noir-lang/noir#6875)
chore(ci): Take averages for compilation and execution report of small programs (noir-lang/noir#6874)
fix: don't deduplicate binary math of unsigned types (noir-lang/noir#6848)
feat: warn on unnecessary unsafe blocks (noir-lang/noir#6867)
chore: remove the `as_field` and `from_field` built-ins (noir-lang/noir#6845)
chore: fix warnings (noir-lang/noir#6863)
fix: detect cycles in globals (noir-lang/noir#6859)
chore(ci): Execution time report (noir-lang/noir#6827)
chore(ci): Add non determinism check and fixes (noir-lang/noir#6847)
chore(docs): updating the solidity contract how-to guide (noir-lang/noir#6804)
fix: double alias in path (noir-lang/noir#6855)
feat: configurable external check failures (noir-lang/noir#6810)
chore: move constant creation out of loop (noir-lang/noir#6836)
fix: implement `as_field` and `from_field` in the interpreter (noir-lang/noir#6829)
chore: Use Vec for callstacks (noir-lang/noir#6821)
feat: replace `eval_global_as_array_length` with type/interpreter evaluation (noir-lang/noir#6469)
chore: refactor `DataFlowGraph.insert_instruction_and_results` (noir-lang/noir#6823)
chore(docs): updating noirjs tutorial for 1.0.0 (noir-lang/noir#6792)
feat: Sync from aztec-packages (noir-lang/noir#6824)
chore: Have rust-analyzer use the stable toolchain (noir-lang/noir#6825)
  • Loading branch information
AztecBot committed Dec 20, 2024
2 parents 04e3ca2 + c803557 commit c1309e6
Show file tree
Hide file tree
Showing 96 changed files with 327 additions and 76 deletions.
2 changes: 1 addition & 1 deletion .noir-sync-commit
Original file line number Diff line number Diff line change
@@ -1 +1 @@
fd816cf1e747ef036be84771a6f3fe0a651d80a6
913be5b013323449963d31ab00f521f572cfcd67
28 changes: 25 additions & 3 deletions noir/noir-repo/.github/workflows/reports.yml
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ jobs:
- name: Compare gates reports
id: gates_diff
uses: noir-lang/noir-gates-diff@1931aaaa848a1a009363d6115293f7b7fc72bb87
uses: noir-lang/noir-gates-diff@84ada11295b9a1e1da7325af4e45e2db9f775175
with:
report: gates_report.json
summaryQuantile: 0.9 # only display the 10% most significant circuit size diffs in the summary (defaults to 20%)
Expand Down Expand Up @@ -121,7 +121,7 @@ jobs:
- name: Compare Brillig bytecode size reports
id: brillig_bytecode_diff
uses: noir-lang/noir-gates-diff@d88f7523b013b9edd3f31c5cfddaef87a3fe1b48
uses: noir-lang/noir-gates-diff@84ada11295b9a1e1da7325af4e45e2db9f775175
with:
report: gates_report_brillig.json
header: |
Expand Down Expand Up @@ -170,7 +170,7 @@ jobs:
- name: Compare Brillig execution reports
id: brillig_execution_diff
uses: noir-lang/noir-gates-diff@d88f7523b013b9edd3f31c5cfddaef87a3fe1b48
uses: noir-lang/noir-gates-diff@84ada11295b9a1e1da7325af4e45e2db9f775175
with:
report: gates_report_brillig_execution.json
header: |
Expand Down Expand Up @@ -367,6 +367,28 @@ jobs:
mv /home/runner/work/noir/noir/scripts/test_programs/execution_report.sh ./execution_report.sh
./execution_report.sh 1 1
- name: Generate compilation report with averages
working-directory: ./test-repo/${{ matrix.project.path }}
if: ${{ matrix.project.take_average }}
run: |
mv /home/runner/work/noir/noir/scripts/test_programs/compilation_report.sh ./compilation_report.sh
chmod +x ./compilation_report.sh
./compilation_report.sh 1 1
- name: Generate execution report without averages
working-directory: ./test-repo/${{ matrix.project.path }}
if: ${{ !matrix.project.is_library && !matrix.project.take_average }}
run: |
mv /home/runner/work/noir/noir/scripts/test_programs/execution_report.sh ./execution_report.sh
./execution_report.sh 1
- name: Generate execution report with averages
working-directory: ./test-repo/${{ matrix.project.path }}
if: ${{ !matrix.project.is_library && matrix.project.take_average }}
run: |
mv /home/runner/work/noir/noir/scripts/test_programs/execution_report.sh ./execution_report.sh
./execution_report.sh 1 1
- name: Move compilation report
id: compilation_report
shell: bash
Expand Down
21 changes: 16 additions & 5 deletions noir/noir-repo/compiler/noirc_frontend/src/debug/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -523,7 +523,9 @@ pub fn build_debug_crate_file() -> String {
__debug_var_assign_oracle(var_id, value);
}
pub fn __debug_var_assign<T>(var_id: u32, value: T) {
unsafe {{
unsafe {
//@safety: debug context
{
__debug_var_assign_inner(var_id, value);
}}
}
Expand All @@ -534,7 +536,9 @@ pub fn build_debug_crate_file() -> String {
__debug_var_drop_oracle(var_id);
}
pub fn __debug_var_drop(var_id: u32) {
unsafe {{
unsafe {
//@safety: debug context
{
__debug_var_drop_inner(var_id);
}}
}
Expand All @@ -545,7 +549,9 @@ pub fn build_debug_crate_file() -> String {
__debug_fn_enter_oracle(fn_id);
}
pub fn __debug_fn_enter(fn_id: u32) {
unsafe {{
unsafe {
//@safety: debug context
{
__debug_fn_enter_inner(fn_id);
}}
}
Expand All @@ -556,7 +562,9 @@ pub fn build_debug_crate_file() -> String {
__debug_fn_exit_oracle(fn_id);
}
pub fn __debug_fn_exit(fn_id: u32) {
unsafe {{
unsafe {
//@safety: debug context
{
__debug_fn_exit_inner(fn_id);
}}
}
Expand All @@ -567,7 +575,9 @@ pub fn build_debug_crate_file() -> String {
__debug_dereference_assign_oracle(var_id, value);
}
pub fn __debug_dereference_assign<T>(var_id: u32, value: T) {
unsafe {{
unsafe {
//@safety: debug context
{
__debug_dereference_assign_inner(var_id, value);
}}
}
Expand All @@ -594,6 +604,7 @@ pub fn build_debug_crate_file() -> String {
}}
pub fn __debug_member_assign_{n}<T, Index>(var_id: u32, value: T, {var_sig}) {{
unsafe {{
//@safety: debug context
__debug_inner_member_assign_{n}(var_id, value, {vars});
}}
}}
Expand Down
23 changes: 19 additions & 4 deletions noir/noir-repo/compiler/noirc_frontend/src/lexer/lexer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -726,7 +726,7 @@ impl<'a> Lexer<'a> {
}

fn parse_comment(&mut self, start: u32) -> SpannedTokenResult {
let doc_style = match self.peek_char() {
let mut doc_style = match self.peek_char() {
Some('!') => {
self.next_char();
Some(DocStyle::Inner)
Expand All @@ -735,9 +735,17 @@ impl<'a> Lexer<'a> {
self.next_char();
Some(DocStyle::Outer)
}
Some('@') => Some(DocStyle::Safety),
_ => None,
};
let comment = self.eat_while(None, |ch| ch != '\n');
let mut comment = self.eat_while(None, |ch| ch != '\n');
if doc_style == Some(DocStyle::Safety) {
if comment.starts_with("@safety") {
comment = comment["@safety".len()..].to_string();
} else {
doc_style = None;
}
}

if !comment.is_ascii() {
let span = Span::from(start..self.position);
Expand All @@ -752,7 +760,7 @@ impl<'a> Lexer<'a> {
}

fn parse_block_comment(&mut self, start: u32) -> SpannedTokenResult {
let doc_style = match self.peek_char() {
let mut doc_style = match self.peek_char() {
Some('!') => {
self.next_char();
Some(DocStyle::Inner)
Expand All @@ -761,6 +769,7 @@ impl<'a> Lexer<'a> {
self.next_char();
Some(DocStyle::Outer)
}
Some('@') => Some(DocStyle::Safety),
_ => None,
};

Expand All @@ -787,7 +796,13 @@ impl<'a> Lexer<'a> {
ch => content.push(ch),
}
}

if doc_style == Some(DocStyle::Safety) {
if content.starts_with("@safety") {
content = content["@safety".len()..].to_string();
} else {
doc_style = None;
}
}
if depth == 0 {
if !content.is_ascii() {
let span = Span::from(start..self.position);
Expand Down
3 changes: 3 additions & 0 deletions noir/noir-repo/compiler/noirc_frontend/src/lexer/token.rs
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,7 @@ impl Display for FmtStrFragment {
pub enum DocStyle {
Outer,
Inner,
Safety,
}

#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
Expand Down Expand Up @@ -424,11 +425,13 @@ impl fmt::Display for Token {
Token::LineComment(ref s, style) => match style {
Some(DocStyle::Inner) => write!(f, "//!{s}"),
Some(DocStyle::Outer) => write!(f, "///{s}"),
Some(DocStyle::Safety) => write!(f, "//@safety{s}"),
None => write!(f, "//{s}"),
},
Token::BlockComment(ref s, style) => match style {
Some(DocStyle::Inner) => write!(f, "/*!{s}*/"),
Some(DocStyle::Outer) => write!(f, "/**{s}*/"),
Some(DocStyle::Safety) => write!(f, "/*@safety{s}*/"),
None => write!(f, "/*{s}*/"),
},
Token::Quote(ref stream) => {
Expand Down
13 changes: 12 additions & 1 deletion noir/noir-repo/compiler/noirc_frontend/src/parser/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,8 @@ pub enum ParserErrorReason {
WrongNumberOfAttributeArguments { name: String, min: usize, max: usize, found: usize },
#[error("The `deprecated` attribute expects a string argument")]
DeprecatedAttributeExpectsAStringArgument,
#[error("Unsafe block must start with a safety comment")]
MissingSafetyComment,
}

/// Represents a parsing error, or a parsing error in the making.
Expand Down Expand Up @@ -181,7 +183,11 @@ impl ParserError {
}

pub fn is_warning(&self) -> bool {
matches!(self.reason(), Some(ParserErrorReason::ExperimentalFeature(_)))
matches!(
self.reason(),
Some(ParserErrorReason::ExperimentalFeature(_))
| Some(ParserErrorReason::MissingSafetyComment)
)
}
}

Expand Down Expand Up @@ -272,6 +278,11 @@ impl<'a> From<&'a ParserError> for Diagnostic {
"Noir doesn't have immutable references, only mutable references".to_string(),
error.span,
),
ParserErrorReason::MissingSafetyComment => Diagnostic::simple_warning(
"Missing Safety Comment".into(),
"Unsafe block must start with a safety comment: //@safety".into(),
error.span,
),
other => Diagnostic::simple_error(format!("{other}"), String::new(), error.span),
},
None => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use crate::{
MemberAccessExpression, MethodCallExpression, Statement, TypePath, UnaryOp, UnresolvedType,
},
parser::{labels::ParsingRuleLabel, parser::parse_many::separated_by_comma, ParserErrorReason},
token::{Keyword, Token, TokenKind},
token::{DocStyle, Keyword, SpannedToken, Token, TokenKind},
};

use super::{
Expand Down Expand Up @@ -375,6 +375,20 @@ impl<'a> Parser<'a> {
return None;
}

let next_token = self.next_token.token();
if matches!(
next_token,
Token::LineComment(_, Some(DocStyle::Safety))
| Token::BlockComment(_, Some(DocStyle::Safety))
) {
//Checks the safety comment is there, and skip it
let span = self.current_token_span;
self.eat_left_brace();
self.token = SpannedToken::new(Token::LeftBrace, span);
} else {
self.push_error(ParserErrorReason::MissingSafetyComment, self.current_token_span);
}

let start_span = self.current_token_span;
if let Some(block) = self.parse_block() {
Some(ExpressionKind::Unsafe(block, self.span_since(start_span)))
Expand Down Expand Up @@ -957,7 +971,8 @@ mod tests {

#[test]
fn parses_unsafe_expression() {
let src = "unsafe { 1 }";
let src = "unsafe { //@safety: test
1 }";
let expr = parse_expression_no_errors(src);
let ExpressionKind::Unsafe(block, _) = expr.kind else {
panic!("Expected unsafe expression");
Expand Down
5 changes: 4 additions & 1 deletion noir/noir-repo/compiler/noirc_frontend/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3880,7 +3880,8 @@ fn errors_on_cyclic_globals() {
fn warns_on_unneeded_unsafe() {
let src = r#"
fn main() {
unsafe {
unsafe {
//@safety: test
foo()
}
}
Expand All @@ -3900,7 +3901,9 @@ fn warns_on_nested_unsafe() {
let src = r#"
fn main() {
unsafe {
//@safety: test
unsafe {
//@safety: test
foo()
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ fn constrained_reference_to_unconstrained() {
let x_ref = &mut x;
if x == 5 {
unsafe {
//@safety: test context
mut_ref_input(x_ref, y);
}
}
Expand Down
2 changes: 2 additions & 0 deletions noir/noir-repo/docs/docs/noir/concepts/unconstrained.md
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ We can then run `u72_to_u8` as unconstrained brillig code in order to calculate
```rust
fn main(num: u72) -> pub [u8; 8] {
let out = unsafe {
//@safety: 'out' is properly constrained below in 'assert(num == reconstructed_num);'
u72_to_u8(num)
};

Expand Down Expand Up @@ -96,6 +97,7 @@ This ends up taking off another ~250 gates from our circuit! We've ended up with

Note that in order to invoke unconstrained functions we need to wrap them in an `unsafe` block,
to make it clear that the call is unconstrained.
Furthermore, a warning is emitted unless the `unsafe` block starts with a `//@safety: ...` comment explaining why it is fine to call the unconstrained function.

Generally we want to use brillig whenever there's something that's easy to verify but hard to compute within the circuit. For example, if you wanted to calculate a square root of a number it'll be a much better idea to calculate this in brillig and then assert that if you square the result you get back your number.

Expand Down
3 changes: 3 additions & 0 deletions noir/noir-repo/noir_stdlib/src/array/check_shuffle.nr
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ where
T: Eq,
{
unsafe {
/*@safety : shuffle_indices is ensured to be a permutation of 0..N, and then
shuffle_indices is ensured to map lhs to rhs: assert(lhs[i] == rhs[shuffle_indices[i]]), for all i in 0..N
*/
let shuffle_indices = __get_shuffle_indices(lhs, rhs);

for i in 0..N {
Expand Down
7 changes: 4 additions & 3 deletions noir/noir-repo/noir_stdlib/src/array/mod.nr
Original file line number Diff line number Diff line change
Expand Up @@ -185,9 +185,10 @@ where
/// ```
pub fn sort_via<Env>(self, ordering: fn[Env](T, T) -> bool) -> Self {
unsafe {
// Safety: `sorted` array is checked to be:
// a. a permutation of `input`'s elements
// b. satisfying the predicate `ordering`
/*@safety: `sorted` array is checked to be:
a. a permutation of `input`'s elements
b. satisfying the predicate `ordering`
*/
let sorted = quicksort::quicksort(self, ordering);

if !is_unconstrained() {
Expand Down
11 changes: 9 additions & 2 deletions noir/noir-repo/noir_stdlib/src/collections/umap.nr
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,11 @@ impl<K, V, B> UHashMap<K, V, B> {
H: Hasher,
{
// docs:end:contains_key
unsafe { self.get(key) }.is_some()
unsafe {
//@safety : unconstrained context
self.get(key)
}
.is_some()
}

// Returns true if the map contains no elements.
Expand Down Expand Up @@ -432,7 +436,10 @@ where
// Not marked as deleted and has key-value.
if equal & slot.is_valid() {
let (key, value) = slot.key_value_unchecked();
let other_value = unsafe { other.get(key) };
let other_value = unsafe {
//@safety : unconstrained context
other.get(key)
};

if other_value.is_none() {
equal = false;
Expand Down
Loading

0 comments on commit c1309e6

Please sign in to comment.