From 58247854fba4309991529317671280bccd5cf21f Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 29 Jul 2024 13:24:46 -0300 Subject: [PATCH 1/7] fix: correct span for prefix operator (#5624) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description ## Problem Resolves #5523 ## Summary The span of a Prefix expression was taken from its prefixed expression instead of from the entire expression. ## Additional Context I'm not sure how to test this 🤔 ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- compiler/noirc_frontend/src/elaborator/expressions.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/expressions.rs b/compiler/noirc_frontend/src/elaborator/expressions.rs index 1683409547e..7116ee0ac10 100644 --- a/compiler/noirc_frontend/src/elaborator/expressions.rs +++ b/compiler/noirc_frontend/src/elaborator/expressions.rs @@ -39,7 +39,7 @@ impl<'context> Elaborator<'context> { let (hir_expr, typ) = match expr.kind { ExpressionKind::Literal(literal) => self.elaborate_literal(literal, expr.span), ExpressionKind::Block(block) => self.elaborate_block(block), - ExpressionKind::Prefix(prefix) => return self.elaborate_prefix(*prefix), + ExpressionKind::Prefix(prefix) => return self.elaborate_prefix(*prefix, expr.span), ExpressionKind::Index(index) => self.elaborate_index(*index), ExpressionKind::Call(call) => self.elaborate_call(*call, expr.span), ExpressionKind::MethodCall(call) => self.elaborate_method_call(*call, expr.span), @@ -225,8 +225,7 @@ impl<'context> Elaborator<'context> { (HirExpression::Literal(HirLiteral::FmtStr(str, fmt_str_idents)), typ) } - fn elaborate_prefix(&mut self, prefix: PrefixExpression) -> (ExprId, Type) { - let span = prefix.rhs.span; + fn elaborate_prefix(&mut self, prefix: PrefixExpression, span: Span) -> (ExprId, Type) { let (rhs, rhs_type) = self.elaborate_expression(prefix.rhs); let trait_id = self.interner.get_prefix_operator_trait_method(&prefix.operator); From b3c408b62424c87f9be5b58c33be7d77e62af98e Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 29 Jul 2024 14:07:25 -0300 Subject: [PATCH 2/7] feat: turbofish in struct pattern (#5616) # Description ## Problem Part of https://github.com/noir-lang/noir/issues/5584 ## Summary Allows parsing turbofish in a struct pattern path segments, then uses those turbofish generics for type binding/inference. ## Additional Context None. ## Documentation Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- compiler/noirc_frontend/src/ast/statement.rs | 12 ++++ .../src/elaborator/expressions.rs | 23 ++---- .../noirc_frontend/src/elaborator/patterns.rs | 41 ++++++++++- .../noirc_frontend/src/elaborator/types.rs | 3 +- compiler/noirc_frontend/src/parser/parser.rs | 4 +- .../noirc_frontend/src/parser/parser/path.rs | 23 +++--- .../src/parser/parser/primitives.rs | 14 ++-- compiler/noirc_frontend/src/tests.rs | 71 +++++++++++++++++++ 8 files changed, 155 insertions(+), 36 deletions(-) diff --git a/compiler/noirc_frontend/src/ast/statement.rs b/compiler/noirc_frontend/src/ast/statement.rs index ac4da2892fb..8ce2e1a41c0 100644 --- a/compiler/noirc_frontend/src/ast/statement.rs +++ b/compiler/noirc_frontend/src/ast/statement.rs @@ -461,6 +461,18 @@ pub struct PathSegment { pub span: Span, } +impl PathSegment { + /// Returns the span where turbofish happen. For example: + /// + /// foo:: + /// ~^^^^ + /// + /// Returns an empty span at the end of `foo` if there's no turbofish. + pub fn turbofish_span(&self) -> Span { + Span::from(self.ident.span().end()..self.span.end()) + } +} + impl From for PathSegment { fn from(ident: Ident) -> PathSegment { let span = ident.span(); diff --git a/compiler/noirc_frontend/src/elaborator/expressions.rs b/compiler/noirc_frontend/src/elaborator/expressions.rs index 7116ee0ac10..295297cc738 100644 --- a/compiler/noirc_frontend/src/elaborator/expressions.rs +++ b/compiler/noirc_frontend/src/elaborator/expressions.rs @@ -429,23 +429,14 @@ impl<'context> Elaborator<'context> { } }; - let struct_generics = if let Some(turbofish_generics) = &last_segment.generics { - if turbofish_generics.len() == struct_generics.len() { - let struct_type = r#type.borrow(); - self.resolve_turbofish_generics(&struct_type.generics, turbofish_generics.clone()) - } else { - self.push_err(TypeCheckError::GenericCountMismatch { - item: format!("struct {}", last_segment.ident), - expected: struct_generics.len(), - found: turbofish_generics.len(), - span: Span::from(last_segment.ident.span().end()..last_segment.span.end()), - }); + let turbofish_span = last_segment.turbofish_span(); - struct_generics - } - } else { - struct_generics - }; + let struct_generics = self.resolve_struct_turbofish_generics( + &r#type.borrow(), + struct_generics, + last_segment.generics, + turbofish_span, + ); let struct_type = r#type.clone(); let generics = struct_generics.clone(); diff --git a/compiler/noirc_frontend/src/elaborator/patterns.rs b/compiler/noirc_frontend/src/elaborator/patterns.rs index 7aab8d1a24c..ade5420bce4 100644 --- a/compiler/noirc_frontend/src/elaborator/patterns.rs +++ b/compiler/noirc_frontend/src/elaborator/patterns.rs @@ -157,8 +157,12 @@ impl<'context> Elaborator<'context> { mutable: Option, new_definitions: &mut Vec, ) -> HirPattern { - let name_span = name.last_ident().span(); - let is_self_type = name.last_ident().is_self_type_name(); + let exclude_last_segment = true; + self.check_unsupported_turbofish_usage(&name, exclude_last_segment); + + let last_segment = name.last_segment(); + let name_span = last_segment.ident.span(); + let is_self_type = last_segment.ident.is_self_type_name(); let error_identifier = |this: &mut Self| { // Must create a name here to return a HirPattern::Identifier. Allowing @@ -178,6 +182,15 @@ impl<'context> Elaborator<'context> { } }; + let turbofish_span = last_segment.turbofish_span(); + + let generics = self.resolve_struct_turbofish_generics( + &struct_type.borrow(), + generics, + last_segment.generics, + turbofish_span, + ); + let actual_type = Type::Struct(struct_type.clone(), generics); let location = Location::new(span, self.file); @@ -426,6 +439,30 @@ impl<'context> Elaborator<'context> { }) } + pub(super) fn resolve_struct_turbofish_generics( + &mut self, + struct_type: &StructType, + generics: Vec, + unresolved_turbofish: Option>, + span: Span, + ) -> Vec { + let Some(turbofish_generics) = unresolved_turbofish else { + return generics; + }; + + if turbofish_generics.len() != generics.len() { + self.push_err(TypeCheckError::GenericCountMismatch { + item: format!("struct {}", struct_type.name), + expected: generics.len(), + found: turbofish_generics.len(), + span, + }); + return generics; + } + + self.resolve_turbofish_generics(&struct_type.generics, turbofish_generics) + } + pub(super) fn resolve_turbofish_generics( &mut self, generics: &[ResolvedGeneric], diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index 430967d8a51..ada6a3494a5 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -1623,8 +1623,7 @@ impl<'context> Elaborator<'context> { } if segment.generics.is_some() { - // From "foo::", create a span for just "::" - let span = Span::from(segment.ident.span().end()..segment.span.end()); + let span = segment.turbofish_span(); self.push_err(TypeCheckError::UnsupportedTurbofishUsage { span }); } } diff --git a/compiler/noirc_frontend/src/parser/parser.rs b/compiler/noirc_frontend/src/parser/parser.rs index 7c9656e3ec0..a7c62048283 100644 --- a/compiler/noirc_frontend/src/parser/parser.rs +++ b/compiler/noirc_frontend/src/parser/parser.rs @@ -557,7 +557,7 @@ fn pattern() -> impl NoirParser { .separated_by(just(Token::Comma)) .delimited_by(just(Token::LeftBrace), just(Token::RightBrace)); - let struct_pattern = path() + let struct_pattern = path(super::parse_type()) .then(struct_pattern_fields) .map_with_span(|(typename, fields), span| Pattern::Struct(typename, fields, span)); @@ -1128,7 +1128,7 @@ fn constructor(expr_parser: impl ExprParser) -> impl NoirParser .allow_trailing() .delimited_by(just(Token::LeftBrace), just(Token::RightBrace)); - path().then(args).map(ExpressionKind::constructor) + path(super::parse_type()).then(args).map(ExpressionKind::constructor) } fn constructor_field

(expr_parser: P) -> impl NoirParser<(Ident, Expression)> diff --git a/compiler/noirc_frontend/src/parser/parser/path.rs b/compiler/noirc_frontend/src/parser/parser/path.rs index 5565c392d59..140650af1a2 100644 --- a/compiler/noirc_frontend/src/parser/parser/path.rs +++ b/compiler/noirc_frontend/src/parser/parser/path.rs @@ -1,4 +1,4 @@ -use crate::ast::{Path, PathKind, PathSegment}; +use crate::ast::{Path, PathKind, PathSegment, UnresolvedType}; use crate::parser::NoirParser; use crate::token::{Keyword, Token}; @@ -8,8 +8,10 @@ use chumsky::prelude::*; use super::keyword; use super::primitives::{path_segment, path_segment_no_turbofish}; -pub(super) fn path() -> impl NoirParser { - path_inner(path_segment()) +pub(super) fn path<'a>( + type_parser: impl NoirParser + 'a, +) -> impl NoirParser + 'a { + path_inner(path_segment(type_parser)) } pub(super) fn path_no_turbofish() -> impl NoirParser { @@ -40,13 +42,16 @@ fn empty_path() -> impl NoirParser { } pub(super) fn maybe_empty_path() -> impl NoirParser { - path().or(empty_path()) + path_no_turbofish().or(empty_path()) } #[cfg(test)] mod test { use super::*; - use crate::parser::parser::test_helpers::{parse_all_failing, parse_with}; + use crate::parser::{ + parse_type, + parser::test_helpers::{parse_all_failing, parse_with}, + }; #[test] fn parse_path() { @@ -59,13 +64,13 @@ mod test { ]; for (src, expected_segments) in cases { - let path: Path = parse_with(path(), src).unwrap(); + let path: Path = parse_with(path(parse_type()), src).unwrap(); for (segment, expected) in path.segments.into_iter().zip(expected_segments) { assert_eq!(segment.ident.0.contents, expected); } } - parse_all_failing(path(), vec!["std::", "::std", "std::hash::", "foo::1"]); + parse_all_failing(path(parse_type()), vec!["std::", "::std", "std::hash::", "foo::1"]); } #[test] @@ -78,12 +83,12 @@ mod test { ]; for (src, expected_path_kind) in cases { - let path = parse_with(path(), src).unwrap(); + let path = parse_with(path(parse_type()), src).unwrap(); assert_eq!(path.kind, expected_path_kind); } parse_all_failing( - path(), + path(parse_type()), vec!["crate", "crate::std::crate", "foo::bar::crate", "foo::dep"], ); } diff --git a/compiler/noirc_frontend/src/parser/parser/primitives.rs b/compiler/noirc_frontend/src/parser/parser/primitives.rs index eb8d67b751a..25f693bf504 100644 --- a/compiler/noirc_frontend/src/parser/parser/primitives.rs +++ b/compiler/noirc_frontend/src/parser/parser/primitives.rs @@ -33,10 +33,14 @@ pub(super) fn token_kind(token_kind: TokenKind) -> impl NoirParser { }) } -pub(super) fn path_segment() -> impl NoirParser { - ident() - .then(turbofish(super::parse_type())) - .map_with_span(|(ident, generics), span| PathSegment { ident, generics, span }) +pub(super) fn path_segment<'a>( + type_parser: impl NoirParser + 'a, +) -> impl NoirParser + 'a { + ident().then(turbofish(type_parser)).map_with_span(|(ident, generics), span| PathSegment { + ident, + generics, + span, + }) } pub(super) fn path_segment_no_turbofish() -> impl NoirParser { @@ -96,7 +100,7 @@ pub(super) fn turbofish<'a>( } pub(super) fn variable() -> impl NoirParser { - path().map(ExpressionKind::Variable) + path(super::parse_type()).map(ExpressionKind::Variable) } pub(super) fn variable_no_turbofish() -> impl NoirParser { diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index f2b83a48022..c23870bbb43 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -2610,3 +2610,74 @@ fn turbofish_in_middle_of_variable_unsupported_yet() { CompilationError::TypeError(TypeCheckError::UnsupportedTurbofishUsage { .. }), )); } + +#[test] +fn turbofish_in_struct_pattern() { + let src = r#" + struct Foo { + x: T + } + + fn main() { + let value: Field = 0; + let Foo:: { x } = Foo { x: value }; + let _ = x; + } + "#; + assert_no_errors(src); +} + +#[test] +fn turbofish_in_struct_pattern_errors_if_type_mismatch() { + let src = r#" + struct Foo { + x: T + } + + fn main() { + let value: Field = 0; + let Foo:: { x } = Foo { x: value }; + let _ = x; + } + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::TypeError(TypeCheckError::TypeMismatchWithSource { .. }) = &errors[0].0 + else { + panic!("Expected a type mismatch error, got {:?}", errors[0].0); + }; +} + +#[test] +fn turbofish_in_struct_pattern_generic_count_mismatch() { + let src = r#" + struct Foo { + x: T + } + + fn main() { + let value = 0; + let Foo:: { x } = Foo { x: value }; + let _ = x; + } + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::TypeError(TypeCheckError::GenericCountMismatch { + item, + expected, + found, + .. + }) = &errors[0].0 + else { + panic!("Expected a generic count mismatch error, got {:?}", errors[0].0); + }; + + assert_eq!(item, "struct Foo"); + assert_eq!(*expected, 1); + assert_eq!(*found, 2); +} From b33495d0799f7c296cf6e284ea19abbbe5821793 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 29 Jul 2024 15:37:26 -0300 Subject: [PATCH 3/7] feat: allow inserting LSP inlay type hints (#5620) # Description ## Problem Resolves #5527 ## Summary Makes type hints insertable, though not all of them can be inserted. For example a for variable can't have a type annotation, and a struct member pattern can't either. I also added a test for when the type hint is shown for a struct member pattern, which was missing (mainly to assert that the type there isn't insertable). https://github.com/user-attachments/assets/b3a02f2b-be82-49b5-9ac5-cebf8cb83214 ## Additional Context None. ## Documentation Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- tooling/lsp/src/requests/inlay_hint.rs | 88 ++++++++++++++++--- .../lsp/test_programs/inlay_hints/src/main.nr | 3 + 2 files changed, 78 insertions(+), 13 deletions(-) diff --git a/tooling/lsp/src/requests/inlay_hint.rs b/tooling/lsp/src/requests/inlay_hint.rs index 3fc6a6752df..48d72fa0e9a 100644 --- a/tooling/lsp/src/requests/inlay_hint.rs +++ b/tooling/lsp/src/requests/inlay_hint.rs @@ -4,8 +4,8 @@ use std::future::{self, Future}; use async_lsp::ResponseError; use fm::{FileId, FileMap, PathString}; use lsp_types::{ - InlayHint, InlayHintKind, InlayHintLabel, InlayHintLabelPart, InlayHintParams, Position, - TextDocumentPositionParams, + InlayHint, InlayHintKind, InlayHintLabel, InlayHintLabelPart, InlayHintParams, Position, Range, + TextDocumentPositionParams, TextEdit, }; use noirc_errors::{Location, Span}; use noirc_frontend::{ @@ -173,7 +173,7 @@ impl<'a> InlayHintCollector<'a> { self.collect_in_expression(&assign_statement.expression); } StatementKind::For(for_loop_statement) => { - self.collect_in_ident(&for_loop_statement.identifier); + self.collect_in_ident(&for_loop_statement.identifier, false); self.collect_in_expression(&for_loop_statement.block); } StatementKind::Comptime(statement) => self.collect_in_statement(statement), @@ -276,7 +276,7 @@ impl<'a> InlayHintCollector<'a> { match pattern { Pattern::Identifier(ident) => { - self.collect_in_ident(ident); + self.collect_in_ident(ident, true); } Pattern::Mutable(pattern, _span, _is_synthesized) => { self.collect_in_pattern(pattern); @@ -294,7 +294,7 @@ impl<'a> InlayHintCollector<'a> { } } - fn collect_in_ident(&mut self, ident: &Ident) { + fn collect_in_ident(&mut self, ident: &Ident, editable: bool) { if !self.options.type_hints.enabled { return; } @@ -308,17 +308,17 @@ impl<'a> InlayHintCollector<'a> { let global_info = self.interner.get_global(global_id); let definition_id = global_info.definition_id; let typ = self.interner.definition_type(definition_id); - self.push_type_hint(lsp_location, &typ); + self.push_type_hint(lsp_location, &typ, editable); } ReferenceId::Local(definition_id) => { let typ = self.interner.definition_type(definition_id); - self.push_type_hint(lsp_location, &typ); + self.push_type_hint(lsp_location, &typ, editable); } ReferenceId::StructMember(struct_id, field_index) => { let struct_type = self.interner.get_struct(struct_id); let struct_type = struct_type.borrow(); let (_field_name, field_type) = struct_type.field_at(field_index); - self.push_type_hint(lsp_location, field_type); + self.push_type_hint(lsp_location, field_type, false); } ReferenceId::Module(_) | ReferenceId::Struct(_) @@ -331,7 +331,7 @@ impl<'a> InlayHintCollector<'a> { } } - fn push_type_hint(&mut self, location: lsp_types::Location, typ: &Type) { + fn push_type_hint(&mut self, location: lsp_types::Location, typ: &Type, editable: bool) { let position = location.range.end; let mut parts = Vec::new(); @@ -342,7 +342,14 @@ impl<'a> InlayHintCollector<'a> { position, label: InlayHintLabel::LabelParts(parts), kind: Some(InlayHintKind::TYPE), - text_edits: None, + text_edits: if editable { + Some(vec![TextEdit { + range: Range { start: location.range.end, end: location.range.end }, + new_text: format!(": {}", typ), + }]) + } else { + None + }, tooltip: None, padding_left: None, padding_right: None, @@ -756,8 +763,10 @@ mod inlay_hints_tests { let inlay_hints = get_inlay_hints(0, 3, type_hints()).await; assert_eq!(inlay_hints.len(), 1); + let position = Position { line: 1, character: 11 }; + let inlay_hint = &inlay_hints[0]; - assert_eq!(inlay_hint.position, Position { line: 1, character: 11 }); + assert_eq!(inlay_hint.position, position); if let InlayHintLabel::LabelParts(labels) = &inlay_hint.label { assert_eq!(labels.len(), 2); @@ -770,6 +779,14 @@ mod inlay_hints_tests { } else { panic!("Expected InlayHintLabel::LabelParts, got {:?}", inlay_hint.label); } + + assert_eq!( + inlay_hint.text_edits, + Some(vec![TextEdit { + range: Range { start: position, end: position }, + new_text: ": Field".to_string(), + }]) + ); } #[test] @@ -777,8 +794,10 @@ mod inlay_hints_tests { let inlay_hints = get_inlay_hints(12, 15, type_hints()).await; assert_eq!(inlay_hints.len(), 1); + let position = Position { line: 13, character: 11 }; + let inlay_hint = &inlay_hints[0]; - assert_eq!(inlay_hint.position, Position { line: 13, character: 11 }); + assert_eq!(inlay_hint.position, position); if let InlayHintLabel::LabelParts(labels) = &inlay_hint.label { assert_eq!(labels.len(), 2); @@ -798,6 +817,34 @@ mod inlay_hints_tests { } else { panic!("Expected InlayHintLabel::LabelParts, got {:?}", inlay_hint.label); } + + assert_eq!( + inlay_hint.text_edits, + Some(vec![TextEdit { + range: Range { start: position, end: position }, + new_text: ": Foo".to_string(), + }]) + ); + } + + #[test] + async fn test_type_inlay_hints_in_struct_member_pattern() { + let inlay_hints = get_inlay_hints(94, 96, type_hints()).await; + assert_eq!(inlay_hints.len(), 1); + + let inlay_hint = &inlay_hints[0]; + assert_eq!(inlay_hint.position, Position { line: 95, character: 24 }); + + if let InlayHintLabel::LabelParts(labels) = &inlay_hint.label { + assert_eq!(labels.len(), 2); + assert_eq!(labels[0].value, ": "); + assert_eq!(labels[0].location, None); + assert_eq!(labels[1].value, "i32"); + } else { + panic!("Expected InlayHintLabel::LabelParts, got {:?}", inlay_hint.label); + } + + assert_eq!(inlay_hint.text_edits, None); } #[test] @@ -816,6 +863,8 @@ mod inlay_hints_tests { } else { panic!("Expected InlayHintLabel::LabelParts, got {:?}", inlay_hint.label); } + + assert_eq!(inlay_hint.text_edits, None); } #[test] @@ -823,8 +872,10 @@ mod inlay_hints_tests { let inlay_hints = get_inlay_hints(19, 21, type_hints()).await; assert_eq!(inlay_hints.len(), 1); + let position = Position { line: 20, character: 10 }; + let inlay_hint = &inlay_hints[0]; - assert_eq!(inlay_hint.position, Position { line: 20, character: 10 }); + assert_eq!(inlay_hint.position, position); if let InlayHintLabel::LabelParts(labels) = &inlay_hint.label { assert_eq!(labels.len(), 2); @@ -834,6 +885,14 @@ mod inlay_hints_tests { } else { panic!("Expected InlayHintLabel::LabelParts, got {:?}", inlay_hint.label); } + + assert_eq!( + inlay_hint.text_edits, + Some(vec![TextEdit { + range: Range { start: position, end: position }, + new_text: ": Field".to_string(), + }]) + ); } #[test] @@ -855,6 +914,7 @@ mod inlay_hints_tests { let inlay_hint = &inlay_hints[0]; assert_eq!(inlay_hint.position, Position { line: 25, character: 12 }); + assert_eq!(inlay_hint.text_edits, None); if let InlayHintLabel::String(label) = &inlay_hint.label { assert_eq!(label, "one: "); } else { @@ -863,6 +923,7 @@ mod inlay_hints_tests { let inlay_hint = &inlay_hints[1]; assert_eq!(inlay_hint.position, Position { line: 25, character: 15 }); + assert_eq!(inlay_hint.text_edits, None); if let InlayHintLabel::String(label) = &inlay_hint.label { assert_eq!(label, "two: "); } else { @@ -877,6 +938,7 @@ mod inlay_hints_tests { let inlay_hint = &inlay_hints[0]; assert_eq!(inlay_hint.position, Position { line: 38, character: 18 }); + assert_eq!(inlay_hint.text_edits, None); if let InlayHintLabel::String(label) = &inlay_hint.label { assert_eq!(label, "one: "); } else { diff --git a/tooling/lsp/test_programs/inlay_hints/src/main.nr b/tooling/lsp/test_programs/inlay_hints/src/main.nr index 2b53f8de339..762bb6300f8 100644 --- a/tooling/lsp/test_programs/inlay_hints/src/main.nr +++ b/tooling/lsp/test_programs/inlay_hints/src/main.nr @@ -92,3 +92,6 @@ fn call_yet_another_function() { yet_another_function(some_name) // Should not show parameter names ("name" is a suffix of "some_name") } +fn struct_member_hint() { + let SomeStruct { one } = SomeStruct { one: 1 }; +} \ No newline at end of file From 1f5d0007430cd5cf057ce61ebc87304bb8cb557c Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 29 Jul 2024 16:00:07 -0300 Subject: [PATCH 4/7] fix: error on incorrect generic count for impl and type alias (#5623) # Description ## Problem Resolves #5583 ## Summary There's a call, `verify_generics_count`, that's supposed to do this check. The problem is that the `args` given to it are the results of zipping the struct's generics with the given generics. That will always produce an `args` the size of the smallest of the two. So, if a struct doesn't have generics, `args` will end up empty and no error is produced. However, if a struct has more generics than given, an error was correctly produced. The solution is to get the actual and expected numbers before shadowing `args`. ## Additional Context In Rust this program gives two errors: ```rust struct Foo {} impl Foo {} fn main() { } ``` 1. cannot find T in this scope 2. struct takes 0 generic arguments but 1 generic argument was supplied With this PR, in Noir we'll be just giving one error (the second one). The reason is that only generics that match the struct generics count are checked. I thought about changing the code to produce the same number of errors as Rust... but I didn't know if it was worth it. Here you'll get the "incorrect generics count" error, so you'll have to fix that by either removing the generic (solved) or by adding a generic to `struct Foo` (likely not what you are going to do), at which point you'll get the other error... so I thought that with just one error it's good enough. ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- .../noirc_frontend/src/elaborator/types.rs | 28 +++++++---- compiler/noirc_frontend/src/tests.rs | 48 +++++++++++++++++++ 2 files changed, 68 insertions(+), 8 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index ada6a3494a5..c134820811e 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -239,6 +239,7 @@ impl<'context> Elaborator<'context> { if let Some(type_alias) = self.lookup_type_alias(path.clone()) { let type_alias = type_alias.borrow(); + let actual_generic_count = args.len(); let expected_generic_count = type_alias.generics.len(); let type_alias_string = type_alias.to_string(); let id = type_alias.id; @@ -247,9 +248,13 @@ impl<'context> Elaborator<'context> { self.resolve_type_inner(arg, &generic.kind) }); - self.verify_generics_count(expected_generic_count, &mut args, span, || { - type_alias_string - }); + self.verify_generics_count( + expected_generic_count, + actual_generic_count, + &mut args, + span, + || type_alias_string, + ); if let Some(item) = self.current_item { self.interner.add_type_alias_dependency(item, id); @@ -279,6 +284,8 @@ impl<'context> Elaborator<'context> { } let expected_generic_count = struct_type.borrow().generics.len(); + let actual_generic_count = args.len(); + if !self.in_contract() && self .interner @@ -296,9 +303,13 @@ impl<'context> Elaborator<'context> { self.resolve_type_inner(arg, &generic.kind) }); - self.verify_generics_count(expected_generic_count, &mut args, span, || { - struct_type.borrow().to_string() - }); + self.verify_generics_count( + expected_generic_count, + actual_generic_count, + &mut args, + span, + || struct_type.borrow().to_string(), + ); if let Some(current_item) = self.current_item { let dependency_id = struct_type.borrow().id; @@ -333,15 +344,16 @@ impl<'context> Elaborator<'context> { fn verify_generics_count( &mut self, expected_count: usize, + actual_count: usize, args: &mut Vec, span: Span, type_name: impl FnOnce() -> String, ) { - if args.len() != expected_count { + if actual_count != expected_count { self.push_err(ResolverError::IncorrectGenericCount { span, item_name: type_name(), - actual: args.len(), + actual: actual_count, expected: expected_count, }); diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index c23870bbb43..a21259a4f0d 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -2681,3 +2681,51 @@ fn turbofish_in_struct_pattern_generic_count_mismatch() { assert_eq!(*expected, 1); assert_eq!(*found, 2); } + +#[test] +fn incorrect_generic_count_on_struct_impl() { + let src = r#" + struct Foo {} + impl Foo {} + fn main() {} + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::ResolverError(ResolverError::IncorrectGenericCount { + actual, + expected, + .. + }) = errors[0].0 + else { + panic!("Expected an incorrect generic count mismatch error, got {:?}", errors[0].0); + }; + + assert_eq!(actual, 1); + assert_eq!(expected, 0); +} + +#[test] +fn incorrect_generic_count_on_type_alias() { + let src = r#" + struct Foo {} + type Bar = Foo; + fn main() {} + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::ResolverError(ResolverError::IncorrectGenericCount { + actual, + expected, + .. + }) = errors[0].0 + else { + panic!("Expected an incorrect generic count mismatch error, got {:?}", errors[0].0); + }; + + assert_eq!(actual, 1); + assert_eq!(expected, 0); +} From af1636c5fd1719ca9de0a8691252af25a5e0d895 Mon Sep 17 00:00:00 2001 From: jfecher Date: Mon, 29 Jul 2024 14:49:07 -0500 Subject: [PATCH 5/7] chore: Switch `Value::TraitConstraint` to a resolved trait constraint (#5618) # Description ## Problem\* Resolves ## Summary\* Changes `Value::TraitConstraint` to hold a resolved trait constraint rather than an unresolved one. This was the original intention of the type but wasn't possible at the time since we didn't have access to the elaborator to resolve it. Keeping it unresolved lead to issues with hashing being different for constraints with different spans or with some spans having `Some(trait_id)` and it being `None` for others. Now they're all uniform. ## Additional Context With this, the `derive` example now passes! Putting it in the stdlib gives some odd errors for now though, so I'm adding it as a test and will add it to the stdlib in a later PR. ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- compiler/noirc_frontend/src/elaborator/mod.rs | 6 ++- .../noirc_frontend/src/hir/comptime/errors.rs | 9 +++- .../src/hir/comptime/interpreter.rs | 3 ++ .../src/hir/comptime/interpreter/builtin.rs | 39 +++++++------- .../noirc_frontend/src/hir/comptime/value.rs | 8 +-- .../execution_success/derive/Nargo.toml | 7 +++ .../execution_success/derive/src/main.nr | 51 +++++++++++++++++++ 7 files changed, 95 insertions(+), 28 deletions(-) create mode 100644 test_programs/execution_success/derive/Nargo.toml create mode 100644 test_programs/execution_success/derive/src/main.nr diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 84572a7b413..d224e38372f 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -618,7 +618,11 @@ impl<'context> Elaborator<'context> { self.resolve_trait_bound(&constraint.trait_bound, typ) } - fn resolve_trait_bound(&mut self, bound: &TraitBound, typ: Type) -> Option { + pub fn resolve_trait_bound( + &mut self, + bound: &TraitBound, + typ: Type, + ) -> Option { let the_trait = self.lookup_trait_or_error(bound.trait_path.clone())?; let resolved_generics = &the_trait.generics.clone(); diff --git a/compiler/noirc_frontend/src/hir/comptime/errors.rs b/compiler/noirc_frontend/src/hir/comptime/errors.rs index 505aa6c67c8..137433b48ef 100644 --- a/compiler/noirc_frontend/src/hir/comptime/errors.rs +++ b/compiler/noirc_frontend/src/hir/comptime/errors.rs @@ -2,6 +2,7 @@ use std::fmt::Display; use std::rc::Rc; use crate::{ + ast::TraitBound, hir::{def_collector::dc_crate::CompilationError, type_check::NoMatchingImplFoundError}, parser::ParserError, token::Tokens, @@ -56,6 +57,7 @@ pub enum InterpreterError { BreakNotInLoop { location: Location }, ContinueNotInLoop { location: Location }, BlackBoxError(BlackBoxResolutionError, Location), + FailedToResolveTraitBound { trait_bound: TraitBound, location: Location }, Unimplemented { item: String, location: Location }, @@ -119,7 +121,8 @@ impl InterpreterError { | InterpreterError::DebugEvaluateComptime { location, .. } | InterpreterError::BlackBoxError(_, location) | InterpreterError::BreakNotInLoop { location, .. } - | InterpreterError::ContinueNotInLoop { location, .. } => *location, + | InterpreterError::ContinueNotInLoop { location, .. } + | InterpreterError::FailedToResolveTraitBound { location, .. } => *location, InterpreterError::FailedToParseMacro { error, file, .. } => { Location::new(error.span(), *file) @@ -373,6 +376,10 @@ impl<'a> From<&'a InterpreterError> for CustomDiagnostic { InterpreterError::BlackBoxError(error, location) => { CustomDiagnostic::simple_error(error.to_string(), String::new(), location.span) } + InterpreterError::FailedToResolveTraitBound { trait_bound, location } => { + let msg = format!("Failed to resolve trait bound `{trait_bound}`"); + CustomDiagnostic::simple_error(msg, String::new(), location.span) + } InterpreterError::NoMatchingImplFound { error, .. } => error.into(), InterpreterError::Break => unreachable!("Uncaught InterpreterError::Break"), InterpreterError::Continue => unreachable!("Uncaught InterpreterError::Continue"), diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs index cb507659887..a3d37fd76fc 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs @@ -199,6 +199,9 @@ impl<'local, 'interner> Interpreter<'local, 'interner> { } else if let Some(oracle) = func_attrs.oracle() { if oracle == "print" { self.print_oracle(arguments) + // Ignore debugger functions + } else if oracle.starts_with("__debug") { + Ok(Value::Unit) } else { let item = format!("Comptime evaluation for oracle functions like {oracle}"); Err(InterpreterError::Unimplemented { item, location }) diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs index 92890cb66b8..b35790fd3d4 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs @@ -6,13 +6,13 @@ use std::{ use acvm::{AcirField, FieldElement}; use chumsky::Parser; use iter_extended::{try_vecmap, vecmap}; -use noirc_errors::{Location, Span}; +use noirc_errors::Location; use rustc_hash::FxHashMap as HashMap; use crate::{ - ast::{IntegerBitSize, TraitBound}, + ast::IntegerBitSize, hir::comptime::{errors::IResult, InterpreterError, Value}, - macros_api::{NodeInterner, Path, Signedness, UnresolvedTypeData}, + macros_api::{NodeInterner, Signedness}, node_interner::TraitId, parser, token::{SpannedToken, Token, Tokens}, @@ -53,9 +53,7 @@ impl<'local, 'context> Interpreter<'local, 'context> { "trait_def_as_trait_constraint" => { trait_def_as_trait_constraint(interner, arguments, location) } - "quoted_as_trait_constraint" => { - quoted_as_trait_constraint(interner, arguments, location) - } + "quoted_as_trait_constraint" => quoted_as_trait_constraint(self, arguments, location), "quoted_as_type" => quoted_as_type(self, arguments, location), "zeroed" => zeroed(return_type), _ => { @@ -133,9 +131,9 @@ pub(super) fn get_u32(value: Value, location: Location) -> IResult { } } -fn get_trait_constraint(value: Value, location: Location) -> IResult { +fn get_trait_constraint(value: Value, location: Location) -> IResult<(TraitId, Vec)> { match value { - Value::TraitConstraint(bound) => Ok(bound), + Value::TraitConstraint(trait_id, generics) => Ok((trait_id, generics)), value => { let expected = Type::Quoted(QuotedType::TraitConstraint); Err(InterpreterError::TypeMismatch { expected, value, location }) @@ -387,7 +385,7 @@ fn slice_insert( // fn as_trait_constraint(quoted: Quoted) -> TraitConstraint fn quoted_as_trait_constraint( - _interner: &mut NodeInterner, + interpreter: &mut Interpreter, mut arguments: Vec<(Value, Location)>, location: Location, ) -> IResult { @@ -402,7 +400,14 @@ fn quoted_as_trait_constraint( InterpreterError::FailedToParseMacro { error, tokens, rule, file: location.file } })?; - Ok(Value::TraitConstraint(trait_bound)) + let bound = interpreter + .elaborator + .elaborate_item_from_comptime(interpreter.current_function, |elaborator| { + elaborator.resolve_trait_bound(&trait_bound, Type::Unit) + }) + .ok_or(InterpreterError::FailedToResolveTraitBound { trait_bound, location })?; + + Ok(Value::TraitConstraint(bound.trait_id, bound.trait_generics)) } // fn as_type(quoted: Quoted) -> Type @@ -529,11 +534,6 @@ fn zeroed(return_type: Type) -> IResult { let element = zeroed(*element)?; Ok(Value::Pointer(Shared::new(element), false)) } - Type::Quoted(QuotedType::TraitConstraint) => Ok(Value::TraitConstraint(TraitBound { - trait_path: Path::from_single(String::new(), Span::default()), - trait_id: None, - trait_generics: Vec::new(), - })), // Optimistically assume we can resolve this type later or that the value is unused Type::TypeVariable(_, _) | Type::Forall(_, _) @@ -618,14 +618,9 @@ fn trait_def_as_trait_constraint( let trait_id = get_trait_def(arguments.pop().unwrap().0, location)?; let the_trait = interner.get_trait(trait_id); - - let trait_path = Path::from_ident(the_trait.name.clone()); - let trait_generics = vecmap(&the_trait.generics, |generic| { - let name = Path::from_single(generic.name.as_ref().clone(), generic.span); - UnresolvedTypeData::Named(name, Vec::new(), false).with_span(generic.span) + Type::NamedGeneric(generic.type_var.clone(), generic.name.clone(), generic.kind.clone()) }); - let trait_id = Some(trait_id); - Ok(Value::TraitConstraint(TraitBound { trait_path, trait_id, trait_generics })) + Ok(Value::TraitConstraint(trait_id, trait_generics)) } diff --git a/compiler/noirc_frontend/src/hir/comptime/value.rs b/compiler/noirc_frontend/src/hir/comptime/value.rs index d20372e6812..0959e4c17ac 100644 --- a/compiler/noirc_frontend/src/hir/comptime/value.rs +++ b/compiler/noirc_frontend/src/hir/comptime/value.rs @@ -7,7 +7,7 @@ use iter_extended::{try_vecmap, vecmap}; use noirc_errors::Location; use crate::{ - ast::{ArrayLiteral, ConstructorExpression, Ident, IntegerBitSize, Signedness, TraitBound}, + ast::{ArrayLiteral, ConstructorExpression, Ident, IntegerBitSize, Signedness}, hir::def_map::ModuleId, hir_def::expr::{HirArrayLiteral, HirConstructorExpression, HirIdent, HirLambda, ImplKind}, macros_api::{ @@ -48,7 +48,7 @@ pub enum Value { Slice(Vector, Type), Code(Rc), StructDefinition(StructId), - TraitConstraint(TraitBound), + TraitConstraint(TraitId, /* trait generics */ Vec), TraitDefinition(TraitId), FunctionDefinition(FuncId), ModuleDefinition(ModuleId), @@ -222,7 +222,7 @@ impl Value { } Value::Pointer(..) | Value::StructDefinition(_) - | Value::TraitConstraint(_) + | Value::TraitConstraint(..) | Value::TraitDefinition(_) | Value::FunctionDefinition(_) | Value::Zeroed(_) @@ -342,7 +342,7 @@ impl Value { Value::Code(block) => HirExpression::Unquote(unwrap_rc(block)), Value::Pointer(..) | Value::StructDefinition(_) - | Value::TraitConstraint(_) + | Value::TraitConstraint(..) | Value::TraitDefinition(_) | Value::FunctionDefinition(_) | Value::Zeroed(_) diff --git a/test_programs/execution_success/derive/Nargo.toml b/test_programs/execution_success/derive/Nargo.toml new file mode 100644 index 00000000000..f3846594305 --- /dev/null +++ b/test_programs/execution_success/derive/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "derive" +type = "bin" +authors = [""] +compiler_version = ">=0.31.0" + +[dependencies] \ No newline at end of file diff --git a/test_programs/execution_success/derive/src/main.nr b/test_programs/execution_success/derive/src/main.nr new file mode 100644 index 00000000000..f226817fbaf --- /dev/null +++ b/test_programs/execution_success/derive/src/main.nr @@ -0,0 +1,51 @@ +use std::collections::umap::UHashMap; +use std::hash::BuildHasherDefault; +use std::hash::poseidon2::Poseidon2Hasher; + +#[my_derive(DoNothing)] +struct MyStruct { my_field: u32 } + +type DeriveFunction = fn(StructDefinition) -> Quoted; + +comptime mut global HANDLERS: UHashMap> = UHashMap::default(); + +comptime fn my_derive(s: StructDefinition, traits: [Quoted]) -> Quoted { + let mut result = quote {}; + + for trait_to_derive in traits { + let handler = HANDLERS.get(trait_to_derive.as_trait_constraint()); + assert(handler.is_some(), f"No derive function registered for `{trait_to_derive}`"); + + let trait_impl = handler.unwrap()(s); + result = quote { $result $trait_impl }; + } + + result +} + +unconstrained comptime fn my_derive_via(t: TraitDefinition, f: Quoted) { + HANDLERS.insert(t.as_trait_constraint(), std::meta::unquote!(f)); +} + +#[my_derive_via(derive_do_nothing)] +trait DoNothing { + fn do_nothing(self); +} + +comptime fn derive_do_nothing(s: StructDefinition) -> Quoted { + let typ = s.as_type(); + let generics = s.generics().join(quote {,}); + quote { + impl<$generics> DoNothing for $typ { + fn do_nothing(_self: Self) { + // Traits can't tell us what to do + println("something"); + } + } + } +} + +fn main() { + let s = MyStruct { my_field: 1 }; + s.do_nothing(); +} From 5ef9daa8fb8d55b194d38d540a79dc29f0090351 Mon Sep 17 00:00:00 2001 From: Savio <72797635+Savio-Sou@users.noreply.github.com> Date: Tue, 30 Jul 2024 06:31:17 +0900 Subject: [PATCH 6/7] chore(github): Switch to organization-wide Issue templates (#5622) # Description ## Summary\* Following https://github.com/noir-lang/.github/pull/29, we can now pull directly from the organization-wide Issue templates and simplify templates to maintain. This PR deletes the repo-specific Issue templates, so GitHub would then automatically pull the org-wide templates. ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. Preview the templates: https://github.com/noir-lang-test/noir/issues/new/choose --- .github/ISSUE_TEMPLATE/bug_report.yml | 120 --------------------- .github/ISSUE_TEMPLATE/feature_request.yml | 71 ------------ 2 files changed, 191 deletions(-) delete mode 100644 .github/ISSUE_TEMPLATE/bug_report.yml delete mode 100644 .github/ISSUE_TEMPLATE/feature_request.yml diff --git a/.github/ISSUE_TEMPLATE/bug_report.yml b/.github/ISSUE_TEMPLATE/bug_report.yml deleted file mode 100644 index 71207793e53..00000000000 --- a/.github/ISSUE_TEMPLATE/bug_report.yml +++ /dev/null @@ -1,120 +0,0 @@ -name: Bug Report -description: Report an unexpected behavior. -labels: ["bug"] -body: - - type: markdown - attributes: - value: | - # Description - Thanks for taking the time to create the Issue and welcome to the Noir community! - - type: textarea - id: aim - attributes: - label: Aim - description: Describe what you tried to achieve. - validations: - required: true - - type: textarea - id: expected - attributes: - label: Expected Behavior - description: Describe what you expected to happen. - validations: - required: true - - type: textarea - id: bug - attributes: - label: Bug - description: Describe the bug. Supply error codes / terminal logs if applicable. - validations: - required: true - - type: textarea - id: reproduction - attributes: - label: To Reproduce - description: Describe the steps to reproduce the behavior. - value: | - 1. - 2. - 3. - 4. - - type: dropdown - id: impact - attributes: - label: Project Impact - description: How does this affect a project you or others are working on? - options: - - "Nice-to-have" - - "Blocker" - - type: textarea - id: impact_context - attributes: - label: Impact Context - description: If a nice-to-have / blocker, supplement how does this Issue affect the project. - - type: dropdown - id: workaround - attributes: - label: Workaround - description: Is there a workaround for this Issue? - options: - - "Yes" - - type: textarea - id: workaround_description - attributes: - label: Workaround Description - description: If yes, supplement how could the Issue be worked around. - - type: textarea - id: additional - attributes: - label: Additional Context - description: Supplement further information if applicable. - - type: markdown - attributes: - value: | - # Environment - Specify your version of Noir tooling used. - - type: markdown - attributes: - value: | - ## Nargo (CLI) - - type: dropdown - id: nargo-install - attributes: - label: Installation Method - description: How did you install Nargo? - options: - - Binary (`noirup` default) - - Compiled from source - - type: input - id: nargo-version - attributes: - label: Nargo Version - description: Output of running `nargo --version` - placeholder: "nargo version = 0.23.0 noirc version = 0.23.0+5be9f9d7e2f39ca228df10e5a530474af0331704 (git version hash: 5be9f9d7e2f39ca228df10e5a530474af0331704, is dirty: false)" - - type: markdown - attributes: - value: | - ## NoirJS (JavaScript) - - type: input - id: noirjs-version - attributes: - label: NoirJS Version - description: Version number of `noir_js` in `package.json` - placeholder: "0.23.0" - - type: markdown - attributes: - value: | - # Pull Request - - type: dropdown - id: pr_preference - attributes: - label: Would you like to submit a PR for this Issue? - description: Fellow contributors are happy to provide support where applicable. - options: - - "Maybe" - - "Yes" - - type: textarea - id: pr_support - attributes: - label: Support Needs - description: Support from other contributors you are looking for to create a PR for this Issue. diff --git a/.github/ISSUE_TEMPLATE/feature_request.yml b/.github/ISSUE_TEMPLATE/feature_request.yml deleted file mode 100644 index abbfe392454..00000000000 --- a/.github/ISSUE_TEMPLATE/feature_request.yml +++ /dev/null @@ -1,71 +0,0 @@ -name: Feature Request -description: Suggest an idea for this project. -labels: ["enhancement"] -body: - - type: markdown - attributes: - value: | - ## Description - Thanks for taking the time to create the Issue and welcome to the Noir community! - - type: textarea - id: problem - attributes: - label: Problem - description: Describe what you feel lacking. Supply code / step-by-step examples if applicable. - validations: - required: true - - type: textarea - id: solution - attributes: - label: Happy Case - description: Describe how you think it should work. Supply pseudocode / step-by-step examples if applicable. - validations: - required: true - - type: dropdown - id: impact - attributes: - label: Project Impact - description: How does this affect a project you or others are working on? - options: - - "Nice-to-have" - - "Blocker" - - type: textarea - id: impact_context - attributes: - label: Impact Context - description: If a nice-to-have / blocker, supplement how does this Issue affect the project. - - type: dropdown - id: workaround - attributes: - label: Workaround - description: Is there a workaround for this Issue? - options: - - "Yes" - - type: textarea - id: workaround_description - attributes: - label: Workaround Description - description: If yes, supplement how could the Issue be worked around. - - type: textarea - id: additional - attributes: - label: Additional Context - description: Supplement further information if applicable. - - type: markdown - attributes: - value: | - ## Pull Request - - type: dropdown - id: pr-preference - attributes: - label: Would you like to submit a PR for this Issue? - description: Fellow contributors are happy to provide support where applicable. - multiple: false - options: - - "Maybe" - - "Yes" - - type: textarea - id: pr-support - attributes: - label: Support Needs - description: Support from other contributors you are looking for to create a PR for this Issue. From f069bc27dcd6ae77ee6df2a5fb5133b2af9279df Mon Sep 17 00:00:00 2001 From: James Zaki Date: Tue, 30 Jul 2024 11:09:52 +0100 Subject: [PATCH 7/7] chore(docs): add Writing Noir doc (#5456) # Description ## Problem\* Resolves https://github.com/AztecProtocol/dev-rel/issues/300 ## Summary\* Documents Noir considerations vs regular procedural programming. Some considerations specifically for Rust syntax conversion. ## Additional Context ## Documentation\* Check one: - [ ] No documentation needed. - [x] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [ ] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: josh crites Co-authored-by: Savio <72797635+Savio-Sou@users.noreply.github.com> --- cspell.json | 2 + docs/docs/explainers/cspell.json | 5 + .../docs/explainers/explainer-writing-noir.md | 173 ++++++++++++++++++ .../tooling => reference}/noir_codegen.md | 2 +- 4 files changed, 181 insertions(+), 1 deletion(-) create mode 100644 docs/docs/explainers/cspell.json create mode 100644 docs/docs/explainers/explainer-writing-noir.md rename docs/docs/{getting_started/tooling => reference}/noir_codegen.md (97%) diff --git a/cspell.json b/cspell.json index 689b72435ef..b9199bea4bd 100644 --- a/cspell.json +++ b/cspell.json @@ -126,6 +126,7 @@ "memset", "merkle", "metas", + "microcontroller", "minreq", "monomorphization", "monomorphize", @@ -135,6 +136,7 @@ "monomorphizing", "montcurve", "MSRV", + "multicall", "nand", "nargo", "neovim", diff --git a/docs/docs/explainers/cspell.json b/docs/docs/explainers/cspell.json new file mode 100644 index 00000000000..c60b0a597b1 --- /dev/null +++ b/docs/docs/explainers/cspell.json @@ -0,0 +1,5 @@ +{ + "words": [ + "Cryptdoku" + ] +} diff --git a/docs/docs/explainers/explainer-writing-noir.md b/docs/docs/explainers/explainer-writing-noir.md new file mode 100644 index 00000000000..c8a42c379e6 --- /dev/null +++ b/docs/docs/explainers/explainer-writing-noir.md @@ -0,0 +1,173 @@ +--- +title: Writing Performant Noir +description: Understand new considerations when writing Noir +keywords: [Noir, programming, rust] +tags: [Optimization] +sidebar_position: 0 +--- + + +This article intends to set you up with key concepts essential for writing more viable applications that use zero knowledge proofs, namely around efficient circuits. + +## Context - 'Efficient' is subjective + +When writing a web application for a performant computer with high-speed internet connection, writing efficient code sometimes is seen as an afterthought only if needed. Large multiplications running at the innermost of nested loops may not even be on a dev's radar. +When writing firmware for a battery-powered microcontroller, you think of cpu cycles as rations to keep within a product's power budget. + +> Code is written to create applications that perform specific tasks within specific constraints + +And these constraints differ depending on where the compiled code is execute. + +### The Ethereum Virtual Machine (EVM) + +In scenarios where extremely low gas costs are required for an Ethereum application to be viable/competitive, Ethereum smart contract developers get into what is colloquially known as: "*gas golfing*". Finding the lowest execution cost of their compiled code (EVM bytecode) to achieve a specific task. + +The equivalent optimization task when writing zk circuits is affectionately referred to as "*gate golfing*", finding the lowest gate representation of the compiled Noir code. + +### Coding for circuits - a paradigm shift + +In zero knowledge cryptography, code is compiled to "circuits" consisting of arithmetic gates, and gate count is the significant cost. Depending on the proving system this is linearly proportionate to proving time, and so from a product point this should be kept as low as possible. + +Whilst writing efficient code for web apps and Solidity has a few key differences, writing efficient circuits have a different set of considerations. It is a bit of a paradigm shift, like writing code for GPUs for the first time... + +For example, drawing a circle at (0, 0) of radius `r`: +- For a single CPU thread, +``` +for theta in 0..2*pi { + let x = r * cos(theta); + let y = r * sin(theta); + draw(x, y); +} // note: would do 0 - pi/2 and draw +ve/-ve x and y. +``` + +- For GPUs (simultaneous parallel calls with x, y across image), +``` +if (x^2 + y^2 = r^2) { + draw(x, y); +} +``` + +([Related](https://www.youtube.com/watch?v=-P28LKWTzrI)) + +Whilst this CPU -> GPU does not translate to circuits exactly, it is intended to exemplify the difference in intuition when coding for different machine capabilities/constraints. + +### Context Takeaway + +For those coming from a primarily web app background, this article will explain what you need to consider when writing circuits. Furthermore, for those experienced writing efficient machine code, prepare to shift what you think is efficient 😬 + +## Translating from Rust + +For some applications using Noir, existing code might be a convenient starting point to then proceed to optimize the gate count of. + +:::note +Many valuable functions and algorithms have been written in more established languages (C/C++), and converted to modern ones (like Rust). +::: + +Fortunately for Noir developers, when needing a particular function a Rust implementation can be readily compiled into Noir with some key changes. While the compiler does a decent amount of optimizations, it won't be able to change code that has been optimized for clock-cycles into code optimized for arithmetic gates. + +A few things to do when converting Rust code to Noir: +- `println!` is not a macro, use `println` function (same for `assert_eq`) +- No early `return` in function. Use constrain via assertion instead +- No passing by reference. Remove `&` operator to pass by value (copy) +- No boolean operators (`&&`, `||`). Use bitwise operators (`&`, `|`) with boolean values +- No type `usize`. Use types `u8`, `u32`, `u64`, ... +- `main` return must be public, `pub` +- No `const`, use `global` +- Noir's LSP is your friend, so error message should be informative enough to resolve syntax issues. + +## Writing efficient Noir for performant products + +The following points help refine our understanding over time. + +:::note +A Noir program makes a statement that can be verified. +::: + +It compiles to a structure that represents the calculation, and can assert results within the calculation at any stage (via the `constrain` keyword). + +A Noir program compiles to an Abstract Circuit Intermediate Representation which is: + - A tree structure + - Leaves (inputs) are the `Field` type + - Nodes contain arithmetic operations to combine them (gates) + - The root is the final result (return value) + +:::tip +The command `nargo info` shows the programs circuit size, and is useful to compare the value of changes made. +You can dig deeper and use the `--print-acir` param to take a closer look at individual ACIR opcodes, and the proving backend to see its gate count (eg for barretenberg, `bb gates -b ./target/program.json`). +::: + +### Use the `Field` type + +Since the native type of values in circuits are `Field`s, using them for variables in Noir means less gates converting them under the hood. + +:::tip +Where possible, use `Field` type for values. Using smaller value types, and bit-packing strategies, will result in MORE gates +::: + +**Note:** Need to remain mindful of overflow. Types with less bits may be used to limit the range of possible values prior to a calculation. + +### Use Arithmetic over non-arithmetic operations + +Since circuits are made of arithmetic gates, the cost of arithmetic operations tends to be one gate. Whereas for procedural code, they represent several clock cycles. + +Inversely, non-arithmetic operators are achieved with multiple gates, vs 1 clock cycle for procedural code. + +| (cost\op) | arithmetic
(`*`, `+`) | bit-wise ops
(eg `<`, `\|`, `>>`) | +| - | - | - | +| **cycles** | 10+ | 1 | +| **gates** | 1 | 10+ | + +Bit-wise operations (e.g. bit shifts `<<` and `>>`), albeit commonly used in general programming and especially for clock cycle optimizations, are on the contrary expensive in gates when performed within circuits. + +Translate away from bit shifts when writing constrained functions for the best performance. + +On the flip side, feel free to use bit shifts in unconstrained functions and tests if necessary, as they are executed outside of circuits and does not induce performance hits. + +### Use static over dynamic values + +Another general theme that manifests in different ways is that static reads are represented with less gates than dynamic ones. + +Reading from read-only memory (ROM) adds less gates than random-access memory (RAM), 2 vs ~3.25 due to the additional bounds checks. Arrays of fixed length (albeit used at a lower capacity), will generate less gates than dynamic storage. + +Related to this, if an index used to access an array is not known at compile time (ie unknown until run time), then ROM will be converted to RAM, expanding the gate count. + +:::tip +Use arrays and indices that are known at compile time where possible. +Using `assert_constant(i);` before an index, `i`, is used in an array will give a compile error if `i` is NOT known at compile time. +::: + +### Leverage unconstrained execution + +Constrained verification can leverage unconstrained execution, this is especially useful for operations that are represented by many gates. +Use an [unconstrained function](../noir/concepts/unconstrained.md) to perform gate-heavy calculations, then verify and constrain the result. + +Eg division generates more gates than multiplication, so calculating the quotient in an unconstrained function then constraining the product for the quotient and divisor (+ any remainder) equals the dividend will be more efficient. + +Use ` if is_unconstrained() { /`, to conditionally execute code if being called in an unconstrained vs constrained way. + +## Advanced + +Unless you're well into the depth of gate optimization, this advanced section can be ignored. + +### Combine arithmetic operations + +A Noir program can be honed further by combining arithmetic operators in a way that makes the most of each constraint of the backend proving system. This is in scenarios where the backend might not be doing this perfectly. + +Eg Barretenberg backend (current default for Noir) is a width-4 PLONKish constraint system +$ w_1*w_2*q_m + w_1*q_1 + w_2*q_2 + w_3*q_3 + w_4*q_4 + q_c = 0 $ + +Here we see there is one occurrence of witness 1 and 2 ($w_1$, $w_2$) being multiplied together, with addition to witnesses 1-4 ($w_1$ .. $w_4$) multiplied by 4 corresponding circuit constants ($q_1$ .. $q_4$) (plus a final circuit constant, $q_c$). + +Use `nargo info --print-acir`, to inspect the ACIR opcodes (and the proving system for gates), and it may present opportunities to amend the order of operations and reduce the number of constraints. + +#### Variable as witness vs expression + +If you've come this far and really know what you're doing at the equation level, a temporary lever (that will become unnecessary/useless over time) is: `std::as_witness`. This informs the compiler to save a variable as a witness not an expression. + +The compiler will mostly be correct and optimal, but this may help some near term edge cases that are yet to optimize. +Note: When used incorrectly it will create **less** efficient circuits (higher gate count). + +## References +- Guillaume's ["`Cryptdoku`" talk](https://www.youtube.com/watch?v=MrQyzuogxgg) (Jun'23) +- Tips from Tom, Jake and Zac. +- [Idiomatic Noir](https://www.vlayer.xyz/blog/idiomatic-noir-part-1-collections) blog post diff --git a/docs/docs/getting_started/tooling/noir_codegen.md b/docs/docs/reference/noir_codegen.md similarity index 97% rename from docs/docs/getting_started/tooling/noir_codegen.md rename to docs/docs/reference/noir_codegen.md index f7505bef7ab..db8f07dc22e 100644 --- a/docs/docs/getting_started/tooling/noir_codegen.md +++ b/docs/docs/reference/noir_codegen.md @@ -33,7 +33,7 @@ yarn add @noir-lang/noir_codegen -D ``` ### Nargo library -Make sure you have Nargo, v0.25.0 or greater, installed. If you don't, follow the [installation guide](../installation/index.md). +Make sure you have Nargo, v0.25.0 or greater, installed. If you don't, follow the [installation guide](../getting_started/installation/index.md). If you're in a new project, make a `circuits` folder and create a new Noir library: