Skip to content

Commit

Permalink
Allow the parser to add contextual help text to errors; Add contextua…
Browse files Browse the repository at this point in the history 
…l help text for parenthesized for loops (#1828)

Given the problems users have faced with non-parenthesized for loop
iterators ([like this
one](https://stackoverflow.com/questions/78592805/q-sharp-syntax-error-in-quantum-hhl-algorithm)),
we wanted to provide some help text for the user in this situation.

Instead of just patching in a fix for precisely this situation, I wanted
to use `miette`'s great functionality for composable errors. So, with
this change, can now compose any parse error with help text.

I also manually implemented `Debug` for `Error` to omit the help text if
it isn't present. This is primarily so this PR doesn't add `None` to
every single parser expect test which tests errors. (Happy to revert
this and update the tests if desired, but I really don't like spraying
the diff with 1k lines of expect-test changes that just say `None`)
  • Loading branch information
@sezna
sezna authored Aug 12, 2024
1 parent 4de42e1 commit 7df2d20
Show file tree
Hide file tree
Showing 9 changed files with 236 additions and 51 deletions.
35 changes: 25 additions & 10 deletions compiler/qsc_parse/src/expr.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,22 @@ fn expr_base(s: &mut ParserContext) -> Result<Box<Expr>> {
} else if token(s, TokenKind::Keyword(Keyword::Fail)).is_ok() {
Ok(Box::new(ExprKind::Fail(expr(s)?)))
} else if token(s, TokenKind::Keyword(Keyword::For)).is_ok() {
let vars = pat(s)?;
let peeked = s.peek();
let vars = match pat(s) {
Ok(o) => o,
Err(e) => {
if let (
TokenKind::Open(Delim::Paren),
ErrorKind::Token(_, TokenKind::Keyword(Keyword::In), _),
) = (peeked.kind, &e.0)
{
return Err(
e.with_help("parenthesis are not permitted around for-loop iterations")
);
}
return Err(e);
}
};
token(s, TokenKind::Keyword(Keyword::In))?;
let iter = expr(s)?;
let body = stmt::parse_block(s)?;
Expand Down Expand Up @@ -209,7 +224,7 @@ fn expr_base(s: &mut ParserContext) -> Result<Box<Expr>> {
} else if let Some(p) = opt(s, single_ident_path)? {
Ok(Box::new(ExprKind::Path(p)))
} else {
Err(Error(ErrorKind::Rule(
Err(Error::new(ErrorKind::Rule(
"expression",
s.peek().kind,
s.peek().span,
Expand Down Expand Up @@ -294,7 +309,7 @@ fn expr_set(s: &mut ParserContext) -> Result<Box<ExprKind>> {
let rhs = expr(s)?;
Ok(Box::new(ExprKind::AssignOp(closed_bin_op(op), lhs, rhs)))
} else {
Err(Error(ErrorKind::Rule(
Err(Error::new(ErrorKind::Rule(
"assignment operator",
s.peek().kind,
s.peek().span,
Expand Down Expand Up @@ -355,7 +370,7 @@ fn expr_interpolate(s: &mut ParserContext) -> Result<Vec<StringComponent>> {
let TokenKind::String(StringToken::Interpolated(InterpolatedStart::DollarQuote, mut end)) =
token.kind
else {
return Err(Error(ErrorKind::Rule(
return Err(Error::new(ErrorKind::Rule(
"interpolated string",
token.kind,
token.span,
Expand All @@ -376,7 +391,7 @@ fn expr_interpolate(s: &mut ParserContext) -> Result<Vec<StringComponent>> {
let TokenKind::String(StringToken::Interpolated(InterpolatedStart::RBrace, next_end)) =
token.kind
else {
return Err(Error(ErrorKind::Rule(
return Err(Error::new(ErrorKind::Rule(
"interpolated string",
token.kind,
token.span,
Expand Down Expand Up @@ -419,20 +434,20 @@ fn lit_token(lexeme: &str, token: Token) -> Result<Option<Lit>> {
let offset = if radix == Radix::Decimal { 0 } else { 2 };
let lexeme = &lexeme[offset..lexeme.len() - 1]; // Slice off prefix and suffix.
let value = BigInt::from_str_radix(lexeme, radix.into())
.map_err(|_| Error(ErrorKind::Lit("big-integer", token.span)))?;
.map_err(|_| Error::new(ErrorKind::Lit("big-integer", token.span)))?;
Ok(Some(Lit::BigInt(Box::new(value))))
}
TokenKind::Float => {
let lexeme = lexeme.replace('_', "");
let value = lexeme
.parse()
.map_err(|_| Error(ErrorKind::Lit("floating-point", token.span)))?;
.map_err(|_| Error::new(ErrorKind::Lit("floating-point", token.span)))?;
Ok(Some(Lit::Double(value)))
}
TokenKind::Int(radix) => {
let offset = if radix == Radix::Decimal { 0 } else { 2 };
let value = lit_int(&lexeme[offset..], radix.into())
.ok_or(Error(ErrorKind::Lit("integer", token.span)))?;
.ok_or(Error::new(ErrorKind::Lit("integer", token.span)))?;
Ok(Some(Lit::Int(value)))
}
TokenKind::String(StringToken::Normal) => {
Expand All @@ -445,7 +460,7 @@ fn lit_token(lexeme: &str, token: Token) -> Result<Option<Lit>> {
let index: u32 = index.try_into().expect("index should fit into u32");
let lo = token.span.lo + index + 2;
let span = Span { lo, hi: lo + 1 };
Error(ErrorKind::Escape(ch, span))
Error::new(ErrorKind::Escape(ch, span))
})?;
Ok(Some(Lit::String(string.into())))
}
Expand Down Expand Up @@ -759,7 +774,7 @@ fn expr_as_pat(expr: Expr) -> Result<Box<Pat>> {
.collect::<Result<_>>()?;
Ok(PatKind::Tuple(pats))
}
_ => Err(Error(ErrorKind::Convert(
_ => Err(Error::new(ErrorKind::Convert(
"pattern",
"expression",
expr.span,
Expand Down
80 changes: 80 additions & 0 deletions compiler/qsc_parse/src/expr/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2521,3 +2521,83 @@ fn invalid_initial_commas_in_pattern() {
]"#]],
);
}

#[test]
fn friendly_error_on_parenthesized_for() {
check(
expr,
"for (x in xs) { () }",
&expect![[r#"
Error(
Token(
Close(
Paren,
),
Keyword(
In,
),
Span {
lo: 7,
hi: 9,
},
),
Some(
"parenthesis are not permitted around for-loop iterations",
),
)
"#]],
);
}

#[test]
fn no_help_text_on_non_parenthesized_for() {
check(
expr,
"for x in invalid syntax { () }",
&expect![[r#"
Error(
Token(
Open(
Brace,
),
Ident,
Span {
lo: 17,
hi: 23,
},
),
)
"#]],
);
}

#[test]
fn parenthesized_pat_in_for_is_valid() {
check(
expr,
"for (x) in xs { () }",
&expect![[r#"
Expr _id_ [0-20]: For:
Pat _id_ [4-7]: Paren:
Pat _id_ [5-6]: Bind:
Ident _id_ [5-6] "x"
Expr _id_ [11-13]: Path: Path _id_ [11-13] (Ident _id_ [11-13] "xs")
Block _id_ [14-20]:
Stmt _id_ [16-18]: Expr: Expr _id_ [16-18]: Unit"#]],
);
}

#[test]
fn parenthesized_expr_in_for_is_valid() {
check(
expr,
"for x in (xs) { () }",
&expect![[r#"
Expr _id_ [0-20]: For:
Pat _id_ [4-5]: Bind:
Ident _id_ [4-5] "x"
Expr _id_ [9-13]: Paren: Expr _id_ [10-12]: Path: Path _id_ [10-12] (Ident _id_ [10-12] "xs")
Block _id_ [14-20]:
Stmt _id_ [16-18]: Expr: Expr _id_ [16-18]: Unit"#]],
);
}
52 changes: 34 additions & 18 deletions compiler/qsc_parse/src/item.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,19 +51,19 @@ pub(super) fn parse(s: &mut ParserContext) -> Result<Box<Item>> {
Box::new(ItemKind::ImportOrExport(decl))
} else if visibility.is_some() {
let err_item = default(s.span(lo));
s.push_error(Error(ErrorKind::FloatingVisibility(err_item.span)));
s.push_error(Error::new(ErrorKind::FloatingVisibility(err_item.span)));
return Ok(err_item);
} else if !attrs.is_empty() {
let err_item = default(s.span(lo));
s.push_error(Error(ErrorKind::FloatingAttr(err_item.span)));
s.push_error(Error::new(ErrorKind::FloatingAttr(err_item.span)));
return Ok(err_item);
} else if doc.is_some() {
let err_item = default(s.span(lo));
s.push_error(Error(ErrorKind::FloatingDocComment(err_item.span)));
s.push_error(Error::new(ErrorKind::FloatingDocComment(err_item.span)));
return Ok(err_item);
} else {
let p = s.peek();
return Err(Error(ErrorKind::Rule("item", p.kind, p.span)));
return Err(Error::new(ErrorKind::Rule("item", p.kind, p.span)));
};

Ok(Box::new(Item {
Expand Down Expand Up @@ -133,7 +133,7 @@ fn parse_top_level_node(s: &mut ParserContext) -> Result<TopLevelNode> {
if let StmtKind::Item(item) = &mut *stmt.kind {
item.doc = doc.into();
} else if !doc.is_empty() {
return Err(Error(ErrorKind::Rule("item", kind, span)));
return Err(Error::new(ErrorKind::Rule("item", kind, span)));
}
Ok(TopLevelNode::Stmt(stmt))
}
Expand All @@ -153,7 +153,10 @@ pub fn parse_implicit_namespace(source_name: &str, s: &mut ParserContext) -> Res
let items = parse_namespace_block_contents(s)?;
recovering_token(s, TokenKind::Eof);
if items.is_empty() || s.peek().kind != TokenKind::Eof {
return Err(Error(ErrorKind::ExpectedItem(s.peek().kind, s.span(lo))));
return Err(Error::new(ErrorKind::ExpectedItem(
s.peek().kind,
s.span(lo),
)));
}
let span = s.span(lo);
let namespace_name = source_name_to_namespace_name(source_name, span)?;
Expand All @@ -177,7 +180,7 @@ fn source_name_to_namespace_name(raw: &str, span: Span) -> Result<Idents> {
match component {
std::path::Component::Normal(name) => {
// strip the extension off, if there is one
let mut name = name.to_str().ok_or(Error(ErrorKind::InvalidFileName(
let mut name = name.to_str().ok_or(Error::new(ErrorKind::InvalidFileName(
span,
name.to_string_lossy().to_string(),
)))?;
Expand All @@ -192,7 +195,12 @@ fn source_name_to_namespace_name(raw: &str, span: Span) -> Result<Idents> {

namespace.push(ident);
}
_ => return Err(Error(ErrorKind::InvalidFileName(span, raw.to_string()))),
_ => {
return Err(Error::new(ErrorKind::InvalidFileName(
span,
raw.to_string(),
)))
}
}
}

Expand All @@ -216,9 +224,9 @@ fn validate_namespace_name(error_span: Span, name: &str) -> Result<Ident> {
// we just directly use the ident parser here instead of trying to recreate
// validation rules
let ident = ident(&mut s)
.map_err(|_| Error(ErrorKind::InvalidFileName(error_span, name.to_string())))?;
.map_err(|_| Error::new(ErrorKind::InvalidFileName(error_span, name.to_string())))?;
if s.peek().kind != TokenKind::Eof {
return Err(Error(ErrorKind::InvalidFileName(
return Err(Error::new(ErrorKind::InvalidFileName(
error_span,
name.to_string(),
)));
Expand Down Expand Up @@ -311,7 +319,7 @@ fn parse_open(s: &mut ParserContext) -> Result<Box<ItemKind>> {
// Peek to see if the next token is a dot -- this means it is likely a dot ident alias, and
// we want to provide a more helpful error message
if s.peek().kind == TokenKind::Dot {
return Err(Error(ErrorKind::DotIdentAlias(s.peek().span)));
return Err(Error::new(ErrorKind::DotIdentAlias(s.peek().span)));
}

token(s, TokenKind::Semi)?;
Expand Down Expand Up @@ -410,7 +418,11 @@ fn parse_ty_def(s: &mut ParserContext) -> Result<Box<TyDef>> {

fn ty_as_ident(ty: Ty) -> Result<Box<Ident>> {
let TyKind::Path(path) = *ty.kind else {
return Err(Error(ErrorKind::Convert("identifier", "type", ty.span)));
return Err(Error::new(ErrorKind::Convert(
"identifier",
"type",
ty.span,
)));
};
if let Path {
segments: None,
Expand All @@ -420,7 +432,11 @@ fn ty_as_ident(ty: Ty) -> Result<Box<Ident>> {
{
Ok(name)
} else {
Err(Error(ErrorKind::Convert("identifier", "type", ty.span)))
Err(Error::new(ErrorKind::Convert(
"identifier",
"type",
ty.span,
)))
}
}

Expand All @@ -433,7 +449,7 @@ fn parse_callable_decl(s: &mut ParserContext) -> Result<Box<CallableDecl>> {
CallableKind::Operation
} else {
let token = s.peek();
return Err(Error(ErrorKind::Rule(
return Err(Error::new(ErrorKind::Rule(
"callable declaration",
token.kind,
token.span,
Expand Down Expand Up @@ -510,7 +526,7 @@ fn parse_spec_decl(s: &mut ParserContext) -> Result<Box<SpecDecl>> {
Spec::Ctl
}
} else {
return Err(Error(ErrorKind::Rule(
return Err(Error::new(ErrorKind::Rule(
"specialization",
s.peek().kind,
s.peek().span,
Expand Down Expand Up @@ -544,7 +560,7 @@ fn parse_spec_gen(s: &mut ParserContext) -> Result<SpecGen> {
} else if token(s, TokenKind::Keyword(Keyword::Slf)).is_ok() {
Ok(SpecGen::Slf)
} else {
Err(Error(ErrorKind::Rule(
Err(Error::new(ErrorKind::Rule(
"specialization generator",
s.peek().kind,
s.peek().span,
Expand All @@ -557,7 +573,7 @@ pub(super) fn check_input_parens(inputs: &Pat) -> Result<()> {
if matches!(*inputs.kind, PatKind::Paren(_) | PatKind::Tuple(_)) {
Ok(())
} else {
Err(Error(ErrorKind::MissingParens(inputs.span)))
Err(Error::new(ErrorKind::MissingParens(inputs.span)))
}
}

Expand All @@ -578,7 +594,7 @@ fn parse_import_or_export(s: &mut ParserContext) -> Result<ImportOrExportDecl> {
TokenKind::Keyword(Keyword::Export) => true,
TokenKind::Keyword(Keyword::Import) => false,
_ => {
return Err(Error(ErrorKind::Rule(
return Err(Error::new(ErrorKind::Rule(
"import or export",
s.peek().kind,
s.peek().span,
Expand Down
Loading

0 comments on commit 7df2d20

Please sign in to comment.