Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(parser)!: deprecate constrain keyword for assert #1286

Merged
merged 7 commits into from
May 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions crates/noirc_frontend/src/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,11 @@ use noirc_errors::Span;
pub use statement::*;
pub use structure::*;

use crate::{parser::ParserError, token::IntType, BinaryTypeOperator, CompTime};
use crate::{
parser::{ParserError, ParserErrorReason},
token::IntType,
BinaryTypeOperator, CompTime,
};
use iter_extended::vecmap;

/// The parser parses types as 'UnresolvedType's which
Expand Down Expand Up @@ -152,9 +156,9 @@ impl UnresolvedTypeExpression {
expr: Expression,
span: Span,
) -> Result<UnresolvedTypeExpression, ParserError> {
Self::from_expr_helper(expr).map_err(|err| {
Self::from_expr_helper(expr).map_err(|err_expr| {
ParserError::with_reason(
format!("Expression is invalid in an array-length type: '{err}'. Only unsigned integer constants, globals, generics, +, -, *, /, and % may be used in this context."),
ParserErrorReason::InvalidArrayLengthExpression(err_expr),
span,
)
})
Expand Down
14 changes: 9 additions & 5 deletions crates/noirc_frontend/src/ast/statement.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::fmt::Display;

use crate::lexer::token::SpannedToken;
use crate::parser::ParserError;
use crate::parser::{ParserError, ParserErrorReason};
use crate::token::Token;
use crate::{Expression, ExpressionKind, IndexExpression, MemberAccessExpression, UnresolvedType};
use iter_extended::vecmap;
Expand Down Expand Up @@ -59,8 +59,10 @@ impl Statement {
| Statement::Error => {
// To match rust, statements always require a semicolon, even at the end of a block
if semi.is_none() {
let reason = "Expected a ; separating these two statements".to_string();
emit_error(ParserError::with_reason(reason, span));
emit_error(ParserError::with_reason(
ParserErrorReason::MissingSeparatingSemi,
span,
));
}
self
}
Expand All @@ -83,8 +85,10 @@ impl Statement {
// for unneeded expressions like { 1 + 2; 3 }
(_, Some(_), false) => Statement::Expression(expr),
(_, None, false) => {
let reason = "Expected a ; separating these two statements".to_string();
emit_error(ParserError::with_reason(reason, span));
emit_error(ParserError::with_reason(
ParserErrorReason::MissingSeparatingSemi,
span,
));
Statement::Expression(expr)
}

Expand Down
45 changes: 31 additions & 14 deletions crates/noirc_frontend/src/parser/errors.rs
Original file line number Diff line number Diff line change
@@ -1,18 +1,33 @@
use std::collections::BTreeSet;

use crate::lexer::token::Token;
use crate::BinaryOp;
use crate::Expression;
use thiserror::Error;

use iter_extended::vecmap;
use noirc_errors::CustomDiagnostic as Diagnostic;
use noirc_errors::Span;

#[derive(Debug, Clone, PartialEq, Eq, Error)]
pub enum ParserErrorReason {
#[error("Arrays must have at least one element")]
ZeroSizedArray,
#[error("Unexpected '{0}', expected a field name")]
ExpectedFieldName(Token),
#[error("Expected a ; separating these two statements")]
MissingSeparatingSemi,
#[error("constrain keyword is deprecated")]
ConstrainDeprecated,
#[error("Expression is invalid in an array-length type: '{0}'. Only unsigned integer constants, globals, generics, +, -, *, /, and % may be used in this context.")]
InvalidArrayLengthExpression(Expression),
}

#[derive(Debug, Clone, PartialEq, Eq)]
pub struct ParserError {
expected_tokens: BTreeSet<Token>,
expected_labels: BTreeSet<String>,
found: Token,
reason: Option<String>,
reason: Option<ParserErrorReason>,
span: Span,
}

Expand All @@ -39,21 +54,11 @@ impl ParserError {
error
}

pub fn with_reason(reason: String, span: Span) -> ParserError {
pub fn with_reason(reason: ParserErrorReason, span: Span) -> ParserError {
let mut error = ParserError::empty(Token::EOF, span);
error.reason = Some(reason);
error
}

pub fn invalid_constrain_operator(operator: BinaryOp) -> ParserError {
let message = format!(
"Cannot use the {} operator in a constraint statement.",
operator.contents.as_string()
);
let mut error = ParserError::empty(operator.contents.as_token(), operator.span());
error.reason = Some(message);
error
}
}

impl std::fmt::Display for ParserError {
Expand Down Expand Up @@ -84,7 +89,19 @@ impl std::fmt::Display for ParserError {
impl From<ParserError> for Diagnostic {
fn from(error: ParserError) -> Diagnostic {
match &error.reason {
Some(reason) => Diagnostic::simple_error(reason.clone(), String::new(), error.span),
Some(reason) => {
match reason {
ParserErrorReason::ConstrainDeprecated => Diagnostic::simple_warning(
"Use of deprecated keyword 'constrain'".into(),
"The 'constrain' keyword has been deprecated. Please use the 'assert' function instead.".into(),
error.span,
),
other => {

Diagnostic::simple_error(format!("{other}"), String::new(), error.span)
}
}
}
None => {
let primary = error.to_string();
Diagnostic::simple_error(primary, String::new(), error.span)
Expand Down
3 changes: 2 additions & 1 deletion crates/noirc_frontend/src/parser/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ use acvm::FieldElement;
use chumsky::prelude::*;
use chumsky::primitive::Container;
pub use errors::ParserError;
pub use errors::ParserErrorReason;
use noirc_errors::Span;
pub use parser::parse_program;

Expand Down Expand Up @@ -176,7 +177,7 @@ where
.try_map(move |peek, span| {
if too_far.get_iter().any(|t| t == peek) {
// This error will never be shown to the user
Err(ParserError::with_reason(String::new(), span))
Err(ParserError::empty(Token::EOF, span))
} else {
Ok(Recoverable::error(span))
}
Expand Down
33 changes: 20 additions & 13 deletions crates/noirc_frontend/src/parser/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
use super::{
foldl_with_span, parameter_name_recovery, parameter_recovery, parenthesized, then_commit,
then_commit_ignore, top_level_statement_recovery, ExprParser, ForRange, NoirParser,
ParsedModule, ParserError, Precedence, SubModule, TopLevelStatement,
ParsedModule, ParserError, ParserErrorReason, Precedence, SubModule, TopLevelStatement,
};
use crate::ast::{Expression, ExpressionKind, LetStatement, Statement, UnresolvedType};
use crate::lexer::Lexer;
Expand Down Expand Up @@ -448,6 +448,10 @@ where
{
ignore_then_commit(keyword(Keyword::Constrain).labelled("statement"), expr_parser)
.map(|expr| Statement::Constrain(ConstrainStatement(expr)))
.validate(|expr, span, emit| {
emit(ParserError::with_reason(ParserErrorReason::ConstrainDeprecated, span));
expr
})
}

fn assertion<'a, P>(expr_parser: P) -> impl NoirParser<Statement> + 'a
Expand Down Expand Up @@ -877,10 +881,7 @@ where
.delimited_by(just(Token::LeftBracket), just(Token::RightBracket))
.validate(|elements, span, emit| {
if elements.is_empty() {
emit(ParserError::with_reason(
"Arrays must have at least one element".to_owned(),
span,
));
emit(ParserError::with_reason(ParserErrorReason::ZeroSizedArray, span));
}
ExpressionKind::array(elements)
})
Expand Down Expand Up @@ -966,8 +967,7 @@ fn field_name() -> impl NoirParser<Ident> {
ident().or(token_kind(TokenKind::Literal).validate(|token, span, emit| match token {
Token::Int(_) => Ident::from(Spanned::from(span, token.to_string())),
other => {
let reason = format!("Unexpected '{other}', expected a field name");
emit(ParserError::with_reason(reason, span));
emit(ParserError::with_reason(ParserErrorReason::ExpectedFieldName(other), span));
Ident::error(span)
}
}))
Expand Down Expand Up @@ -1196,10 +1196,12 @@ mod test {
);
}

/// This is the standard way to declare a constrain statement
/// Deprecated constrain usage test
#[test]
fn parse_constrain() {
parse_with(constrain(expression()), "constrain x == y").unwrap();
let errors = parse_with(constrain(expression()), "constrain x == y").unwrap_err();
assert_eq!(errors.len(), 1);
assert!(format!("{}", errors.first().unwrap()).contains("deprecated"));

// Currently we disallow constrain statements where the outer infix operator
// produces a value. This would require an implicit `==` which
Expand All @@ -1217,7 +1219,9 @@ mod test {

for operator in disallowed_operators {
let src = format!("constrain x {} y;", operator.as_string());
parse_with(constrain(expression()), &src).unwrap_err();
let errors = parse_with(constrain(expression()), &src).unwrap_err();
assert_eq!(errors.len(), 2);
assert!(format!("{}", errors.first().unwrap()).contains("deprecated"));
}

// These are general cases which should always work.
Expand All @@ -1226,7 +1230,7 @@ mod test {
// The first (inner) `==` is a predicate which returns 0/1
// The outer layer is an infix `==` which is
// associated with the Constrain statement
parse_all(
let errors = parse_all_failing(
constrain(expression()),
vec![
"constrain ((x + y) == k) + z == y",
Expand All @@ -1236,8 +1240,11 @@ mod test {
"constrain x + x ^ x == y | m",
],
);
assert_eq!(errors.len(), 5);
assert!(errors.iter().all(|err| { format!("{}", err).contains("deprecated") }));
}

/// This is the standard way to declare an assert statement
#[test]
fn parse_assert() {
parse_with(assertion(expression()), "assert(x == y)").unwrap();
Expand Down Expand Up @@ -1533,9 +1540,9 @@ mod test {
("let = ", 2, "let $error: unspecified = Error"),
("let", 3, "let $error: unspecified = Error"),
("foo = one two three", 1, "foo = plain::one"),
("constrain", 1, "constrain Error"),
("constrain", 2, "constrain Error"),
("assert", 1, "constrain Error"),
("constrain x ==", 1, "constrain (plain::x == Error)"),
("constrain x ==", 2, "constrain (plain::x == Error)"),
("assert(x ==)", 1, "constrain (plain::x == Error)"),
];

Expand Down