Skip to content

Commit

Permalink
feat: fault-tolerant parsing of fn and impl (#5753)
Browse files Browse the repository at this point in the history
# Description

## Problem

Part of #1577

## Summary

Autocompletion wasn't triggering in function parameter types, or in impl
names. The reason is that when you start writing them the body will
still be missing, and that discarded the entire fn or impl. In this PR
we make that parse well, but produce an error (and create a fake empty
body).

Now autocompletion works fine in these scenarios:


![lsp-recover-fn](https://github.com/user-attachments/assets/e2358d49-c2e3-473b-bd4c-bfc6c0ff43e9)


![lsp-recover-impl](https://github.com/user-attachments/assets/a8fb1d44-86ce-4514-b167-f1ff6eb4b9b7)

## Additional Context

I'm not sure about the many new "parser error" introduced, maybe there
should be a generic "expected these tokens"? I think chumsky has one of
those errors but it's kind of tied to the parsing logic. On the other
hand adding new errors is simple, so maybe it's fine.

## 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 19, 2024
1 parent d3f20c6 commit d4e2f0a
Show file tree
Hide file tree
Showing 6 changed files with 220 additions and 13 deletions.
8 changes: 8 additions & 0 deletions compiler/noirc_frontend/src/parser/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,16 @@ pub enum ParserErrorReason {
ExpectedIdentifierAfterDot,
#[error("expected an identifier after ::")]
ExpectedIdentifierAfterColons,
#[error("expected {{ or -> after function parameters")]
ExpectedLeftBraceOrArrowAfterFunctionParameters,
#[error("expected {{ after if condition")]
ExpectedLeftBraceAfterIfCondition,
#[error("expected <, where or {{ after trait name")]
ExpectedLeftBracketOrWhereOrLeftBraceOrArrowAfterTraitName,
#[error("expected <, where or {{ after impl type")]
ExpectedLeftBracketOrWhereOrLeftBraceOrArrowAfterImplType,
#[error("expected <, where or {{ after trait impl for type")]
ExpectedLeftBracketOrWhereOrLeftBraceOrArrowAfterTraitImplForType,
#[error("Expected a ; separating these two statements")]
MissingSeparatingSemi,
#[error("constrain keyword is deprecated")]
Expand Down
37 changes: 34 additions & 3 deletions compiler/noirc_frontend/src/parser/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,13 +219,27 @@ fn top_level_statement<'a>(
///
/// implementation: 'impl' generics type '{' function_definition ... '}'
fn implementation() -> impl NoirParser<TopLevelStatement> {
let methods_or_error = just(Token::LeftBrace)
.ignore_then(spanned(function::function_definition(true)).repeated())
.then_ignore(just(Token::RightBrace))
.or_not()
.validate(|methods, span, emit| {
if let Some(methods) = methods {
methods
} else {
emit(ParserError::with_reason(
ParserErrorReason::ExpectedLeftBracketOrWhereOrLeftBraceOrArrowAfterImplType,
span,
));
vec![]
}
});

keyword(Keyword::Impl)
.ignore_then(function::generics())
.then(parse_type().map_with_span(|typ, span| (typ, span)))
.then(where_clause())
.then_ignore(just(Token::LeftBrace))
.then(spanned(function::function_definition(true)).repeated())
.then_ignore(just(Token::RightBrace))
.then(methods_or_error)
.map(|args| {
let ((other_args, where_clause), methods) = args;
let (generics, (object_type, type_span)) = other_args;
Expand Down Expand Up @@ -1760,4 +1774,21 @@ mod test {

assert_eq!(var.to_string(), "foo");
}

#[test]
fn parse_recover_impl_without_body() {
let src = "impl Foo";

let (top_level_statement, errors) = parse_recover(implementation(), src);
assert_eq!(errors.len(), 1);
assert_eq!(errors[0].message, "expected <, where or { after impl type");

let top_level_statement = top_level_statement.unwrap();
let TopLevelStatement::Impl(impl_) = top_level_statement else {
panic!("Expected to parse an impl");
};

assert_eq!(impl_.object_type.to_string(), "Foo");
assert!(impl_.methods.is_empty());
}
}
35 changes: 33 additions & 2 deletions compiler/noirc_frontend/src/parser/parser/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@ use super::{
self_parameter, where_clause, NoirParser,
};
use crate::token::{Keyword, Token, TokenKind};
use crate::{ast::IntegerBitSize, parser::spanned};
use crate::{
ast::{BlockExpression, IntegerBitSize},
parser::spanned,
};
use crate::{
ast::{
FunctionDefinition, FunctionReturnType, ItemVisibility, NoirFunction, Param, Visibility,
Expand All @@ -20,10 +23,24 @@ use crate::{
};

use chumsky::prelude::*;
use noirc_errors::Span;

/// function_definition: attribute function_modifiers 'fn' ident generics '(' function_parameters ')' function_return_type block
/// function_modifiers 'fn' ident generics '(' function_parameters ')' function_return_type block
pub(super) fn function_definition(allow_self: bool) -> impl NoirParser<NoirFunction> {
let body_or_error =
spanned(block(fresh_statement()).or_not()).validate(|(body, body_span), span, emit| {
if let Some(body) = body {
(body, body_span)
} else {
emit(ParserError::with_reason(
ParserErrorReason::ExpectedLeftBraceOrArrowAfterFunctionParameters,
span,
));
(BlockExpression { statements: vec![] }, Span::from(span.end()..span.end()))
}
});

attributes()
.then(function_modifiers())
.then_ignore(keyword(Keyword::Fn))
Expand All @@ -32,7 +49,7 @@ pub(super) fn function_definition(allow_self: bool) -> impl NoirParser<NoirFunct
.then(parenthesized(function_parameters(allow_self)))
.then(function_return_type())
.then(where_clause())
.then(spanned(block(fresh_statement())))
.then(body_or_error)
.validate(|(((args, ret), where_clause), (body, body_span)), span, emit| {
let ((((attributes, modifiers), name), generics), parameters) = args;

Expand Down Expand Up @@ -268,4 +285,18 @@ mod test {
],
);
}

#[test]
fn parse_recover_function_without_body() {
let src = "fn foo(x: i32)";

let (noir_function, errors) = parse_recover(function_definition(false), src);
assert_eq!(errors.len(), 1);
assert_eq!(errors[0].message, "expected { or -> after function parameters");

let noir_function = noir_function.unwrap();
assert_eq!(noir_function.name(), "foo");
assert_eq!(noir_function.parameters().len(), 1);
assert!(noir_function.def.body.statements.is_empty());
}
}
108 changes: 100 additions & 8 deletions compiler/noirc_frontend/src/parser/parser/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,28 @@ use crate::{
use super::{generic_type_args, parse_type, primitives::ident};

pub(super) fn trait_definition() -> impl NoirParser<TopLevelStatement> {
let trait_body_or_error = just(Token::LeftBrace)
.ignore_then(trait_body())
.then_ignore(just(Token::RightBrace))
.or_not()
.validate(|items, span, emit| {
if let Some(items) = items {
items
} else {
emit(ParserError::with_reason(
ParserErrorReason::ExpectedLeftBracketOrWhereOrLeftBraceOrArrowAfterTraitName,
span,
));
vec![]
}
});

attributes()
.then_ignore(keyword(Keyword::Trait))
.then(ident())
.then(function::generics())
.then(where_clause())
.then_ignore(just(Token::LeftBrace))
.then(trait_body())
.then_ignore(just(Token::RightBrace))
.then(trait_body_or_error)
.validate(|((((attributes, name), generics), where_clause), items), span, emit| {
let attributes = validate_secondary_attributes(attributes, span, emit);
TopLevelStatement::Trait(NoirTrait {
Expand Down Expand Up @@ -70,13 +84,26 @@ fn trait_function_declaration() -> impl NoirParser<TraitItem> {
let trait_function_body_or_semicolon =
block(fresh_statement()).map(Option::from).or(just(Token::Semicolon).to(Option::None));

let trait_function_body_or_semicolon_or_error =
trait_function_body_or_semicolon.or_not().validate(|body, span, emit| {
if let Some(body) = body {
body
} else {
emit(ParserError::with_reason(
ParserErrorReason::ExpectedLeftBraceOrArrowAfterFunctionParameters,
span,
));
None
}
});

keyword(Keyword::Fn)
.ignore_then(ident())
.then(function::generics())
.then(parenthesized(function_declaration_parameters()))
.then(function_return_type().map(|(_, typ)| typ))
.then(where_clause())
.then(trait_function_body_or_semicolon)
.then(trait_function_body_or_semicolon_or_error)
.map(|(((((name, generics), parameters), return_type), where_clause), body)| {
TraitItem::Function { name, generics, parameters, return_type, where_clause, body }
})
Expand All @@ -101,20 +128,35 @@ fn trait_type_declaration() -> impl NoirParser<TraitItem> {
///
/// trait_implementation: 'impl' generics ident generic_args for type '{' trait_implementation_body '}'
pub(super) fn trait_implementation() -> impl NoirParser<TopLevelStatement> {
let body_or_error =
just(Token::LeftBrace)
.ignore_then(trait_implementation_body())
.then_ignore(just(Token::RightBrace))
.or_not()
.validate(|items, span, emit| {
if let Some(items) = items {
items
} else {
emit(ParserError::with_reason(
ParserErrorReason::ExpectedLeftBracketOrWhereOrLeftBraceOrArrowAfterTraitImplForType,
span,
));

vec![]
}
});

keyword(Keyword::Impl)
.ignore_then(function::generics())
.then(path_no_turbofish())
.then(generic_type_args(parse_type()))
.then_ignore(keyword(Keyword::For))
.then(parse_type())
.then(where_clause())
.then_ignore(just(Token::LeftBrace))
.then(trait_implementation_body())
.then_ignore(just(Token::RightBrace))
.then(body_or_error)
.map(|args| {
let (((other_args, object_type), where_clause), items) = args;
let ((impl_generics, trait_name), trait_generics) = other_args;

TopLevelStatement::TraitImpl(NoirTraitImpl {
impl_generics,
trait_name,
Expand Down Expand Up @@ -227,4 +269,54 @@ mod test {
vec!["trait MissingBody", "trait WrongDelimiter { fn foo() -> u8, fn bar() -> u8 }"],
);
}

#[test]
fn parse_recover_function_without_left_brace_or_semicolon() {
let src = "fn foo(x: i32)";

let (trait_item, errors) = parse_recover(trait_function_declaration(), src);
assert_eq!(errors.len(), 1);
assert_eq!(errors[0].message, "expected { or -> after function parameters");

let Some(TraitItem::Function { name, parameters, body, .. }) = trait_item else {
panic!("Expected to parser trait item as function");
};

assert_eq!(name.to_string(), "foo");
assert_eq!(parameters.len(), 1);
assert!(body.is_none());
}

#[test]
fn parse_recover_trait_without_body() {
let src = "trait Foo";

let (top_level_statement, errors) = parse_recover(trait_definition(), src);
assert_eq!(errors.len(), 1);
assert_eq!(errors[0].message, "expected <, where or { after trait name");

let top_level_statement = top_level_statement.unwrap();
let TopLevelStatement::Trait(trait_) = top_level_statement else {
panic!("Expected to parse a trait");
};

assert_eq!(trait_.name.to_string(), "Foo");
assert!(trait_.items.is_empty());
}

#[test]
fn parse_recover_trait_impl_without_body() {
let src = "impl Foo for Bar";

let (top_level_statement, errors) = parse_recover(trait_implementation(), src);
assert_eq!(errors.len(), 1);
assert_eq!(errors[0].message, "expected <, where or { after trait impl for type");

let top_level_statement = top_level_statement.unwrap();
let TopLevelStatement::TraitImpl(trait_impl) = top_level_statement else {
panic!("Expected to parse a trait impl");
};

assert!(trait_impl.items.is_empty());
}
}
5 changes: 5 additions & 0 deletions tooling/lsp/src/requests/completion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,9 @@ impl<'a> NodeFinder<'a> {
}

fn find_in_noir_trait_impl(&mut self, noir_trait_impl: &NoirTraitImpl) {
self.find_in_path(&noir_trait_impl.trait_name, RequestedItems::OnlyTypes);
self.find_in_unresolved_type(&noir_trait_impl.object_type);

self.type_parameters.clear();
self.collect_type_parameters_in_generics(&noir_trait_impl.impl_generics);

Expand All @@ -263,6 +266,8 @@ impl<'a> NodeFinder<'a> {
}

fn find_in_type_impl(&mut self, type_impl: &TypeImpl) {
self.find_in_unresolved_type(&type_impl.object_type);

self.type_parameters.clear();
self.collect_type_parameters_in_generics(&type_impl.generics);

Expand Down
40 changes: 40 additions & 0 deletions tooling/lsp/src/requests/completion/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1573,4 +1573,44 @@ mod completion_tests {
"#;
assert_completion(src, vec![field_completion_item("some_property", "i32")]).await;
}

#[test]
async fn test_completes_in_impl_type() {
let src = r#"
struct FooBar {
}
impl FooB>|<
"#;

assert_completion(
src,
vec![simple_completion_item(
"FooBar",
CompletionItemKind::STRUCT,
Some("FooBar".to_string()),
)],
)
.await;
}

#[test]
async fn test_completes_in_impl_for_type() {
let src = r#"
struct FooBar {
}
impl Default for FooB>|<
"#;

assert_completion(
src,
vec![simple_completion_item(
"FooBar",
CompletionItemKind::STRUCT,
Some("FooBar".to_string()),
)],
)
.await;
}
}

0 comments on commit d4e2f0a

Please sign in to comment.