diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs index 80c4573138c..ea787f05e98 100644 --- a/tooling/lsp/src/lib.rs +++ b/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/tooling/lsp/src/requests/inlay_hint.rs b/tooling/lsp/src/requests/inlay_hint.rs index c47e59b0c2b..2afa5fa44fd 100644 --- a/tooling/lsp/src/requests/inlay_hint.rs +++ b/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/tooling/lsp/src/requests/mod.rs b/tooling/lsp/src/requests/mod.rs index 5e2250ff248..08c4bc32b65 100644 --- a/tooling/lsp/src/requests/mod.rs +++ b/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/tooling/lsp/test_programs/inlay_hints/src/main.nr b/tooling/lsp/test_programs/inlay_hints/src/main.nr index 005444ec0ae..2b53f8de339 100644 --- a/tooling/lsp/test_programs/inlay_hints/src/main.nr +++ b/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") +} +