Skip to content

Commit

Permalink
fix: let LSP autocompletion work in more contexts (#5719)
Browse files Browse the repository at this point in the history
# Description

## Problem

There were a couple of contexts where autocompletion wouldn't trigger.
This PR fixes some of these.

## Summary

### Autocompletion inside if condition without following body

Before:


![lsp-autocomplete-if-before](https://github.com/user-attachments/assets/e4d96573-9c96-4ab7-b5b7-7c99a87c4b3a)

After:


![lsp-autocomplete-if-after](https://github.com/user-attachments/assets/318569b2-9556-41f1-bd63-c68eda3e7371)

### Autocompletion in nested expression

Before:


![lsp-autocomplete-nested-before](https://github.com/user-attachments/assets/ef1aa9f8-4c30-4796-9672-dc54af913b69)

The reason is that is was autocompleting for the type of `foo.bar & foo`
instead of just `foo`.

After:


![lsp-autocomplete-nested-after](https://github.com/user-attachments/assets/98c28c43-029d-4305-ba41-18f2697ba58f)


### Autocompletion in call chains

Before:


![lsp-autocomplete-chain-before](https://github.com/user-attachments/assets/d2d3e4f5-3b00-4f80-bd37-25cf76288d8e)

After:


![lsp-autocomplete-chain-after](https://github.com/user-attachments/assets/bead94ba-55d2-40f1-896e-58625fd7f542)

### Autocompletion when assignment follows

Before:


![lsp-autocomplete-assignment-follows-before](https://github.com/user-attachments/assets/80a5db63-bcd3-421a-85e4-9acdfec79763)

After:


![lsp-autocomplete-assignment-follows-after](https://github.com/user-attachments/assets/328a2a83-959a-4252-b974-17f547f7fc13)

## Additional Context

I might send a follow-up PR to split `completion.rs` into several files
because it's getting pretty big. For example tests could go in a
separate file.

## 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.
  • Loading branch information
asterite authored Aug 14, 2024
1 parent 86de991 commit 03ba6dd
Show file tree
Hide file tree
Showing 4 changed files with 159 additions and 19 deletions.
3 changes: 2 additions & 1 deletion compiler/noirc_frontend/src/elaborator/expressions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,8 @@ impl<'context> Elaborator<'context> {
function_args.push((typ, arg, span));
}

let location = Location::new(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;
Expand Down
2 changes: 2 additions & 0 deletions compiler/noirc_frontend/src/parser/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Expand Down
48 changes: 44 additions & 4 deletions compiler/noirc_frontend/src/parser/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -954,10 +954,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,
}))
}
})
})
}
Expand Down Expand Up @@ -1431,6 +1453,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();
Expand Down
125 changes: 111 additions & 14 deletions tooling/lsp/src/requests/completion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -477,19 +488,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) => {
Expand Down Expand Up @@ -554,6 +552,23 @@ 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);
}
}
}

fn find_in_literal(&mut self, literal: &Literal) {
Expand Down Expand Up @@ -2883,4 +2898,86 @@ 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;
}

#[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;
}

#[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;
}

#[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;
}
}

0 comments on commit 03ba6dd

Please sign in to comment.