Skip to content

Commit

Permalink
fix: Prevent if and for from parsing constructor expressions (#1916)
Browse files Browse the repository at this point in the history 
* Prevent if and for from parsing constructor expressions

* Update outdated test
  • Loading branch information
@jfecher
jfecher authored Jul 13, 2023
1 parent 5ff8b53 commit 6d3029a
Showing 1 changed file with 133 additions and 50 deletions.
183 changes: 133 additions & 50 deletions crates/noirc_frontend/src/parser/parser.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -875,9 +875,17 @@ fn array_type(type_parser: impl NoirParser<UnresolvedType>) -> impl NoirParser<U
}

fn type_expression() -> impl NoirParser<UnresolvedTypeExpression> {
recursive(|expr| expression_with_precedence(Precedence::lowest_type_precedence(), expr, true))
.labelled(ParsingRuleLabel::TypeExpression)
.try_map(UnresolvedTypeExpression::from_expr)
recursive(|expr| {
expression_with_precedence(
Precedence::lowest_type_precedence(),
nothing(),
expr,
true,
false,
)
})
.labelled(ParsingRuleLabel::TypeExpression)
.try_map(UnresolvedTypeExpression::from_expr)
}

fn tuple_type<T>(type_parser: T) -> impl NoirParser<UnresolvedType>
Expand Down Expand Up @@ -911,8 +919,23 @@ where
}

fn expression() -> impl ExprParser {
recursive(|expr| expression_with_precedence(Precedence::Lowest, expr, false))
.labelled(ParsingRuleLabel::Expression)
recursive(|expr| {
expression_with_precedence(
Precedence::Lowest,
expr,
expression_no_constructors(),
false,
true,
)
})
.labelled(ParsingRuleLabel::Expression)
}

fn expression_no_constructors() -> impl ExprParser {
recursive(|expr| {
expression_with_precedence(Precedence::Lowest, expr.clone(), expr, false, false)
})
.labelled(ParsingRuleLabel::Expression)
}

fn return_statement<'a, P>(expr_parser: P) -> impl NoirParser<Statement> + 'a
Expand All @@ -930,27 +953,40 @@ where
// An expression is a single term followed by 0 or more (OP subexpression)*
// where OP is an operator at the given precedence level and subexpression
// is an expression at the current precedence level plus one.
fn expression_with_precedence<'a, P>(
fn expression_with_precedence<'a, P, P2>(
precedence: Precedence,
expr_parser: P,
expr_no_constructors: P2,
// True if we should only parse the restricted subset of operators valid within type expressions
is_type_expression: bool,
// True if we should also parse constructors `Foo { field1: value1, ... }` as an expression.
// This is disabled when parsing the condition of an if statement due to a parsing conflict
// with `then` bodies containing only a single variable.
allow_constructors: bool,
) -> impl NoirParser<Expression> + 'a
where
P: ExprParser + 'a,
P2: ExprParser + 'a,
{
if precedence == Precedence::Highest {
if is_type_expression {
type_expression_term(expr_parser).boxed().labelled(ParsingRuleLabel::Term)
} else {
term(expr_parser).boxed().labelled(ParsingRuleLabel::Term)
term(expr_parser, expr_no_constructors, allow_constructors)
.boxed()
.labelled(ParsingRuleLabel::Term)
}
} else {
let next_precedence =
if is_type_expression { precedence.next_type_precedence() } else { precedence.next() };

let next_expr =
expression_with_precedence(next_precedence, expr_parser, is_type_expression);
let next_expr = expression_with_precedence(
next_precedence,
expr_parser,
expr_no_constructors,
is_type_expression,
allow_constructors,
);

next_expr
.clone()
Expand Down Expand Up @@ -987,9 +1023,14 @@ fn operator_with_precedence(precedence: Precedence) -> impl NoirParser<Spanned<B
})
}

fn term<'a, P>(expr_parser: P) -> impl NoirParser<Expression> + 'a
fn term<'a, P, P2>(
expr_parser: P,
expr_no_constructors: P2,
allow_constructors: bool,
) -> impl NoirParser<Expression> + 'a
where
P: ExprParser + 'a,
P2: ExprParser + 'a,
{
recursive(move |term_parser| {
choice((
Expand All @@ -1002,7 +1043,7 @@ where
// right-unary operators like a[0] or a.f bind more tightly than left-unary
// operators like - or !, so that !a[0] is parsed as !(a[0]). This is a bit
// awkward for casts so -a as i32 actually binds as -(a as i32).
.or(atom_or_right_unary(expr_parser))
.or(atom_or_right_unary(expr_parser, expr_no_constructors, allow_constructors))
})
}

Expand All @@ -1017,9 +1058,14 @@ where
})
}

fn atom_or_right_unary<'a, P>(expr_parser: P) -> impl NoirParser<Expression> + 'a
fn atom_or_right_unary<'a, P, P2>(
expr_parser: P,
expr_no_constructors: P2,
allow_constructors: bool,
) -> impl NoirParser<Expression> + 'a
where
P: ExprParser + 'a,
P2: ExprParser + 'a,
{
enum UnaryRhs {
Call(Vec<Expression>),
Expand Down Expand Up @@ -1052,17 +1098,27 @@ where

let rhs = choice((call_rhs, array_rhs, cast_rhs, member_rhs));

foldl_with_span(atom(expr_parser), rhs, |lhs, rhs, span| match rhs {
UnaryRhs::Call(args) => Expression::call(lhs, args, span),
UnaryRhs::ArrayIndex(index) => Expression::index(lhs, index, span),
UnaryRhs::Cast(r#type) => Expression::cast(lhs, r#type, span),
UnaryRhs::MemberAccess(field) => Expression::member_access_or_method_call(lhs, field, span),
})
foldl_with_span(
atom(expr_parser, expr_no_constructors, allow_constructors),
rhs,
|lhs, rhs, span| match rhs {
UnaryRhs::Call(args) => Expression::call(lhs, args, span),
UnaryRhs::ArrayIndex(index) => Expression::index(lhs, index, span),
UnaryRhs::Cast(r#type) => Expression::cast(lhs, r#type, span),
UnaryRhs::MemberAccess(field) => {
Expression::member_access_or_method_call(lhs, field, span)
}
},
)
}

fn if_expr<'a, P>(expr_parser: P) -> impl NoirParser<ExpressionKind> + 'a
fn if_expr<'a, P1, P2>(
expr_parser: P1,
expr_no_constructors: P2,
) -> impl NoirParser<ExpressionKind> + 'a
where
P: ExprParser + 'a,
P1: ExprParser + 'a,
P2: ExprParser + 'a,
{
recursive(|if_parser| {
let if_block = block_expr(expr_parser.clone());
Expand All @@ -1077,7 +1133,7 @@ where
}));

keyword(Keyword::If)
.ignore_then(expr_parser)
.ignore_then(expr_no_constructors)
.then(if_block)
.then(keyword(Keyword::Else).ignore_then(else_block).or_not())
.map(|((condition, consequence), alternative)| {
Expand All @@ -1098,29 +1154,33 @@ fn lambda<'a>(
})
}

fn for_expr<'a, P>(expr_parser: P) -> impl NoirParser<ExpressionKind> + 'a
fn for_expr<'a, P, P2>(
expr_parser: P,
expr_no_constructors: P2,
) -> impl NoirParser<ExpressionKind> + 'a
where
P: ExprParser + 'a,
P2: ExprParser + 'a,
{
keyword(Keyword::For)
.ignore_then(ident())
.then_ignore(keyword(Keyword::In))
.then(for_range(expr_parser.clone()))
.then(for_range(expr_no_constructors))
.then(block_expr(expr_parser))
.map_with_span(|((identifier, range), block), span| range.into_for(identifier, block, span))
}

/// The 'range' of a for loop. Either an actual range `start .. end` or an array expression.
fn for_range<P>(expr_parser: P) -> impl NoirParser<ForRange>
fn for_range<P>(expr_no_constructors: P) -> impl NoirParser<ForRange>
where
P: ExprParser,
{
expr_parser
expr_no_constructors
.clone()
.then_ignore(just(Token::DoubleDot))
.then(expr_parser.clone())
.then(expr_no_constructors.clone())
.map(|(start, end)| ForRange::Range(start, end))
.or(expr_parser.map(ForRange::Array))
.or(expr_no_constructors.map(ForRange::Array))
}

fn array_expr<P>(expr_parser: P) -> impl NoirParser<ExpressionKind>
Expand Down Expand Up @@ -1194,15 +1254,27 @@ where
.map(|rhs| ExpressionKind::prefix(UnaryOp::Dereference, rhs))
}

fn atom<'a, P>(expr_parser: P) -> impl NoirParser<Expression> + 'a
/// Atoms are parameterized on whether constructor expressions are allowed or not.
/// Certain constructs like `if` and `for` disallow constructor expressions when a
/// block may be expected.
fn atom<'a, P, P2>(
expr_parser: P,
expr_no_constructors: P2,
allow_constructors: bool,
) -> impl NoirParser<Expression> + 'a
where
P: ExprParser + 'a,
P2: ExprParser + 'a,
{
choice((
if_expr(expr_parser.clone()),
for_expr(expr_parser.clone()),
if_expr(expr_parser.clone(), expr_no_constructors.clone()),
for_expr(expr_parser.clone(), expr_no_constructors),
array_expr(expr_parser.clone()),
constructor(expr_parser.clone()),
if allow_constructors {
constructor(expr_parser.clone()).boxed()
} else {
nothing().boxed()
},
lambda(expr_parser.clone()),
block(expr_parser.clone()).map(ExpressionKind::Block),
variable(),
Expand Down Expand Up @@ -1248,7 +1320,6 @@ fn field_name() -> impl NoirParser<Ident> {
fn constructor(expr_parser: impl ExprParser) -> impl NoirParser<ExpressionKind> {
let args = constructor_field(expr_parser)
.separated_by(just(Token::Comma))
.at_least(1)
.allow_trailing()
.delimited_by(just(Token::LeftBrace), just(Token::RightBrace));

Expand Down Expand Up @@ -1277,7 +1348,9 @@ fn literal() -> impl NoirParser<ExpressionKind> {
})
}

fn literal_or_collection(expr_parser: impl ExprParser) -> impl NoirParser<ExpressionKind> {
fn literal_or_collection<'a>(
expr_parser: impl ExprParser + 'a,
) -> impl NoirParser<ExpressionKind> + 'a {
choice((literal(), constructor(expr_parser.clone()), array_expr(expr_parser)))
}

Expand Down Expand Up @@ -1389,10 +1462,13 @@ mod test {
#[test]
fn parse_cast() {
parse_all(
atom_or_right_unary(expression()),
atom_or_right_unary(expression(), expression_no_constructors(), true),
vec!["x as u8", "0 as Field", "(x + 3) as [Field; 8]"],
);
parse_all_failing(atom_or_right_unary(expression()), vec!["x as pub u8"]);
parse_all_failing(
atom_or_right_unary(expression(), expression_no_constructors(), true),
vec!["x as pub u8"],
);
}

#[test]
Expand All @@ -1404,7 +1480,7 @@ mod test {
"baz[bar]",
"foo.bar[3] as Field .baz as i32 [7]",
];
parse_all(atom_or_right_unary(expression()), valid);
parse_all(atom_or_right_unary(expression(), expression_no_constructors(), true), valid);
}

fn expr_to_array(expr: ExpressionKind) -> ArrayLiteral {
Expand Down Expand Up @@ -1577,12 +1653,12 @@ mod test {
#[test]
fn parse_for_loop() {
parse_all(
for_expr(expression()),
for_expr(expression(), expression_no_constructors()),
vec!["for i in x+y..z {}", "for i in 0..100 { foo; bar }"],
);

parse_all_failing(
for_expr(expression()),
for_expr(expression(), expression_no_constructors()),
vec![
"for 1 in x+y..z {}", // Cannot have a literal as the loop identifier
"for i in 0...100 {}", // Only '..' is supported, there are no inclusive ranges yet
Expand Down Expand Up @@ -1613,8 +1689,14 @@ mod test {

#[test]
fn parse_parenthesized_expression() {
parse_all(atom(expression()), vec!["(0)", "(x+a)", "({(({{({(nested)})}}))})"]);
parse_all_failing(atom(expression()), vec!["(x+a", "((x+a)", "(,)"]);
parse_all(
atom(expression(), expression_no_constructors(), true),
vec!["(0)", "(x+a)", "({(({{({(nested)})}}))})"],
);
parse_all_failing(
atom(expression(), expression_no_constructors(), true),
vec!["(x+a", "((x+a)", "(,)"],
);
}

#[test]
Expand All @@ -1625,12 +1707,12 @@ mod test {
#[test]
fn parse_if_expr() {
parse_all(
if_expr(expression()),
if_expr(expression(), expression_no_constructors()),
vec!["if x + a { } else { }", "if x {}", "if x {} else if y {} else {}"],
);

parse_all_failing(
if_expr(expression()),
if_expr(expression(), expression_no_constructors()),
vec!["if (x / a) + 1 {} else", "if foo then 1 else 2", "if true { 1 }else 3"],
);
}
Expand Down Expand Up @@ -1723,8 +1805,14 @@ mod test {

#[test]
fn parse_unary() {
parse_all(term(expression()), vec!["!hello", "-hello", "--hello", "-!hello", "!-hello"]);
parse_all_failing(term(expression()), vec!["+hello", "/hello"]);
parse_all(
term(expression(), expression_no_constructors(), true),
vec!["!hello", "-hello", "--hello", "-!hello", "!-hello"],
);
parse_all_failing(
term(expression(), expression_no_constructors(), true),
vec!["+hello", "/hello"],
);
}

#[test]
Expand Down Expand Up @@ -1786,12 +1874,7 @@ mod test {
];
parse_all(expression(), cases);

// TODO: Constructor expressions with no fields are currently
// disallowed, they conflict with block syntax in some cases. Namely:
// if a + b {}
// for i in 0..a { }
// https://github.com/noir-lang/noir/issues/152
parse_with(expression(), "Foo {}").unwrap_err();
parse_with(expression(), "Foo { a + b }").unwrap_err();
}

// Semicolons are:
Expand Down

0 comments on commit 6d3029a

Please sign in to comment.