Skip to content

Commit

Permalink
wgsl-in: Improve assignment diagnostics (#2056)
Browse files Browse the repository at this point in the history 
Fixes #2052.

* improved assignment statement errors

* hint immutable binding

* fix clippy

* add diagnostic tests

* cleanup formatting
  • Loading branch information
@SparkyPotato
SparkyPotato authored Sep 15, 2022
1 parent 4f8db99 commit 5166c2a
Show file tree
Hide file tree
Showing 3 changed files with 165 additions and 31 deletions.
6 changes: 3 additions & 3 deletions src/front/wgsl/lexer.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ impl<'a> Lexer<'a> {
if next.0 == expected {
Ok(next.1)
} else {
Err(Error::Unexpected(next, ExpectedToken::Token(expected)))
Err(Error::Unexpected(next.1, ExpectedToken::Token(expected)))
}
}

Expand All @@ -288,7 +288,7 @@ impl<'a> Lexer<'a> {
Ok(())
} else {
Err(Error::Unexpected(
next,
next.1,
ExpectedToken::Token(Token::Paren(expected)),
))
}
Expand All @@ -314,7 +314,7 @@ impl<'a> Lexer<'a> {
Err(Error::ReservedIdentifierPrefix(span))
}
(Token::Word(word), span) => Ok((word, span)),
other => Err(Error::Unexpected(other, ExpectedToken::Identifier)),
other => Err(Error::Unexpected(other.1, ExpectedToken::Identifier)),
}
}

Expand Down
110 changes: 82 additions & 28 deletions src/front/wgsl/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ pub enum ExpectedToken<'a> {
Constant,
/// Expected: constant, parenthesized expression, identifier
PrimaryExpression,
/// Expected: assignment, increment/decrement expression
Assignment,
/// Expected: '}', identifier
FieldName,
/// Expected: attribute for a type
Expand All @@ -95,9 +97,16 @@ pub enum NumberError {
UnimplementedF16,
}

#[derive(Copy, Clone, Debug, PartialEq)]
pub enum InvalidAssignmentType {
Other,
Swizzle,
ImmutableBinding,
}

#[derive(Clone, Debug)]
pub enum Error<'a> {
Unexpected(TokenSpan<'a>, ExpectedToken<'a>),
Unexpected(Span, ExpectedToken<'a>),
UnexpectedComponents(Span),
BadNumber(Span, NumberError),
/// A negative signed integer literal where both signed and unsigned,
Expand Down Expand Up @@ -151,6 +160,10 @@ pub enum Error<'a> {
Pointer(&'static str, Span),
NotPointer(Span),
NotReference(&'static str, Span),
InvalidAssignment {
span: Span,
ty: InvalidAssignmentType,
},
ReservedKeyword(Span),
Redefinition {
previous: Span,
Expand All @@ -162,7 +175,7 @@ pub enum Error<'a> {
impl<'a> Error<'a> {
fn as_parse_error(&self, source: &'a str) -> ParseError {
match *self {
Error::Unexpected((_, ref unexpected_span), expected) => {
Error::Unexpected(ref unexpected_span, expected) => {
let expected_str = match expected {
ExpectedToken::Token(token) => {
match token {
Expand Down Expand Up @@ -195,6 +208,7 @@ impl<'a> Error<'a> {
ExpectedToken::Integer => "unsigned/signed integer literal".to_string(),
ExpectedToken::Constant => "constant".to_string(),
ExpectedToken::PrimaryExpression => "expression".to_string(),
ExpectedToken::Assignment => "assignment or increment/decrement".to_string(),
ExpectedToken::FieldName => "field name or a closing curly bracket to signify the end of the struct".to_string(),
ExpectedToken::TypeAttribute => "type attribute".to_string(),
ExpectedToken::Statement => "statement".to_string(),
Expand Down Expand Up @@ -439,6 +453,20 @@ impl<'a> Error<'a> {
labels: vec![(span.clone(), "expression is not a reference".into())],
notes: vec![],
},
Error::InvalidAssignment{ ref span, ty} => ParseError {
message: "invalid left-hand side of assignment".into(),
labels: vec![(span.clone(), "cannot assign to this expression".into())],
notes: match ty {
InvalidAssignmentType::Swizzle => vec![
"WGSL does not support assignments to swizzles".into(),
"consider assigning each component individually".into()
],
InvalidAssignmentType::ImmutableBinding => vec![
format!("'{}' is an immutable binding", &source[span.clone()]), "consider declaring it with `var` instead of `let`".into()
],
InvalidAssignmentType::Other => vec![],
},
},
Error::Pointer(what, ref span) => ParseError {
message: format!("{} must not be a pointer", what),
labels: vec![(span.clone(), "expression is a pointer".into())],
Expand Down Expand Up @@ -1409,7 +1437,7 @@ impl Parser {
Token::Number(Ok(Number::U32(num))) if uint => Ok(num as i32),
Token::Number(Ok(Number::I32(num))) if !uint => Ok(num),
Token::Number(Err(e)) => Err(Error::BadNumber(token_span.1, e)),
_ => Err(Error::Unexpected(token_span, ExpectedToken::Integer)),
_ => Err(Error::Unexpected(token_span.1, ExpectedToken::Integer)),
}
}

Expand All @@ -1422,7 +1450,7 @@ impl Parser {
}
(Token::Number(Err(e)), span) => Err(Error::BadNumber(span, e)),
other => Err(Error::Unexpected(
other,
other.1,
ExpectedToken::Number(NumberType::I32),
)),
}
Expand All @@ -1439,7 +1467,7 @@ impl Parser {
(Token::Number(Ok(Number::U32(num))), _) => Ok(num),
(Token::Number(Err(e)), span) => Err(Error::BadNumber(span, e)),
other => Err(Error::Unexpected(
other,
other.1,
ExpectedToken::Number(NumberType::I32),
)),
}
Expand Down Expand Up @@ -2239,7 +2267,7 @@ impl Parser {
components,
}
}
other => return Err(Error::Unexpected(other, ExpectedToken::Constant)),
other => return Err(Error::Unexpected(other.1, ExpectedToken::Constant)),
};

// Only set span if it's a named constant. Otherwise, the enclosing Expression should have
Expand Down Expand Up @@ -2330,7 +2358,7 @@ impl Parser {
}
}
}
other => return Err(Error::Unexpected(other, ExpectedToken::PrimaryExpression)),
other => return Err(Error::Unexpected(other.1, ExpectedToken::PrimaryExpression)),
};
Ok(expr)
}
Expand Down Expand Up @@ -2831,7 +2859,7 @@ impl Parser {
while !lexer.skip(Token::Paren('}')) {
if !ready {
return Err(Error::Unexpected(
lexer.next(),
lexer.next().1,
ExpectedToken::Token(Token::Separator(',')),
));
}
Expand Down Expand Up @@ -2863,7 +2891,7 @@ impl Parser {

let (name, span) = match lexer.next() {
(Token::Word(word), span) => (word, span),
other => return Err(Error::Unexpected(other, ExpectedToken::FieldName)),
other => return Err(Error::Unexpected(other.1, ExpectedToken::FieldName)),
};
if crate::keywords::wgsl::RESERVED.contains(&name) {
return Err(Error::ReservedKeyword(span));
Expand Down Expand Up @@ -3276,7 +3304,7 @@ impl Parser {

if lexer.skip(Token::Attribute) {
let other = lexer.next();
return Err(Error::Unexpected(other, ExpectedToken::TypeAttribute));
return Err(Error::Unexpected(other.1, ExpectedToken::TypeAttribute));
}

let (name, name_span) = lexer.next_ident_with_span()?;
Expand All @@ -3300,23 +3328,42 @@ impl Parser {
fn parse_assignment_statement<'a, 'out>(
&mut self,
lexer: &mut Lexer<'a>,
mut context: ExpressionContext<'a, '_, 'out>,
mut context: StatementContext<'a, '_, 'out>,
block: &mut crate::Block,
emitter: &mut super::Emitter,
) -> Result<(), Error<'a>> {
use crate::BinaryOperator as Bo;

let span_start = lexer.start_byte_offset();
context.emitter.start(context.expressions);
let reference = self.parse_unary_expression(lexer, context.reborrow())?;
emitter.start(context.expressions);
let (reference, lhs_span) = self
.parse_general_expression_for_reference(lexer, context.as_expression(block, emitter))?;
let op = lexer.next();
// The left hand side of an assignment must be a reference.
let lhs_span = span_start..lexer.end_byte_offset();
if !reference.is_reference {
return Err(Error::NotReference(
"the left-hand side of an assignment",
lhs_span,
));
if !matches!(
op.0,
Token::Operation('=')
| Token::AssignmentOperation(_)
| Token::IncrementOperation
| Token::DecrementOperation
) {
return Err(Error::Unexpected(lhs_span, ExpectedToken::Assignment));
} else if !reference.is_reference {
let ty = if context.named_expressions.contains_key(&reference.handle) {
InvalidAssignmentType::ImmutableBinding
} else {
match *context.expressions.get_mut(reference.handle) {
crate::Expression::Swizzle { .. } => InvalidAssignmentType::Swizzle,
_ => InvalidAssignmentType::Other,
}
};

return Err(Error::InvalidAssignment { span: lhs_span, ty });
}

let value = match lexer.next() {
let mut context = context.as_expression(block, emitter);

let value = match op {
(Token::Operation('='), _) => {
self.parse_general_expression(lexer, context.reborrow())?
}
Expand Down Expand Up @@ -3406,7 +3453,7 @@ impl Parser {
op_span.into(),
)
}
other => return Err(Error::Unexpected(other, ExpectedToken::SwitchItem)),
other => return Err(Error::Unexpected(other.1, ExpectedToken::SwitchItem)),
};

let span_end = lexer.end_byte_offset();
Expand Down Expand Up @@ -3843,7 +3890,10 @@ impl Parser {
}
(Token::Paren('}'), _) => break,
other => {
return Err(Error::Unexpected(other, ExpectedToken::SwitchItem))
return Err(Error::Unexpected(
other.1,
ExpectedToken::SwitchItem,
))
}
}
}
Expand Down Expand Up @@ -3954,7 +4004,9 @@ impl Parser {
}
_ => self.parse_assignment_statement(
lexer,
context.as_expression(&mut continuing, &mut emitter),
context.reborrow(),
&mut continuing,
&mut emitter,
)?,
}
lexer.expect(Token::Paren(')'))?;
Expand Down Expand Up @@ -4059,7 +4111,9 @@ impl Parser {
match context.symbol_table.lookup(ident) {
Some(_) => self.parse_assignment_statement(
lexer,
context.as_expression(block, &mut emitter),
context,
block,
&mut emitter,
)?,
None => self.parse_function_statement(
lexer,
Expand All @@ -4078,7 +4132,7 @@ impl Parser {
}
_ => {
let mut emitter = super::Emitter::default();
self.parse_assignment_statement(lexer, context.as_expression(block, &mut emitter))?;
self.parse_assignment_statement(lexer, context, block, &mut emitter)?;
self.pop_rule_span(lexer);
}
}
Expand Down Expand Up @@ -4262,7 +4316,7 @@ impl Parser {
while !lexer.skip(Token::Paren(')')) {
if !ready {
return Err(Error::Unexpected(
lexer.next(),
lexer.next().1,
ExpectedToken::Token(Token::Separator(',')),
));
}
Expand Down Expand Up @@ -4391,7 +4445,7 @@ impl Parser {
(Token::Separator(','), _) if i != 2 => (),
other => {
return Err(Error::Unexpected(
other,
other.1,
ExpectedToken::WorkgroupSizeSeparator,
))
}
Expand Down Expand Up @@ -4577,7 +4631,7 @@ impl Parser {
}
}
(Token::End, _) => return Ok(false),
other => return Err(Error::Unexpected(other, ExpectedToken::GlobalItem)),
other => return Err(Error::Unexpected(other.1, ExpectedToken::GlobalItem)),
}

match binding {
Expand Down
80 changes: 80 additions & 0 deletions tests/wgsl-errors.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1597,3 +1597,83 @@ fn break_if_bad_condition() {
)
}
}

#[test]
fn swizzle_assignment() {
check(
"
fn f() {
var v = vec2(0);
v.xy = vec2(1);
}
",
r###"error: invalid left-hand side of assignment
┌─ wgsl:4:13
│
4 │ v.xy = vec2(1);
│ ^^^^ cannot assign to this expression
│
= note: WGSL does not support assignments to swizzles
= note: consider assigning each component individually
"###,
);
}

#[test]
fn binary_statement() {
check(
"
fn f() {
3 + 5;
}
",
r###"error: expected assignment or increment/decrement, found '3 + 5'
┌─ wgsl:3:13
│
3 │ 3 + 5;
│ ^^^^^ expected assignment or increment/decrement
"###,
);
}

#[test]
fn assign_to_expr() {
check(
"
fn f() {
3 + 5 = 10;
}
",
r###"error: invalid left-hand side of assignment
┌─ wgsl:3:13
│
3 │ 3 + 5 = 10;
│ ^^^^^ cannot assign to this expression
"###,
);
}

#[test]
fn assign_to_let() {
check(
"
fn f() {
let a = 10;
a = 20;
}
",
r###"error: invalid left-hand side of assignment
┌─ wgsl:4:10
│
4 │ a = 20;
│ ^ cannot assign to this expression
│
= note: 'a' is an immutable binding
= note: consider declaring it with `var` instead of `let`
"###,
);
}

0 comments on commit 5166c2a

Please sign in to comment.