From 7cfd872faa95539156a80ddb9506172d23fef41b Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 14 Aug 2024 07:53:21 -0300 Subject: [PATCH 1/7] fix: allow autocompletion to work in broken if --- compiler/noirc_frontend/src/parser/errors.rs | 2 + compiler/noirc_frontend/src/parser/parser.rs | 48 ++++++++++++++++++-- tooling/lsp/src/requests/completion.rs | 18 ++++++++ 3 files changed, 64 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_frontend/src/parser/errors.rs b/compiler/noirc_frontend/src/parser/errors.rs index b2de3b0794b..72719adb4f8 100644 --- a/compiler/noirc_frontend/src/parser/errors.rs +++ b/compiler/noirc_frontend/src/parser/errors.rs @@ -20,6 +20,8 @@ pub enum ParserErrorReason { ExpectedIdentifierAfterDot, #[error("expected an identifier after ::")] ExpectedIdentifierAfterColons, + #[error("expected {{ after if condition")] + ExpectedLeftBraceAfterIfCondition, #[error("Expected a ; separating these two statements")] MissingSeparatingSemi, #[error("constrain keyword is deprecated")] diff --git a/compiler/noirc_frontend/src/parser/parser.rs b/compiler/noirc_frontend/src/parser/parser.rs index 876fc454565..533bbe72414 100644 --- a/compiler/noirc_frontend/src/parser/parser.rs +++ b/compiler/noirc_frontend/src/parser/parser.rs @@ -944,10 +944,32 @@ where keyword(Keyword::If) .ignore_then(expr_no_constructors) - .then(if_block) - .then(keyword(Keyword::Else).ignore_then(else_block).or_not()) - .map(|((condition, consequence), alternative)| { - ExpressionKind::If(Box::new(IfExpression { condition, consequence, alternative })) + .then(if_block.then(keyword(Keyword::Else).ignore_then(else_block).or_not()).or_not()) + .validate(|(condition, consequence_and_alternative), span, emit_error| { + if let Some((consequence, alternative)) = consequence_and_alternative { + ExpressionKind::If(Box::new(IfExpression { + condition, + consequence, + alternative, + })) + } else { + // We allow `if cond` without a block mainly for LSP, so that it parses right + // and autocompletion works there. + emit_error(ParserError::with_reason( + ParserErrorReason::ExpectedLeftBraceAfterIfCondition, + span, + )); + + let span_end = condition.span.end(); + ExpressionKind::If(Box::new(IfExpression { + condition, + consequence: Expression::new( + ExpressionKind::Error, + Span::from(span_end..span_end), + ), + alternative: None, + })) + } }) }) } @@ -1420,6 +1442,24 @@ mod test { ); } + #[test] + fn parse_if_without_block() { + let src = "if foo"; + let parser = if_expr(expression_no_constructors(expression()), fresh_statement()); + let (expression_kind, errors) = parse_recover(parser, src); + + let expression_kind = expression_kind.unwrap(); + let ExpressionKind::If(if_expression) = expression_kind else { + panic!("Expected an if expression, got {:?}", expression_kind); + }; + + assert_eq!(if_expression.consequence.kind, ExpressionKind::Error); + assert_eq!(if_expression.alternative, None); + + assert_eq!(errors.len(), 1); + assert_eq!(errors[0].message, "expected { after if condition"); + } + #[test] fn parse_module_declaration() { parse_with(module_declaration(), "mod foo").unwrap(); diff --git a/tooling/lsp/src/requests/completion.rs b/tooling/lsp/src/requests/completion.rs index 028bbcd6b28..ed148ad4924 100644 --- a/tooling/lsp/src/requests/completion.rs +++ b/tooling/lsp/src/requests/completion.rs @@ -2877,4 +2877,22 @@ mod completion_tests { ) .await; } + + #[test] + async fn test_completes_in_broken_if_after_dot() { + let src = r#" + struct Some { + foo: i32, + } + + fn foo(s: Some) { + if s.>|< + } + "#; + assert_completion( + src, + vec![simple_completion_item("foo", CompletionItemKind::FIELD, Some("i32".to_string()))], + ) + .await; + } } From c2d9ca0445b42640f745d4615bbca139f80737a3 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 14 Aug 2024 08:22:05 -0300 Subject: [PATCH 2/7] Autocomplete for nested expressions first --- tooling/lsp/src/requests/completion.rs | 48 +++++++++++++++++++------- 1 file changed, 35 insertions(+), 13 deletions(-) diff --git a/tooling/lsp/src/requests/completion.rs b/tooling/lsp/src/requests/completion.rs index ed148ad4924..7c529854e29 100644 --- a/tooling/lsp/src/requests/completion.rs +++ b/tooling/lsp/src/requests/completion.rs @@ -477,19 +477,6 @@ impl<'a> NodeFinder<'a> { } fn find_in_expression(&mut self, expression: &Expression) { - // "foo." (no identifier afterwards) is parsed as the expression on the left hand-side of the dot. - // Here we check if there's a dot at the completion position, and if the expression - // ends right before the dot. If so, it means we want to complete the expression's type fields and methods. - if self.byte == Some(b'.') && expression.span.end() as usize == self.byte_index - 1 { - let location = Location::new(expression.span, self.file); - if let Some(typ) = self.interner.type_at_location(location) { - let typ = typ.follow_bindings(); - let prefix = ""; - self.complete_type_fields_and_methods(&typ, prefix); - return; - } - } - match &expression.kind { noirc_frontend::ast::ExpressionKind::Literal(literal) => self.find_in_literal(literal), noirc_frontend::ast::ExpressionKind::Block(block_expression) => { @@ -551,6 +538,24 @@ impl<'a> NodeFinder<'a> { | noirc_frontend::ast::ExpressionKind::Resolved(_) | noirc_frontend::ast::ExpressionKind::Error => (), } + + // "foo." (no identifier afterwards) is parsed as the expression on the left hand-side of the dot. + // Here we check if there's a dot at the completion position, and if the expression + // ends right before the dot. If so, it means we want to complete the expression's type fields and methods. + // We only do this after visiting nested expressions, because in an expression like `foo & bar.` we want + // to complete for `bar`, not for `foo & bar`. + if self.completion_items.is_empty() + && self.byte == Some(b'.') + && expression.span.end() as usize == self.byte_index - 1 + { + let location = Location::new(expression.span, self.file); + if let Some(typ) = self.interner.type_at_location(location) { + let typ = typ.follow_bindings(); + let prefix = ""; + self.complete_type_fields_and_methods(&typ, prefix); + return; + } + } } fn find_in_literal(&mut self, literal: &Literal) { @@ -2895,4 +2900,21 @@ mod completion_tests { ) .await; } + + #[test] + async fn test_completes_in_nested_expression() { + let src = r#" + struct Foo { bar: Bar } + struct Bar { baz: i32 } + + fn foo(f: Foo) { + f.bar & f.>|< + } + "#; + assert_completion( + src, + vec![simple_completion_item("bar", CompletionItemKind::FIELD, Some("Bar".to_string()))], + ) + .await; + } } From 63d16443aca567b0ca9e7d1f1347230d2d05f71c Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 14 Aug 2024 10:21:37 -0300 Subject: [PATCH 3/7] Use object span for method call to make LSP work in call chains --- .../src/elaborator/expressions.rs | 2 +- tooling/lsp/src/requests/completion.rs | 25 +++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/elaborator/expressions.rs b/compiler/noirc_frontend/src/elaborator/expressions.rs index 5ba448f890e..86b5bca5b12 100644 --- a/compiler/noirc_frontend/src/elaborator/expressions.rs +++ b/compiler/noirc_frontend/src/elaborator/expressions.rs @@ -369,7 +369,7 @@ impl<'context> Elaborator<'context> { function_args.push((typ, arg, span)); } - let location = Location::new(span, self.file); + let location = Location::new(object_span, self.file); let method = method_call.method_name; let turbofish_generics = generics.clone(); let is_macro_call = method_call.is_macro_call; diff --git a/tooling/lsp/src/requests/completion.rs b/tooling/lsp/src/requests/completion.rs index 7c529854e29..124a47d3290 100644 --- a/tooling/lsp/src/requests/completion.rs +++ b/tooling/lsp/src/requests/completion.rs @@ -2917,4 +2917,29 @@ mod completion_tests { ) .await; } + + #[test] + async fn test_completes_in_call_chain() { + let src = r#" + struct Foo {} + + impl Foo { + fn foo(self) -> Foo { self } + } + + fn foo(f: Foo) { + f.foo().>|< + } + "#; + assert_completion( + src, + vec![snippet_completion_item( + "foo()", + CompletionItemKind::FUNCTION, + "foo()", + Some("fn(self) -> Foo".to_string()), + )], + ) + .await; + } } From e8a9e0396db5b7a99d2ab854d090a1359d7e372a Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 14 Aug 2024 10:24:34 -0300 Subject: [PATCH 4/7] Make it work when assignment follows --- tooling/lsp/src/requests/completion.rs | 35 +++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/tooling/lsp/src/requests/completion.rs b/tooling/lsp/src/requests/completion.rs index 124a47d3290..ef97634fd34 100644 --- a/tooling/lsp/src/requests/completion.rs +++ b/tooling/lsp/src/requests/completion.rs @@ -450,7 +450,18 @@ impl<'a> NodeFinder<'a> { fn find_in_lvalue(&mut self, lvalue: &LValue) { match lvalue { - LValue::Ident(_) => (), + LValue::Ident(ident) => { + if self.byte == Some(b'.') && ident.span().end() as usize == self.byte_index - 1 { + let location = Location::new(ident.span(), self.file); + if let Some(ReferenceId::Local(definition_id)) = + self.interner.find_referenced(location) + { + let typ = self.interner.definition_type(definition_id); + let prefix = ""; + self.complete_type_fields_and_methods(&typ, prefix); + } + } + } LValue::MemberAccess { object, field_name: _, span: _ } => self.find_in_lvalue(object), LValue::Index { array, index, span: _ } => { self.find_in_lvalue(array); @@ -2942,4 +2953,26 @@ mod completion_tests { ) .await; } + + #[test] + async fn test_completes_when_assignment_follows() { + let src = r#" + struct Foo { + bar: i32, + } + + fn foo(f: Foo) { + let mut x = 1; + + f.>|< + + x = 2; + } + "#; + assert_completion( + src, + vec![simple_completion_item("bar", CompletionItemKind::FIELD, Some("i32".to_string()))], + ) + .await; + } } From 82b0cdac19e420fd7084de29ea73ce317890f151 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 14 Aug 2024 10:42:38 -0300 Subject: [PATCH 5/7] clippy --- tooling/lsp/src/requests/completion.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/tooling/lsp/src/requests/completion.rs b/tooling/lsp/src/requests/completion.rs index ef97634fd34..5e9a1bc472f 100644 --- a/tooling/lsp/src/requests/completion.rs +++ b/tooling/lsp/src/requests/completion.rs @@ -564,7 +564,6 @@ impl<'a> NodeFinder<'a> { let typ = typ.follow_bindings(); let prefix = ""; self.complete_type_fields_and_methods(&typ, prefix); - return; } } } From 8f774f1219310c1e8e1d5737b15ca25e363b7619 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 14 Aug 2024 10:56:00 -0300 Subject: [PATCH 6/7] Use method_name_span instead of object_span --- compiler/noirc_frontend/src/elaborator/expressions.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/elaborator/expressions.rs b/compiler/noirc_frontend/src/elaborator/expressions.rs index 86b5bca5b12..30abf58121c 100644 --- a/compiler/noirc_frontend/src/elaborator/expressions.rs +++ b/compiler/noirc_frontend/src/elaborator/expressions.rs @@ -369,7 +369,7 @@ impl<'context> Elaborator<'context> { function_args.push((typ, arg, span)); } - let location = Location::new(object_span, self.file); + let location = Location::new(method_name_span, self.file); let method = method_call.method_name; let turbofish_generics = generics.clone(); let is_macro_call = method_call.is_macro_call; From efb0dddefc7acd8c9e64cceecc2309515a457ee5 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 14 Aug 2024 13:32:25 -0300 Subject: [PATCH 7/7] Use object+name span for method call span --- compiler/noirc_frontend/src/elaborator/expressions.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/elaborator/expressions.rs b/compiler/noirc_frontend/src/elaborator/expressions.rs index 30abf58121c..e147bfa4724 100644 --- a/compiler/noirc_frontend/src/elaborator/expressions.rs +++ b/compiler/noirc_frontend/src/elaborator/expressions.rs @@ -369,7 +369,8 @@ impl<'context> Elaborator<'context> { function_args.push((typ, arg, span)); } - let location = Location::new(method_name_span, self.file); + let call_span = Span::from(object_span.start()..method_name_span.end()); + let location = Location::new(call_span, self.file); let method = method_call.method_name; let turbofish_generics = generics.clone(); let is_macro_call = method_call.is_macro_call;