From 5166c2a1d84c3c96ff1ed2b827c313640d1e3fb9 Mon Sep 17 00:00:00 2001 From: Shaye Garg <64652557+SparkyPotato@users.noreply.github.com> Date: Thu, 15 Sep 2022 21:47:00 +0530 Subject: [PATCH] wgsl-in: Improve assignment diagnostics (#2056) Fixes #2052. * improved assignment statement errors * hint immutable binding * fix clippy * add diagnostic tests * cleanup formatting --- src/front/wgsl/lexer.rs | 6 +-- src/front/wgsl/mod.rs | 110 ++++++++++++++++++++++++++++++---------- tests/wgsl-errors.rs | 80 +++++++++++++++++++++++++++++ 3 files changed, 165 insertions(+), 31 deletions(-) diff --git a/src/front/wgsl/lexer.rs b/src/front/wgsl/lexer.rs index 6255d36f26..35fe450892 100644 --- a/src/front/wgsl/lexer.rs +++ b/src/front/wgsl/lexer.rs @@ -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))) } } @@ -288,7 +288,7 @@ impl<'a> Lexer<'a> { Ok(()) } else { Err(Error::Unexpected( - next, + next.1, ExpectedToken::Token(Token::Paren(expected)), )) } @@ -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)), } } diff --git a/src/front/wgsl/mod.rs b/src/front/wgsl/mod.rs index f39f88fd28..df71cfc988 100644 --- a/src/front/wgsl/mod.rs +++ b/src/front/wgsl/mod.rs @@ -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 @@ -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, @@ -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, @@ -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 { @@ -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(), @@ -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())], @@ -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)), } } @@ -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), )), } @@ -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), )), } @@ -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 @@ -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) } @@ -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(',')), )); } @@ -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)); @@ -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()?; @@ -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())? } @@ -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(); @@ -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, + )) } } } @@ -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(')'))?; @@ -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, @@ -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); } } @@ -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(',')), )); } @@ -4391,7 +4445,7 @@ impl Parser { (Token::Separator(','), _) if i != 2 => (), other => { return Err(Error::Unexpected( - other, + other.1, ExpectedToken::WorkgroupSizeSeparator, )) } @@ -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 { diff --git a/tests/wgsl-errors.rs b/tests/wgsl-errors.rs index 13b020ee42..445459f3b2 100644 --- a/tests/wgsl-errors.rs +++ b/tests/wgsl-errors.rs @@ -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` + +"###, + ); +}