diff --git a/.noir-sync-commit b/.noir-sync-commit index b143949000d..44f96ad3927 100644 --- a/.noir-sync-commit +++ b/.noir-sync-commit @@ -1 +1 @@ -c6e5c4b304eb4e87ca519151f610e7e03b1a4fba +2b4853e71859f225acc123160e87c522212b16b5 diff --git a/noir/noir-repo/.github/workflows/test-js-packages.yml b/noir/noir-repo/.github/workflows/test-js-packages.yml index 06436af8d27..5a09a0cf444 100644 --- a/noir/noir-repo/.github/workflows/test-js-packages.yml +++ b/noir/noir-repo/.github/workflows/test-js-packages.yml @@ -517,8 +517,8 @@ jobs: fail-fast: false matrix: project: - - { repo: AztecProtocol/aztec-nr, path: ./ } - - { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-contracts } + # - { repo: AztecProtocol/aztec-nr, path: ./ } + # - { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-contracts } # Disabled as aztec-packages requires a setup-step in order to generate a `Nargo.toml` #- { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-protocol-circuits } - { repo: zac-williamson/noir-edwards, path: ./, ref: 0016ce82cd58b6ebb0c43c271725590bcff4e755 } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs index 9d29d1d24d6..44467677154 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs @@ -294,24 +294,7 @@ impl GeneratedAcir { outputs: (outputs[0], outputs[1], outputs[2]), }, BlackBoxFunc::Keccak256 => { - let var_message_size = match inputs.to_vec().pop() { - Some(var_message_size) => var_message_size[0], - None => { - return Err(InternalError::MissingArg { - name: "".to_string(), - arg: "message_size".to_string(), - call_stack: self.call_stack.clone(), - }); - } - }; - - BlackBoxFuncCall::Keccak256 { - inputs: inputs[0].clone(), - var_message_size, - outputs: outputs - .try_into() - .expect("Compiler should generate correct size outputs"), - } + unreachable!("unexpected BlackBox {}", func_name.to_string()) } BlackBoxFunc::Keccakf1600 => BlackBoxFuncCall::Keccakf1600 { inputs: inputs[0] diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs index b284fb6c00d..9baeb19206f 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs @@ -1,9 +1,11 @@ use iter_extended::vecmap; +use crate::ssa::ir::types::Type; + use super::{ basic_block::BasicBlockId, dfg::{CallStack, InsertInstructionResult}, - function::Function, + function::{Function, RuntimeType}, instruction::{Instruction, InstructionId}, value::ValueId, }; @@ -20,7 +22,7 @@ pub(crate) struct FunctionInserter<'f> { /// array unnecessarily. An extra bool is included as part of the key to /// distinguish between Type::Array and Type::Slice, as both are valid /// types for a Value::Array - const_arrays: HashMap, ValueId>, + const_arrays: HashMap<(im::Vector, bool), ValueId>, } impl<'f> FunctionInserter<'f> { @@ -42,15 +44,26 @@ impl<'f> FunctionInserter<'f> { let new_array: im::Vector = array.iter().map(|id| self.resolve(*id)).collect(); - if self.const_arrays.get(&new_array) == Some(&value) { - value - } else { - let new_array_clone = new_array.clone(); - let new_id = self.function.dfg.make_array(new_array, typ); - self.values.insert(value, new_id); - self.const_arrays.insert(new_array_clone, new_id); - new_id - } + // Flag to determine the type of the value's array list + let is_array = matches!(typ, Type::Array { .. }); + if let Some(fetched_value) = + self.const_arrays.get(&(new_array.clone(), is_array)) + { + // Arrays in ACIR are immutable, but in Brillig arrays are copy-on-write + // so for function's with a Brillig runtime we make sure to check that value + // in our constants array map matches the resolved array value id. + if matches!(self.function.runtime(), RuntimeType::Acir(_)) { + return *fetched_value; + } else if *fetched_value == value { + return value; + } + }; + + let new_array_clone = new_array.clone(); + let new_id = self.function.dfg.make_array(new_array, typ); + self.values.insert(value, new_id); + self.const_arrays.insert((new_array_clone, is_array), new_id); + new_id } _ => value, }, diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs index 3b52e5add4e..ad01edbd0b2 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs @@ -489,7 +489,9 @@ fn simplify_black_box_func( SimplifyResult::None } } - BlackBoxFunc::Keccak256 => SimplifyResult::None, + BlackBoxFunc::Keccak256 => { + unreachable!("Keccak256 should have been replaced by calls to Keccakf1600") + } BlackBoxFunc::Poseidon2Permutation => SimplifyResult::None, //TODO(Guillaume) BlackBoxFunc::EcdsaSecp256k1 => { simplify_signature(dfg, arguments, acvm::blackbox_solver::ecdsa_secp256k1_verify) diff --git a/noir/noir-repo/compiler/noirc_frontend/src/ast/mod.rs b/noir/noir-repo/compiler/noirc_frontend/src/ast/mod.rs index dfe4258744a..8cdf377528b 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/ast/mod.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/ast/mod.rs @@ -323,13 +323,11 @@ impl UnresolvedTypeExpression { fn from_expr_helper(expr: Expression) -> Result { match expr.kind { - ExpressionKind::Literal(Literal::Integer(int, sign)) => { - assert!(!sign, "Negative literal is not allowed here"); - match int.try_to_u32() { - Some(int) => Ok(UnresolvedTypeExpression::Constant(int, expr.span)), - None => Err(expr), - } - } + // TODO(https://github.com/noir-lang/noir/issues/5571): The `sign` bool from `Literal::Integer` should be used to construct the constant type expression. + ExpressionKind::Literal(Literal::Integer(int, _)) => match int.try_to_u32() { + Some(int) => Ok(UnresolvedTypeExpression::Constant(int, expr.span)), + None => Err(expr), + }, ExpressionKind::Variable(path, _) => Ok(UnresolvedTypeExpression::Variable(path)), ExpressionKind::Prefix(prefix) if prefix.operator == UnaryOp::Minus => { let lhs = Box::new(UnresolvedTypeExpression::Constant(0, expr.span)); diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter.rs index cbea5cd158e..1f5903df4ad 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter.rs @@ -613,7 +613,12 @@ impl<'a> Interpreter<'a> { UnaryOp::MutableReference => self.evaluate_no_dereference(prefix.rhs)?, _ => self.evaluate(prefix.rhs)?, }; - self.evaluate_prefix_with_value(rhs, prefix.operator, id) + + if self.interner.get_selected_impl_for_expression(id).is_some() { + self.evaluate_overloaded_prefix(prefix, rhs, id) + } else { + self.evaluate_prefix_with_value(rhs, prefix.operator, id) + } } fn evaluate_prefix_with_value( @@ -953,6 +958,25 @@ impl<'a> Interpreter<'a> { } } + fn evaluate_overloaded_prefix( + &mut self, + prefix: HirPrefixExpression, + rhs: Value, + id: ExprId, + ) -> IResult { + let method = + prefix.trait_method_id.expect("ice: expected prefix operator trait at this point"); + let operator = prefix.operator; + + let method_id = resolve_trait_method(self.interner, method, id)?; + let type_bindings = self.interner.get_instantiation_bindings(id).clone(); + + let rhs = (rhs, self.interner.expr_location(&prefix.rhs)); + + let location = self.interner.expr_location(&id); + self.call_function(method_id, vec![rhs], type_bindings, location) + } + /// Given the result of a `cmp` operation, convert it into the boolean result of the given operator. /// - `<`: `ordering == Ordering::Less` /// - `<=`: `ordering != Ordering::Greater` diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs index 017ea360562..771f23335fd 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs @@ -5,12 +5,14 @@ use std::{ use acvm::{AcirField, FieldElement}; use chumsky::Parser; +use iter_extended::vecmap; use noirc_errors::Location; use crate::{ ast::{IntegerBitSize, TraitBound}, hir::comptime::{errors::IResult, InterpreterError, Value}, - macros_api::{NodeInterner, Signedness}, + macros_api::{NodeInterner, Path, Signedness, UnresolvedTypeData}, + node_interner::TraitId, parser, token::{SpannedToken, Token, Tokens}, QuotedType, Type, @@ -42,6 +44,9 @@ pub(super) fn call_builtin( "struct_def_generics" => struct_def_generics(interner, arguments, location), "trait_constraint_eq" => trait_constraint_eq(interner, arguments, location), "trait_constraint_hash" => trait_constraint_hash(interner, arguments, location), + "trait_def_as_trait_constraint" => { + trait_def_as_trait_constraint(interner, arguments, location) + } "quoted_as_trait_constraint" => quoted_as_trait_constraint(interner, arguments, location), _ => { let item = format!("Comptime evaluation for builtin function {name}"); @@ -103,6 +108,16 @@ fn get_trait_constraint(value: Value, location: Location) -> IResult } } +fn get_trait_def(value: Value, location: Location) -> IResult { + match value { + Value::TraitDefinition(id) => Ok(id), + value => { + let expected = Type::Quoted(QuotedType::TraitDefinition); + Err(InterpreterError::TypeMismatch { expected, value, location }) + } + } +} + fn get_quoted(value: Value, location: Location) -> IResult> { match value { Value::Code(tokens) => Ok(tokens), @@ -456,3 +471,24 @@ fn modulus_num_bits( let bits = FieldElement::max_num_bits().into(); Ok(Value::U64(bits)) } + +fn trait_def_as_trait_constraint( + interner: &mut NodeInterner, + mut arguments: Vec<(Value, Location)>, + location: Location, +) -> Result { + check_argument_count(1, &arguments, location)?; + + 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) + }); + + let trait_id = Some(trait_id); + Ok(Value::TraitConstraint(TraitBound { trait_path, trait_id, trait_generics })) +} diff --git a/noir/noir-repo/compiler/noirc_frontend/src/parser/errors.rs b/noir/noir-repo/compiler/noirc_frontend/src/parser/errors.rs index 41ea9f88c19..9df9a8e15dd 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/parser/errors.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/parser/errors.rs @@ -44,6 +44,10 @@ pub enum ParserErrorReason { InvalidBitSize(u32), #[error("{0}")] Lexer(LexerErrorKind), + // TODO(https://github.com/noir-lang/noir/issues/5571): This error can be removed once support is expanded for type-level integers. + // This error reason was added to prevent the panic outline in this issue: https://github.com/noir-lang/noir/issues/5552. + #[error("Only unsigned integers allowed for numeric generics")] + SignedNumericGeneric, } /// Represents a parsing error, or a parsing error in the making. diff --git a/noir/noir-repo/compiler/noirc_frontend/src/parser/parser/function.rs b/noir/noir-repo/compiler/noirc_frontend/src/parser/parser/function.rs index 3e686ee4c85..fd1c7d5237f 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/parser/parser/function.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/parser/parser/function.rs @@ -4,13 +4,17 @@ use super::{ parameter_name_recovery, parameter_recovery, parenthesized, parse_type, pattern, self_parameter, where_clause, NoirParser, }; -use crate::ast::{ - FunctionDefinition, FunctionReturnType, ItemVisibility, NoirFunction, Param, Visibility, -}; use crate::parser::spanned; use crate::token::{Keyword, Token}; use crate::{ - ast::{UnresolvedGeneric, UnresolvedGenerics}, + ast::{ + FunctionDefinition, FunctionReturnType, ItemVisibility, NoirFunction, Param, Visibility, + }, + macros_api::UnresolvedTypeData, + parser::{ParserError, ParserErrorReason}, +}; +use crate::{ + ast::{Signedness, UnresolvedGeneric, UnresolvedGenerics}, parser::labels::ParsingRuleLabel, }; @@ -85,6 +89,19 @@ pub(super) fn numeric_generic() -> impl NoirParser { .then_ignore(just(Token::Colon)) .then(parse_type()) .map(|(ident, typ)| UnresolvedGeneric::Numeric { ident, typ }) + .validate(|generic, span, emit| { + if let UnresolvedGeneric::Numeric { typ, .. } = &generic { + if let UnresolvedTypeData::Integer(signedness, _) = typ.typ { + if matches!(signedness, Signedness::Signed) { + emit(ParserError::with_reason( + ParserErrorReason::SignedNumericGeneric, + span, + )); + } + } + } + generic + }) } pub(super) fn generic_type() -> impl NoirParser { @@ -233,6 +250,7 @@ mod test { "fn func_name(y: T) {}", "fn func_name(y: T) {}", "fn func_name(y: T) {}", + "fn func_name() {}", ], ); } diff --git a/noir/noir-repo/noir_stdlib/src/embedded_curve_ops.nr b/noir/noir-repo/noir_stdlib/src/embedded_curve_ops.nr index 6b70b6ddef0..9324802c2f8 100644 --- a/noir/noir-repo/noir_stdlib/src/embedded_curve_ops.nr +++ b/noir/noir-repo/noir_stdlib/src/embedded_curve_ops.nr @@ -105,6 +105,9 @@ fn multi_scalar_mul_array_return(points: [EmbeddedCurvePoint; N], sc #[foreign(multi_scalar_mul)] pub(crate) fn multi_scalar_mul_slice(points: [EmbeddedCurvePoint], scalars: [EmbeddedCurveScalar]) -> [Field; 3] {} +#[foreign(multi_scalar_mul)] +pub(crate) fn multi_scalar_mul_slice(points: [EmbeddedCurvePoint], scalars: [EmbeddedCurveScalar]) -> [Field; 3] {} + // docs:start:fixed_base_scalar_mul pub fn fixed_base_scalar_mul(scalar: EmbeddedCurveScalar) -> EmbeddedCurvePoint // docs:end:fixed_base_scalar_mul diff --git a/noir/noir-repo/noir_stdlib/src/hash/mod.nr b/noir/noir-repo/noir_stdlib/src/hash/mod.nr index b47cbf5acc3..69470b56ab2 100644 --- a/noir/noir-repo/noir_stdlib/src/hash/mod.nr +++ b/noir/noir-repo/noir_stdlib/src/hash/mod.nr @@ -106,11 +106,12 @@ pub fn hash_to_field(inputs: [Field]) -> Field { sum } -#[foreign(keccak256)] // docs:start:keccak256 pub fn keccak256(input: [u8; N], message_size: u32) -> [u8; 32] // docs:end:keccak256 -{} +{ + crate::hash::keccak::keccak256(input, message_size) +} #[foreign(poseidon2_permutation)] pub fn poseidon2_permutation(_input: [Field; N], _state_length: u32) -> [Field; N] {} diff --git a/noir/noir-repo/noir_stdlib/src/meta/mod.nr b/noir/noir-repo/noir_stdlib/src/meta/mod.nr index ed3365d755c..395f09a453e 100644 --- a/noir/noir-repo/noir_stdlib/src/meta/mod.nr +++ b/noir/noir-repo/noir_stdlib/src/meta/mod.nr @@ -1,5 +1,6 @@ -mod type_def; mod trait_constraint; +mod trait_def; +mod type_def; mod quoted; /// Calling unquote as a macro (via `unquote!(arg)`) will unquote diff --git a/noir/noir-repo/noir_stdlib/src/meta/trait_def.nr b/noir/noir-repo/noir_stdlib/src/meta/trait_def.nr new file mode 100644 index 00000000000..5de7631e34d --- /dev/null +++ b/noir/noir-repo/noir_stdlib/src/meta/trait_def.nr @@ -0,0 +1,4 @@ +impl TraitDefinition { + #[builtin(trait_def_as_trait_constraint)] + fn as_trait_constraint(_self: Self) -> TraitConstraint {} +} diff --git a/noir/noir-repo/test_programs/compile_success_empty/comptime_traits/src/main.nr b/noir/noir-repo/test_programs/compile_success_empty/comptime_traits/src/main.nr index 143c9cda274..8b1f81e6594 100644 --- a/noir/noir-repo/test_programs/compile_success_empty/comptime_traits/src/main.nr +++ b/noir/noir-repo/test_programs/compile_success_empty/comptime_traits/src/main.nr @@ -1,3 +1,5 @@ +use std::ops::Neg; + fn main() { comptime { @@ -13,3 +15,22 @@ fn main() { assert([1, 2] != array); } } + +struct MyType { + value: i32, +} + +comptime impl Neg for MyType { + comptime fn neg(self) -> Self { + self + } +} + +fn neg_at_comptime() { + comptime + { + let value = MyType { value: 1 }; + let _result = -value; + } +} + diff --git a/noir/noir-repo/test_programs/compile_success_empty/trait_as_constraint/Nargo.toml b/noir/noir-repo/test_programs/compile_success_empty/trait_as_constraint/Nargo.toml new file mode 100644 index 00000000000..907b5ce09ed --- /dev/null +++ b/noir/noir-repo/test_programs/compile_success_empty/trait_as_constraint/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "trait_as_constraint" +type = "bin" +authors = [""] +compiler_version = ">=0.31.0" + +[dependencies] \ No newline at end of file diff --git a/noir/noir-repo/test_programs/compile_success_empty/trait_as_constraint/src/main.nr b/noir/noir-repo/test_programs/compile_success_empty/trait_as_constraint/src/main.nr new file mode 100644 index 00000000000..1911f045c27 --- /dev/null +++ b/noir/noir-repo/test_programs/compile_success_empty/trait_as_constraint/src/main.nr @@ -0,0 +1,9 @@ +#[test_as_constraint] +trait Foo {} + +comptime fn test_as_constraint(t: TraitDefinition) { + let constraint = t.as_trait_constraint(); + assert(constraint == constraint); +} + +fn main() {} diff --git a/noir/noir-repo/test_programs/execution_success/trait_method_mut_self/Nargo.toml b/noir/noir-repo/test_programs/compile_success_empty/trait_method_mut_self/Nargo.toml similarity index 100% rename from noir/noir-repo/test_programs/execution_success/trait_method_mut_self/Nargo.toml rename to noir/noir-repo/test_programs/compile_success_empty/trait_method_mut_self/Nargo.toml diff --git a/noir/noir-repo/test_programs/execution_success/trait_method_mut_self/Prover.toml b/noir/noir-repo/test_programs/compile_success_empty/trait_method_mut_self/Prover.toml similarity index 100% rename from noir/noir-repo/test_programs/execution_success/trait_method_mut_self/Prover.toml rename to noir/noir-repo/test_programs/compile_success_empty/trait_method_mut_self/Prover.toml diff --git a/noir/noir-repo/test_programs/execution_success/trait_method_mut_self/src/main.nr b/noir/noir-repo/test_programs/compile_success_empty/trait_method_mut_self/src/main.nr similarity index 100% rename from noir/noir-repo/test_programs/execution_success/trait_method_mut_self/src/main.nr rename to noir/noir-repo/test_programs/compile_success_empty/trait_method_mut_self/src/main.nr diff --git a/noir/noir-repo/test_programs/execution_success/turbofish_call_func_diff_types/Nargo.toml b/noir/noir-repo/test_programs/compile_success_empty/turbofish_call_func_diff_types/Nargo.toml similarity index 100% rename from noir/noir-repo/test_programs/execution_success/turbofish_call_func_diff_types/Nargo.toml rename to noir/noir-repo/test_programs/compile_success_empty/turbofish_call_func_diff_types/Nargo.toml diff --git a/noir/noir-repo/test_programs/execution_success/turbofish_call_func_diff_types/Prover.toml b/noir/noir-repo/test_programs/compile_success_empty/turbofish_call_func_diff_types/Prover.toml similarity index 100% rename from noir/noir-repo/test_programs/execution_success/turbofish_call_func_diff_types/Prover.toml rename to noir/noir-repo/test_programs/compile_success_empty/turbofish_call_func_diff_types/Prover.toml diff --git a/noir/noir-repo/test_programs/execution_success/turbofish_call_func_diff_types/src/main.nr b/noir/noir-repo/test_programs/compile_success_empty/turbofish_call_func_diff_types/src/main.nr similarity index 100% rename from noir/noir-repo/test_programs/execution_success/turbofish_call_func_diff_types/src/main.nr rename to noir/noir-repo/test_programs/compile_success_empty/turbofish_call_func_diff_types/src/main.nr diff --git a/noir/noir-repo/test_programs/execution_success/uhashmap/Nargo.toml b/noir/noir-repo/test_programs/execution_success/uhashmap/Nargo.toml index c09debc9833..0d898e53003 100644 --- a/noir/noir-repo/test_programs/execution_success/uhashmap/Nargo.toml +++ b/noir/noir-repo/test_programs/execution_success/uhashmap/Nargo.toml @@ -1,6 +1,6 @@ [package] -name = "hashmap" +name = "uhashmap" type = "bin" authors = [""] -[dependencies] \ No newline at end of file +[dependencies] diff --git a/noir/noir-repo/tooling/lsp/src/lib.rs b/noir/noir-repo/tooling/lsp/src/lib.rs index 80c4573138c..ea787f05e98 100644 --- a/noir/noir-repo/tooling/lsp/src/lib.rs +++ b/noir/noir-repo/tooling/lsp/src/lib.rs @@ -52,6 +52,7 @@ use requests::{ on_goto_definition_request, on_goto_type_definition_request, on_hover_request, on_initialize, on_inlay_hint_request, on_prepare_rename_request, on_profile_run_request, on_references_request, on_rename_request, on_shutdown, on_test_run_request, on_tests_request, + LspInitializationOptions, }; use serde_json::Value as JsonValue; use thiserror::Error; @@ -85,7 +86,7 @@ pub struct LspState { cached_lenses: HashMap>, cached_definitions: HashMap, cached_parsed_files: HashMap))>, - parsing_cache_enabled: bool, + options: LspInitializationOptions, } impl LspState { @@ -102,7 +103,7 @@ impl LspState { cached_definitions: HashMap::new(), open_documents_count: 0, cached_parsed_files: HashMap::new(), - parsing_cache_enabled: true, + options: Default::default(), } } } @@ -341,7 +342,7 @@ fn prepare_source(source: String, state: &mut LspState) -> (Context<'static, 'st } fn parse_diff(file_manager: &FileManager, state: &mut LspState) -> ParsedFiles { - if state.parsing_cache_enabled { + if state.options.enable_parsing_cache { let noir_file_hashes: Vec<_> = file_manager .as_file_map() .all_file_ids() diff --git a/noir/noir-repo/tooling/lsp/src/requests/inlay_hint.rs b/noir/noir-repo/tooling/lsp/src/requests/inlay_hint.rs index c47e59b0c2b..2afa5fa44fd 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/inlay_hint.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/inlay_hint.rs @@ -14,6 +14,7 @@ use noirc_frontend::{ BlockExpression, Expression, ExpressionKind, Ident, LetStatement, NoirFunction, Pattern, Statement, StatementKind, TraitImplItem, TraitItem, UnresolvedTypeData, }, + hir_def::stmt::HirPattern, macros_api::NodeInterner, node_interner::ReferenceId, parser::{Item, ItemKind}, @@ -22,7 +23,7 @@ use noirc_frontend::{ use crate::LspState; -use super::{process_request, to_lsp_location}; +use super::{process_request, to_lsp_location, InlayHintsOptions}; pub(crate) fn on_inlay_hint_request( state: &mut LspState, @@ -33,6 +34,8 @@ pub(crate) fn on_inlay_hint_request( position: Position { line: 0, character: 0 }, }; + let options = state.options.inlay_hints; + let result = process_request(state, text_document_position_params, |args| { let path = PathString::from_path(params.text_document.uri.to_file_path().unwrap()); args.files.get_file_id(&path).map(|file_id| { @@ -43,7 +46,8 @@ pub(crate) fn on_inlay_hint_request( let span = range_to_byte_span(args.files, file_id, ¶ms.range) .map(|range| Span::from(range.start as u32..range.end as u32)); - let mut collector = InlayHintCollector::new(args.files, file_id, args.interner, span); + let mut collector = + InlayHintCollector::new(args.files, file_id, args.interner, span, options); collector.collect_in_parsed_module(&parsed_moduled); collector.inlay_hints }) @@ -56,6 +60,7 @@ pub(crate) struct InlayHintCollector<'a> { file_id: FileId, interner: &'a NodeInterner, span: Option, + options: InlayHintsOptions, inlay_hints: Vec, } @@ -65,8 +70,9 @@ impl<'a> InlayHintCollector<'a> { file_id: FileId, interner: &'a NodeInterner, span: Option, + options: InlayHintsOptions, ) -> InlayHintCollector<'a> { - InlayHintCollector { files, file_id, interner, span, inlay_hints: Vec::new() } + InlayHintCollector { files, file_id, interner, span, options, inlay_hints: Vec::new() } } fn collect_in_parsed_module(&mut self, parsed_module: &ParsedModule) { for item in &parsed_module.items { @@ -195,12 +201,24 @@ impl<'a> InlayHintCollector<'a> { self.collect_in_expression(&index_expression.index); } ExpressionKind::Call(call_expression) => { + self.collect_call_parameter_names( + get_expression_name(&call_expression.func), + call_expression.func.span, + &call_expression.arguments, + ); + self.collect_in_expression(&call_expression.func); for arg in &call_expression.arguments { self.collect_in_expression(arg); } } ExpressionKind::MethodCall(method_call_expression) => { + self.collect_call_parameter_names( + Some(method_call_expression.method_name.to_string()), + method_call_expression.method_name.span(), + &method_call_expression.arguments, + ); + self.collect_in_expression(&method_call_expression.object); for arg in &method_call_expression.arguments { self.collect_in_expression(arg); @@ -252,6 +270,10 @@ impl<'a> InlayHintCollector<'a> { } fn collect_in_pattern(&mut self, pattern: &Pattern) { + if !self.options.type_hints.enabled { + return; + } + match pattern { Pattern::Identifier(ident) => { self.collect_in_ident(ident); @@ -273,6 +295,10 @@ impl<'a> InlayHintCollector<'a> { } fn collect_in_ident(&mut self, ident: &Ident) { + if !self.options.type_hints.enabled { + return; + } + let span = ident.span(); let location = Location::new(ident.span(), self.file_id); if let Some(lsp_location) = to_lsp_location(self.files, self.file_id, span) { @@ -324,6 +350,117 @@ impl<'a> InlayHintCollector<'a> { }); } + fn collect_call_parameter_names( + &mut self, + function_name: Option, + at: Span, + arguments: &[Expression], + ) { + if !self.options.parameter_hints.enabled { + return; + } + + // The `at` span might be the span of a path like `Foo::bar`. + // In order to find the function behind it, we use a span that is just the last char. + let at = Span::single_char(at.end() - 1); + + let referenced = self.interner.find_referenced(Location::new(at, self.file_id)); + if let Some(ReferenceId::Function(func_id)) = referenced { + let func_meta = self.interner.function_meta(&func_id); + + let mut parameters = func_meta.parameters.iter().peekable(); + let mut parameters_count = func_meta.parameters.len(); + + // Skip `self` parameter + if let Some((pattern, _, _)) = parameters.peek() { + if self.is_self_parameter(pattern) { + parameters.next(); + parameters_count -= 1; + } + } + + for (call_argument, (pattern, _, _)) in arguments.iter().zip(parameters) { + let Some(lsp_location) = + to_lsp_location(self.files, self.file_id, call_argument.span) + else { + continue; + }; + + let Some(parameter_name) = self.get_pattern_name(pattern) else { + continue; + }; + + if parameter_name.starts_with('_') { + continue; + } + + if parameters_count == 1 { + if parameter_name.len() == 1 + || parameter_name == "other" + || parameter_name == "value" + { + continue; + } + + if let Some(function_name) = &function_name { + if function_name.ends_with(¶meter_name) { + continue; + } + } + } + + if let Some(call_argument_name) = get_expression_name(call_argument) { + if parameter_name == call_argument_name + || call_argument_name.ends_with(¶meter_name) + { + continue; + } + } + + self.push_parameter_hint(lsp_location.range.start, ¶meter_name); + } + } + } + + fn get_pattern_name(&self, pattern: &HirPattern) -> Option { + match pattern { + HirPattern::Identifier(ident) => { + let definition = self.interner.definition(ident.id); + Some(definition.name.clone()) + } + HirPattern::Mutable(pattern, _location) => self.get_pattern_name(pattern), + HirPattern::Tuple(..) | HirPattern::Struct(..) => None, + } + } + + fn push_parameter_hint(&mut self, position: Position, str: &str) { + self.push_text_hint(position, format!("{}: ", str)); + } + + fn push_text_hint(&mut self, position: Position, str: String) { + self.inlay_hints.push(InlayHint { + position, + label: InlayHintLabel::String(str), + kind: Some(InlayHintKind::PARAMETER), + text_edits: None, + tooltip: None, + padding_left: None, + padding_right: None, + data: None, + }); + } + + fn is_self_parameter(&self, pattern: &HirPattern) -> bool { + match pattern { + HirPattern::Identifier(ident) => { + let definition_info = self.interner.definition(ident.id); + definition_info.name == "self" + } + HirPattern::Mutable(pattern, _location) => self.is_self_parameter(pattern), + HirPattern::Tuple(..) | HirPattern::Struct(..) => false, + } + } + fn intersects_span(&self, other_span: Span) -> bool { self.span.map_or(true, |span| span.intersects(&other_span)) } @@ -470,16 +607,106 @@ fn push_type_variable_parts( } } +fn get_expression_name(expression: &Expression) -> Option { + match &expression.kind { + ExpressionKind::Variable(path, _) => Some(path.last_segment().to_string()), + ExpressionKind::Prefix(prefix) => get_expression_name(&prefix.rhs), + ExpressionKind::MemberAccess(member_access) => Some(member_access.rhs.to_string()), + ExpressionKind::Call(call) => get_expression_name(&call.func), + ExpressionKind::MethodCall(method_call) => Some(method_call.method_name.to_string()), + ExpressionKind::Cast(cast) => get_expression_name(&cast.lhs), + ExpressionKind::Parenthesized(expr) => get_expression_name(expr), + ExpressionKind::Constructor(..) + | ExpressionKind::Infix(..) + | ExpressionKind::Index(..) + | ExpressionKind::Block(..) + | ExpressionKind::If(..) + | ExpressionKind::Lambda(..) + | ExpressionKind::Tuple(..) + | ExpressionKind::Quote(..) + | ExpressionKind::Unquote(..) + | ExpressionKind::Comptime(..) + | ExpressionKind::Resolved(..) + | ExpressionKind::Literal(..) + | ExpressionKind::Error => None, + } +} + +// These functions are copied from the codespan_lsp crate, except that they never panic +// (the library will sometimes panic, so functions returning Result are not always accurate) + +fn range_to_byte_span( + files: &FileMap, + file_id: FileId, + range: &lsp_types::Range, +) -> Option> { + Some( + position_to_byte_index(files, file_id, &range.start)? + ..position_to_byte_index(files, file_id, &range.end)?, + ) +} + +fn position_to_byte_index( + files: &FileMap, + file_id: FileId, + position: &lsp_types::Position, +) -> Option { + let Ok(source) = files.source(file_id) else { + return None; + }; + + let Ok(line_span) = files.line_range(file_id, position.line as usize) else { + return None; + }; + let line_str = source.get(line_span.clone())?; + + let byte_offset = character_to_line_offset(line_str, position.character)?; + + Some(line_span.start + byte_offset) +} + +fn character_to_line_offset(line: &str, character: u32) -> Option { + let line_len = line.len(); + let mut character_offset = 0; + + let mut chars = line.chars(); + while let Some(ch) = chars.next() { + if character_offset == character { + let chars_off = chars.as_str().len(); + let ch_off = ch.len_utf8(); + + return Some(line_len - chars_off - ch_off); + } + + character_offset += ch.len_utf16() as u32; + } + + // Handle positions after the last character on the line + if character_offset == character { + Some(line_len) + } else { + None + } +} + #[cfg(test)] mod inlay_hints_tests { - use crate::test_utils; + use crate::{ + requests::{ParameterHintsOptions, TypeHintsOptions}, + test_utils, + }; use super::*; use lsp_types::{Range, TextDocumentIdentifier, WorkDoneProgressParams}; use tokio::test; - async fn get_inlay_hints(start_line: u32, end_line: u32) -> Vec { + async fn get_inlay_hints( + start_line: u32, + end_line: u32, + options: InlayHintsOptions, + ) -> Vec { let (mut state, noir_text_document) = test_utils::init_lsp_server("inlay_hints").await; + state.options.inlay_hints = options; on_inlay_hint_request( &mut state, @@ -497,9 +724,36 @@ mod inlay_hints_tests { .unwrap() } + fn no_hints() -> InlayHintsOptions { + InlayHintsOptions { + type_hints: TypeHintsOptions { enabled: false }, + parameter_hints: ParameterHintsOptions { enabled: false }, + } + } + + fn type_hints() -> InlayHintsOptions { + InlayHintsOptions { + type_hints: TypeHintsOptions { enabled: true }, + parameter_hints: ParameterHintsOptions { enabled: false }, + } + } + + fn parameter_hints() -> InlayHintsOptions { + InlayHintsOptions { + type_hints: TypeHintsOptions { enabled: false }, + parameter_hints: ParameterHintsOptions { enabled: true }, + } + } + + #[test] + async fn test_do_not_collect_type_hints_if_disabled() { + let inlay_hints = get_inlay_hints(0, 3, no_hints()).await; + assert!(inlay_hints.is_empty()); + } + #[test] async fn test_type_inlay_hints_without_location() { - let inlay_hints = get_inlay_hints(0, 3).await; + let inlay_hints = get_inlay_hints(0, 3, type_hints()).await; assert_eq!(inlay_hints.len(), 1); let inlay_hint = &inlay_hints[0]; @@ -520,7 +774,7 @@ mod inlay_hints_tests { #[test] async fn test_type_inlay_hints_with_location() { - let inlay_hints = get_inlay_hints(12, 15).await; + let inlay_hints = get_inlay_hints(12, 15, type_hints()).await; assert_eq!(inlay_hints.len(), 1); let inlay_hint = &inlay_hints[0]; @@ -548,7 +802,7 @@ mod inlay_hints_tests { #[test] async fn test_type_inlay_hints_in_for() { - let inlay_hints = get_inlay_hints(16, 18).await; + let inlay_hints = get_inlay_hints(16, 18, type_hints()).await; assert_eq!(inlay_hints.len(), 1); let inlay_hint = &inlay_hints[0]; @@ -566,7 +820,7 @@ mod inlay_hints_tests { #[test] async fn test_type_inlay_hints_in_global() { - let inlay_hints = get_inlay_hints(19, 21).await; + let inlay_hints = get_inlay_hints(19, 21, type_hints()).await; assert_eq!(inlay_hints.len(), 1); let inlay_hint = &inlay_hints[0]; @@ -584,64 +838,92 @@ mod inlay_hints_tests { #[test] async fn test_do_not_panic_when_given_line_is_too_big() { - let inlay_hints = get_inlay_hints(0, 100000).await; + let inlay_hints = get_inlay_hints(0, 100000, type_hints()).await; assert!(!inlay_hints.is_empty()); } -} -// These functions are copied from the codespan_lsp crate, except that they never panic -// (the library will sometimes panic, so functions returning Result are not always accurate) + #[test] + async fn test_do_not_collect_parameter_inlay_hints_if_disabled() { + let inlay_hints = get_inlay_hints(24, 26, no_hints()).await; + assert!(inlay_hints.is_empty()); + } -fn range_to_byte_span( - files: &FileMap, - file_id: FileId, - range: &lsp_types::Range, -) -> Option> { - Some( - position_to_byte_index(files, file_id, &range.start)? - ..position_to_byte_index(files, file_id, &range.end)?, - ) -} + #[test] + async fn test_collect_parameter_inlay_hints_in_function_call() { + let inlay_hints = get_inlay_hints(24, 26, parameter_hints()).await; + assert_eq!(inlay_hints.len(), 2); -fn position_to_byte_index( - files: &FileMap, - file_id: FileId, - position: &lsp_types::Position, -) -> Option { - let Ok(source) = files.source(file_id) else { - return None; - }; + let inlay_hint = &inlay_hints[0]; + assert_eq!(inlay_hint.position, Position { line: 25, character: 12 }); + if let InlayHintLabel::String(label) = &inlay_hint.label { + assert_eq!(label, "one: "); + } else { + panic!("Expected InlayHintLabel::String, got {:?}", inlay_hint.label); + } - let Ok(line_span) = files.line_range(file_id, position.line as usize) else { - return None; - }; - let line_str = source.get(line_span.clone())?; + let inlay_hint = &inlay_hints[1]; + assert_eq!(inlay_hint.position, Position { line: 25, character: 15 }); + if let InlayHintLabel::String(label) = &inlay_hint.label { + assert_eq!(label, "two: "); + } else { + panic!("Expected InlayHintLabel::String, got {:?}", inlay_hint.label); + } + } - let byte_offset = character_to_line_offset(line_str, position.character)?; + #[test] + async fn test_collect_parameter_inlay_hints_in_method_call() { + let inlay_hints = get_inlay_hints(36, 39, parameter_hints()).await; + assert_eq!(inlay_hints.len(), 1); - Some(line_span.start + byte_offset) -} + let inlay_hint = &inlay_hints[0]; + assert_eq!(inlay_hint.position, Position { line: 38, character: 18 }); + if let InlayHintLabel::String(label) = &inlay_hint.label { + assert_eq!(label, "one: "); + } else { + panic!("Expected InlayHintLabel::String, got {:?}", inlay_hint.label); + } + } -fn character_to_line_offset(line: &str, character: u32) -> Option { - let line_len = line.len(); - let mut character_offset = 0; + #[test] + async fn test_do_not_show_parameter_inlay_hints_if_name_matches_var_name() { + let inlay_hints = get_inlay_hints(41, 45, parameter_hints()).await; + assert!(inlay_hints.is_empty()); + } - let mut chars = line.chars(); - while let Some(ch) = chars.next() { - if character_offset == character { - let chars_off = chars.as_str().len(); - let ch_off = ch.len_utf8(); + #[test] + async fn test_do_not_show_parameter_inlay_hints_if_name_matches_member_name() { + let inlay_hints = get_inlay_hints(48, 52, parameter_hints()).await; + assert!(inlay_hints.is_empty()); + } - return Some(line_len - chars_off - ch_off); - } + #[test] + async fn test_do_not_show_parameter_inlay_hints_if_name_matches_call_name() { + let inlay_hints = get_inlay_hints(57, 60, parameter_hints()).await; + assert!(inlay_hints.is_empty()); + } - character_offset += ch.len_utf16() as u32; + #[test] + async fn test_do_not_show_parameter_inlay_hints_if_single_param_name_is_suffix_of_function_name( + ) { + let inlay_hints = get_inlay_hints(64, 67, parameter_hints()).await; + assert!(inlay_hints.is_empty()); } - // Handle positions after the last character on the line - if character_offset == character { - Some(line_len) - } else { - None + #[test] + async fn test_do_not_show_parameter_inlay_hints_if_param_name_starts_with_underscore() { + let inlay_hints = get_inlay_hints(71, 73, parameter_hints()).await; + assert!(inlay_hints.is_empty()); + } + + #[test] + async fn test_do_not_show_parameter_inlay_hints_if_single_argument_with_single_letter() { + let inlay_hints = get_inlay_hints(77, 79, parameter_hints()).await; + assert!(inlay_hints.is_empty()); + } + + #[test] + async fn test_do_not_show_parameter_inlay_hints_if_param_name_is_suffix_of_arg_name() { + let inlay_hints = get_inlay_hints(89, 92, parameter_hints()).await; + assert!(inlay_hints.is_empty()); } } diff --git a/noir/noir-repo/tooling/lsp/src/requests/mod.rs b/noir/noir-repo/tooling/lsp/src/requests/mod.rs index 5e2250ff248..08c4bc32b65 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/mod.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/mod.rs @@ -56,15 +56,39 @@ pub(crate) use { /// LSP client will send initialization request after the server has started. /// [InitializeParams].`initialization_options` will contain the options sent from the client. -#[derive(Debug, Deserialize, Serialize)] -struct LspInitializationOptions { +#[derive(Debug, Deserialize, Serialize, Copy, Clone)] +pub(crate) struct LspInitializationOptions { /// Controls whether code lens is enabled by the server /// By default this will be set to true (enabled). #[serde(rename = "enableCodeLens", default = "default_enable_code_lens")] - enable_code_lens: bool, + pub(crate) enable_code_lens: bool, #[serde(rename = "enableParsingCache", default = "default_enable_parsing_cache")] - enable_parsing_cache: bool, + pub(crate) enable_parsing_cache: bool, + + #[serde(rename = "inlayHints", default = "default_inlay_hints")] + pub(crate) inlay_hints: InlayHintsOptions, +} + +#[derive(Debug, Deserialize, Serialize, Copy, Clone)] +pub(crate) struct InlayHintsOptions { + #[serde(rename = "typeHints", default = "default_type_hints")] + pub(crate) type_hints: TypeHintsOptions, + + #[serde(rename = "parameterHints", default = "default_parameter_hints")] + pub(crate) parameter_hints: ParameterHintsOptions, +} + +#[derive(Debug, Deserialize, Serialize, Copy, Clone)] +pub(crate) struct TypeHintsOptions { + #[serde(rename = "enabled", default = "default_type_hints_enabled")] + pub(crate) enabled: bool, +} + +#[derive(Debug, Deserialize, Serialize, Copy, Clone)] +pub(crate) struct ParameterHintsOptions { + #[serde(rename = "enabled", default = "default_parameter_hints_enabled")] + pub(crate) enabled: bool, } fn default_enable_code_lens() -> bool { @@ -75,11 +99,35 @@ fn default_enable_parsing_cache() -> bool { true } +fn default_inlay_hints() -> InlayHintsOptions { + InlayHintsOptions { + type_hints: default_type_hints(), + parameter_hints: default_parameter_hints(), + } +} + +fn default_type_hints() -> TypeHintsOptions { + TypeHintsOptions { enabled: default_type_hints_enabled() } +} + +fn default_type_hints_enabled() -> bool { + true +} + +fn default_parameter_hints() -> ParameterHintsOptions { + ParameterHintsOptions { enabled: default_parameter_hints_enabled() } +} + +fn default_parameter_hints_enabled() -> bool { + true +} + impl Default for LspInitializationOptions { fn default() -> Self { Self { enable_code_lens: default_enable_code_lens(), enable_parsing_cache: default_enable_parsing_cache(), + inlay_hints: default_inlay_hints(), } } } @@ -93,7 +141,7 @@ pub(crate) fn on_initialize( .initialization_options .and_then(|value| serde_json::from_value(value).ok()) .unwrap_or_default(); - state.parsing_cache_enabled = initialization_options.enable_parsing_cache; + state.options = initialization_options; async move { let text_document_sync = TextDocumentSyncCapability::Kind(TextDocumentSyncKind::FULL); diff --git a/noir/noir-repo/tooling/lsp/test_programs/inlay_hints/src/main.nr b/noir/noir-repo/tooling/lsp/test_programs/inlay_hints/src/main.nr index 005444ec0ae..2b53f8de339 100644 --- a/noir/noir-repo/tooling/lsp/test_programs/inlay_hints/src/main.nr +++ b/noir/noir-repo/tooling/lsp/test_programs/inlay_hints/src/main.nr @@ -20,3 +20,75 @@ fn test_for() { global var = 0; +fn test_fn(one: i32, two: i32) {} + +fn call_test_fn() { + test_fn(1, 2); // Should show parameter names +} + +struct SomeStruct { + one: i32, +} + +impl SomeStruct { + fn some_method(self, one: i32) {} +} + +fn call_method() { + let s = SomeStruct { one: 1 }; + s.some_method(1); // Should show parameter names +} + +fn call_where_name_matches() { + let one = 1; + let two = 2; + test_fn(one, two); // Should not show parameter names (names match) +} + +fn call_where_member_name_matches() { + let s = SomeStruct { one: 1 }; + let two = 2; + test_fn(s.one, two); // Should not show parameter names (member name matches) +} + +fn one() -> i32 { + 1 +} + +fn call_where_call_matches_name() { + let two = 2; + test_fn(one(), two); // Should not show parameter names (call name matches) +} + +fn with_arg(arg: i32) {} + +fn call_with_arg() { + let x = 1; + with_arg(x); // Should not show parameter names ("arg" is a suffix of "with_arg") +} + +fn with_underscore(_x: i32) {} + +fn call_with_underscore() { + with_underscore(1); // Should not show parameter names (param name starts with underscore) +} + +fn one_arg_with_one_char(x: i32) {} + +fn call_one_arg_with_one_char() { + one_arg_with_one_char(1); // Should not show parameter names (only one param and it's a single letter) +} + +fn one_arg_with_obvious_name(other: i32) {} + +fn call_one_arg_with_obvious_name() { + one_arg_with_obvious_name(1); // Should not show parameter names (only one param and it's an obvious name) +} + +fn yet_another_function(name: i32) {} + +fn call_yet_another_function() { + let some_name = 1; + yet_another_function(some_name) // Should not show parameter names ("name" is a suffix of "some_name") +} +