From 2a8861d20ec425d7391631331089b41bd4b47933 Mon Sep 17 00:00:00 2001 From: Ben Clayton Date: Thu, 16 Jun 2022 08:36:30 +0000 Subject: [PATCH] tint: Rework errors around variable declarations Standarize the messages, using 'let', 'override' or 'var'. Module-scope 'let' needs to be replaced with 'const', but baby steps. Bug: tint:1582 Change-Id: I290aede118a30ab0f4294c89ec43005371c87b45 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/93446 Kokoro: Kokoro Reviewed-by: Dan Sinclair Commit-Queue: Ben Clayton --- src/tint/reader/wgsl/parser_impl.cc | 24 +++++--- .../reader/wgsl/parser_impl_error_msg_test.cc | 14 ++--- .../reader/wgsl/parser_impl_for_stmt_test.cc | 4 +- .../wgsl/parser_impl_global_decl_test.cc | 11 +++- .../reader/wgsl/parser_impl_statement_test.cc | 2 +- .../wgsl/parser_impl_variable_stmt_test.cc | 8 +-- .../resolver/assignment_validation_test.cc | 2 +- .../resolver/attribute_validation_test.cc | 54 +++++++++++++++- .../compound_assignment_validation_test.cc | 2 +- .../increment_decrement_validation_test.cc | 2 +- src/tint/resolver/inferred_type_test.cc | 2 +- src/tint/resolver/resolver.cc | 31 +++++----- .../resolver/storage_class_validation_test.cc | 6 +- src/tint/resolver/type_validation_test.cc | 26 +++++--- src/tint/resolver/validation_test.cc | 6 +- src/tint/resolver/validator.cc | 61 +++++++++---------- src/tint/resolver/var_let_validation_test.cc | 10 ++- 17 files changed, 169 insertions(+), 96 deletions(-) diff --git a/src/tint/reader/wgsl/parser_impl.cc b/src/tint/reader/wgsl/parser_impl.cc index 13194780fc..c5c1b346d0 100644 --- a/src/tint/reader/wgsl/parser_impl.cc +++ b/src/tint/reader/wgsl/parser_impl.cc @@ -433,7 +433,7 @@ Maybe ParserImpl::global_decl() { } if (gc.matched) { - if (!expect("let declaration", Token::Type::kSemicolon)) { + if (!expect("'let' declaration", Token::Type::kSemicolon)) { return Failure::kErrored; } @@ -562,7 +562,7 @@ Maybe ParserImpl::global_constant_decl(ast::AttributeList& bool is_overridable = false; const char* use = nullptr; if (match(Token::Type::kLet)) { - use = "let declaration"; + use = "'let' declaration"; } else if (match(Token::Type::kOverride)) { use = "override declaration"; is_overridable = true; @@ -575,8 +575,18 @@ Maybe ParserImpl::global_constant_decl(ast::AttributeList& return Failure::kErrored; } + bool has_initializer = false; + if (is_overridable) { + has_initializer = match(Token::Type::kEqual); + } else { + if (!expect(use, Token::Type::kEqual)) { + return Failure::kErrored; + } + has_initializer = true; + } + const ast::Expression* initializer = nullptr; - if (match(Token::Type::kEqual)) { + if (has_initializer) { auto init = expect_const_expr(); if (init.errored) { return Failure::kErrored; @@ -1757,13 +1767,13 @@ Maybe ParserImpl::return_stmt() { // | CONST variable_ident_decl EQUAL logical_or_expression Maybe ParserImpl::variable_stmt() { if (match(Token::Type::kLet)) { - auto decl = expect_variable_ident_decl("let declaration", + auto decl = expect_variable_ident_decl("'let' declaration", /*allow_inferred = */ true); if (decl.errored) { return Failure::kErrored; } - if (!expect("let declaration", Token::Type::kEqual)) { + if (!expect("'let' declaration", Token::Type::kEqual)) { return Failure::kErrored; } @@ -1772,7 +1782,7 @@ Maybe ParserImpl::variable_stmt() { return Failure::kErrored; } if (!constructor.matched) { - return add_error(peek(), "missing constructor for let declaration"); + return add_error(peek(), "missing constructor for 'let' declaration"); } auto* var = create(decl->source, // source @@ -1803,7 +1813,7 @@ Maybe ParserImpl::variable_stmt() { return Failure::kErrored; } if (!constructor_expr.matched) { - return add_error(peek(), "missing constructor for variable declaration"); + return add_error(peek(), "missing constructor for 'var' declaration"); } constructor = constructor_expr.value; diff --git a/src/tint/reader/wgsl/parser_impl_error_msg_test.cc b/src/tint/reader/wgsl/parser_impl_error_msg_test.cc index 972e118c0e..fc141c0c7a 100644 --- a/src/tint/reader/wgsl/parser_impl_error_msg_test.cc +++ b/src/tint/reader/wgsl/parser_impl_error_msg_test.cc @@ -204,7 +204,7 @@ fn f() { x = vec2(1,2; } TEST_F(ParserImplErrorTest, ConstVarStmtInvalid) { EXPECT("fn f() { let >; }", - R"(test.wgsl:1:14 error: expected identifier for let declaration + R"(test.wgsl:1:14 error: expected identifier for 'let' declaration fn f() { let >; } ^ )"); @@ -212,7 +212,7 @@ fn f() { let >; } TEST_F(ParserImplErrorTest, ConstVarStmtMissingAssignment) { EXPECT("fn f() { let a : i32; }", - R"(test.wgsl:1:21 error: expected '=' for let declaration + R"(test.wgsl:1:21 error: expected '=' for 'let' declaration fn f() { let a : i32; } ^ )"); @@ -220,7 +220,7 @@ fn f() { let a : i32; } TEST_F(ParserImplErrorTest, ConstVarStmtMissingConstructor) { EXPECT("fn f() { let a : i32 = >; }", - R"(test.wgsl:1:24 error: missing constructor for let declaration + R"(test.wgsl:1:24 error: missing constructor for 'let' declaration fn f() { let a : i32 = >; } ^ )"); @@ -472,7 +472,7 @@ test.wgsl:3:3 error: statement found outside of function body TEST_F(ParserImplErrorTest, GlobalDeclConstInvalidIdentifier) { EXPECT("let ^ : i32 = 1;", - R"(test.wgsl:1:5 error: expected identifier for let declaration + R"(test.wgsl:1:5 error: expected identifier for 'let' declaration let ^ : i32 = 1; ^ )"); @@ -480,7 +480,7 @@ let ^ : i32 = 1; TEST_F(ParserImplErrorTest, GlobalDeclConstMissingSemicolon) { EXPECT("let i : i32 = 1", - R"(test.wgsl:1:16 error: expected ';' for let declaration + R"(test.wgsl:1:16 error: expected ';' for 'let' declaration let i : i32 = 1 ^ )"); @@ -512,7 +512,7 @@ let i : vec2 = vec2(!); TEST_F(ParserImplErrorTest, GlobalDeclConstBadConstLiteralSpaceLessThan) { EXPECT("let i = 1 < 2;", - R"(test.wgsl:1:11 error: expected ';' for let declaration + R"(test.wgsl:1:11 error: expected ';' for 'let' declaration let i = 1 < 2; ^ )"); @@ -1215,7 +1215,7 @@ fn f() { var a : u32 } TEST_F(ParserImplErrorTest, VarStmtInvalidAssignment) { EXPECT("fn f() { var a : u32 = >; }", - R"(test.wgsl:1:24 error: missing constructor for variable declaration + R"(test.wgsl:1:24 error: missing constructor for 'var' declaration fn f() { var a : u32 = >; } ^ )"); diff --git a/src/tint/reader/wgsl/parser_impl_for_stmt_test.cc b/src/tint/reader/wgsl/parser_impl_for_stmt_test.cc index 26f3298fd5..3ae7a32f7c 100644 --- a/src/tint/reader/wgsl/parser_impl_for_stmt_test.cc +++ b/src/tint/reader/wgsl/parser_impl_for_stmt_test.cc @@ -253,7 +253,7 @@ TEST_F(ForStmtErrorTest, MissingRightBrace) { // Test a for loop with an invalid initializer statement. TEST_F(ForStmtErrorTest, InvalidInitializerAsConstDecl) { std::string for_str = "for (let x: i32;;) { }"; - std::string error_str = "1:16: expected '=' for let declaration"; + std::string error_str = "1:16: expected '=' for 'let' declaration"; TestForWithError(for_str, error_str); } @@ -304,7 +304,7 @@ TEST_F(ForStmtErrorTest, InvalidContinuingMatch) { // Test a for loop with an invalid body. TEST_F(ForStmtErrorTest, InvalidBody) { std::string for_str = "for (;;) { let x: i32; }"; - std::string error_str = "1:22: expected '=' for let declaration"; + std::string error_str = "1:22: expected '=' for 'let' declaration"; TestForWithError(for_str, error_str); } diff --git a/src/tint/reader/wgsl/parser_impl_global_decl_test.cc b/src/tint/reader/wgsl/parser_impl_global_decl_test.cc index e2c7d7ad6d..59012a3aaa 100644 --- a/src/tint/reader/wgsl/parser_impl_global_decl_test.cc +++ b/src/tint/reader/wgsl/parser_impl_global_decl_test.cc @@ -61,18 +61,25 @@ TEST_F(ParserImplTest, GlobalDecl_GlobalConstant) { EXPECT_EQ(v->symbol, program.Symbols().Get("a")); } +TEST_F(ParserImplTest, GlobalDecl_GlobalConstant_MissingInitializer) { + auto p = parser("let a : vec2;"); + p->global_decl(); + ASSERT_TRUE(p->has_error()); + EXPECT_EQ(p->error(), "1:18: expected '=' for 'let' declaration"); +} + TEST_F(ParserImplTest, GlobalDecl_GlobalConstant_Invalid) { auto p = parser("let a : vec2 1.0;"); p->global_decl(); ASSERT_TRUE(p->has_error()); - EXPECT_EQ(p->error(), "1:19: expected ';' for let declaration"); + EXPECT_EQ(p->error(), "1:19: expected '=' for 'let' declaration"); } TEST_F(ParserImplTest, GlobalDecl_GlobalConstant_MissingSemicolon) { auto p = parser("let a : vec2 = vec2(1, 2)"); p->global_decl(); ASSERT_TRUE(p->has_error()); - EXPECT_EQ(p->error(), "1:36: expected ';' for let declaration"); + EXPECT_EQ(p->error(), "1:36: expected ';' for 'let' declaration"); } TEST_F(ParserImplTest, GlobalDecl_TypeAlias) { diff --git a/src/tint/reader/wgsl/parser_impl_statement_test.cc b/src/tint/reader/wgsl/parser_impl_statement_test.cc index 235c748548..819082907d 100644 --- a/src/tint/reader/wgsl/parser_impl_statement_test.cc +++ b/src/tint/reader/wgsl/parser_impl_statement_test.cc @@ -114,7 +114,7 @@ TEST_F(ParserImplTest, Statement_Variable_Invalid) { EXPECT_TRUE(e.errored); EXPECT_FALSE(e.matched); EXPECT_EQ(e.value, nullptr); - EXPECT_EQ(p->error(), "1:14: missing constructor for variable declaration"); + EXPECT_EQ(p->error(), "1:14: missing constructor for 'var' declaration"); } TEST_F(ParserImplTest, Statement_Variable_MissingSemicolon) { diff --git a/src/tint/reader/wgsl/parser_impl_variable_stmt_test.cc b/src/tint/reader/wgsl/parser_impl_variable_stmt_test.cc index ee899478a8..9fdc5032e5 100644 --- a/src/tint/reader/wgsl/parser_impl_variable_stmt_test.cc +++ b/src/tint/reader/wgsl/parser_impl_variable_stmt_test.cc @@ -63,7 +63,7 @@ TEST_F(ParserImplTest, VariableStmt_VariableDecl_ConstructorInvalid) { EXPECT_TRUE(e.errored); EXPECT_EQ(e.value, nullptr); EXPECT_TRUE(p->has_error()); - EXPECT_EQ(p->error(), "1:15: missing constructor for variable declaration"); + EXPECT_EQ(p->error(), "1:15: missing constructor for 'var' declaration"); } TEST_F(ParserImplTest, VariableStmt_VariableDecl_ArrayInit) { @@ -160,7 +160,7 @@ TEST_F(ParserImplTest, VariableStmt_Let_MissingEqual) { EXPECT_TRUE(e.errored); EXPECT_EQ(e.value, nullptr); EXPECT_TRUE(p->has_error()); - EXPECT_EQ(p->error(), "1:13: expected '=' for let declaration"); + EXPECT_EQ(p->error(), "1:13: expected '=' for 'let' declaration"); } TEST_F(ParserImplTest, VariableStmt_Let_MissingConstructor) { @@ -170,7 +170,7 @@ TEST_F(ParserImplTest, VariableStmt_Let_MissingConstructor) { EXPECT_TRUE(e.errored); EXPECT_EQ(e.value, nullptr); EXPECT_TRUE(p->has_error()); - EXPECT_EQ(p->error(), "1:14: missing constructor for let declaration"); + EXPECT_EQ(p->error(), "1:14: missing constructor for 'let' declaration"); } TEST_F(ParserImplTest, VariableStmt_Let_InvalidConstructor) { @@ -180,7 +180,7 @@ TEST_F(ParserImplTest, VariableStmt_Let_InvalidConstructor) { EXPECT_TRUE(e.errored); EXPECT_EQ(e.value, nullptr); EXPECT_TRUE(p->has_error()); - EXPECT_EQ(p->error(), "1:15: missing constructor for let declaration"); + EXPECT_EQ(p->error(), "1:15: missing constructor for 'let' declaration"); } } // namespace diff --git a/src/tint/resolver/assignment_validation_test.cc b/src/tint/resolver/assignment_validation_test.cc index 64363e3942..6af636e160 100644 --- a/src/tint/resolver/assignment_validation_test.cc +++ b/src/tint/resolver/assignment_validation_test.cc @@ -222,7 +222,7 @@ TEST_F(ResolverAssignmentValidationTest, AssignToLet_Fail) { WrapInFunction(var, Assign(Expr(Source{{12, 34}}, "a"), 2_i)); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "12:34 error: cannot assign to const\nnote: 'a' is declared here:"); + EXPECT_EQ(r()->error(), "12:34 error: cannot assign to 'let'\nnote: 'a' is declared here:"); } TEST_F(ResolverAssignmentValidationTest, AssignNonConstructible_Handle) { diff --git a/src/tint/resolver/attribute_validation_test.cc b/src/tint/resolver/attribute_validation_test.cc index 29ad1525bb..f7e401be93 100644 --- a/src/tint/resolver/attribute_validation_test.cc +++ b/src/tint/resolver/attribute_validation_test.cc @@ -721,7 +721,7 @@ TEST_P(VariableAttributeTest, IsValid) { } else { EXPECT_FALSE(r()->Resolve()); if (!IsBindingAttribute(params.kind)) { - EXPECT_EQ(r()->error(), "12:34 error: attribute is not valid for variables"); + EXPECT_EQ(r()->error(), "12:34 error: attribute is not valid for module-scope 'var'"); } } } @@ -783,11 +783,59 @@ TEST_P(ConstantAttributeTest, IsValid) { EXPECT_TRUE(r()->Resolve()) << r()->error(); } else { EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "12:34 error: attribute is not valid for constants"); + EXPECT_EQ(r()->error(), + "12:34 error: attribute is not valid for module-scope 'let' declaration"); } } INSTANTIATE_TEST_SUITE_P(ResolverAttributeValidationTest, ConstantAttributeTest, + testing::Values(TestParams{AttributeKind::kAlign, false}, + TestParams{AttributeKind::kBinding, false}, + TestParams{AttributeKind::kBuiltin, false}, + TestParams{AttributeKind::kGroup, false}, + TestParams{AttributeKind::kId, false}, + TestParams{AttributeKind::kInterpolate, false}, + TestParams{AttributeKind::kInvariant, false}, + TestParams{AttributeKind::kLocation, false}, + TestParams{AttributeKind::kOffset, false}, + TestParams{AttributeKind::kSize, false}, + TestParams{AttributeKind::kStage, false}, + TestParams{AttributeKind::kStride, false}, + TestParams{AttributeKind::kWorkgroup, false}, + TestParams{AttributeKind::kBindingAndGroup, false})); + +TEST_F(ConstantAttributeTest, DuplicateAttribute) { + GlobalConst("a", ty.f32(), Expr(1.23_f), + ast::AttributeList{ + create(Source{{12, 34}}, 0), + create(Source{{56, 78}}, 1), + }); + + WrapInFunction(); + + EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ(r()->error(), + R"(56:78 error: duplicate id attribute +12:34 note: first attribute declared here)"); +} + +using OverrideAttributeTest = TestWithParams; +TEST_P(OverrideAttributeTest, IsValid) { + auto& params = GetParam(); + + Override("a", ty.f32(), Expr(1.23_f), createAttributes(Source{{12, 34}}, *this, params.kind)); + + WrapInFunction(); + + if (params.should_pass) { + EXPECT_TRUE(r()->Resolve()) << r()->error(); + } else { + EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ(r()->error(), "12:34 error: attribute is not valid for 'override' declaration"); + } +} +INSTANTIATE_TEST_SUITE_P(ResolverAttributeValidationTest, + OverrideAttributeTest, testing::Values(TestParams{AttributeKind::kAlign, false}, TestParams{AttributeKind::kBinding, false}, TestParams{AttributeKind::kBuiltin, false}, @@ -803,7 +851,7 @@ INSTANTIATE_TEST_SUITE_P(ResolverAttributeValidationTest, TestParams{AttributeKind::kWorkgroup, false}, TestParams{AttributeKind::kBindingAndGroup, false})); -TEST_F(ConstantAttributeTest, DuplicateAttribute) { +TEST_F(OverrideAttributeTest, DuplicateAttribute) { GlobalConst("a", ty.f32(), Expr(1.23_f), ast::AttributeList{ create(Source{{12, 34}}, 0), diff --git a/src/tint/resolver/compound_assignment_validation_test.cc b/src/tint/resolver/compound_assignment_validation_test.cc index 1ae70406b4..b453c3b5d8 100644 --- a/src/tint/resolver/compound_assignment_validation_test.cc +++ b/src/tint/resolver/compound_assignment_validation_test.cc @@ -249,7 +249,7 @@ TEST_F(ResolverCompoundAssignmentValidationTest, LhsConstant) { WrapInFunction(a, CompoundAssign(Expr(Source{{56, 78}}, "a"), 1_i, ast::BinaryOp::kAdd)); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), R"(56:78 error: cannot assign to const + EXPECT_EQ(r()->error(), R"(56:78 error: cannot assign to 'let' 12:34 note: 'a' is declared here:)"); } diff --git a/src/tint/resolver/increment_decrement_validation_test.cc b/src/tint/resolver/increment_decrement_validation_test.cc index e03352e97b..b8e3aa16d9 100644 --- a/src/tint/resolver/increment_decrement_validation_test.cc +++ b/src/tint/resolver/increment_decrement_validation_test.cc @@ -151,7 +151,7 @@ TEST_F(ResolverIncrementDecrementValidationTest, Constant) { WrapInFunction(a, Increment(Expr(Source{{56, 78}}, "a"))); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), R"(56:78 error: cannot modify constant value + EXPECT_EQ(r()->error(), R"(56:78 error: cannot modify 'let' 12:34 note: 'a' is declared here:)"); } diff --git a/src/tint/resolver/inferred_type_test.cc b/src/tint/resolver/inferred_type_test.cc index d8cc649ffa..4d01f7e77f 100644 --- a/src/tint/resolver/inferred_type_test.cc +++ b/src/tint/resolver/inferred_type_test.cc @@ -98,7 +98,7 @@ TEST_P(ResolverInferredTypeParamTest, GlobalVar_Fail) { WrapInFunction(); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "12:34 error: global var declaration must specify a type"); + EXPECT_EQ(r()->error(), "12:34 error: module-scope 'var' declaration must specify a type"); } TEST_P(ResolverInferredTypeParamTest, LocalLet_Pass) { diff --git a/src/tint/resolver/resolver.cc b/src/tint/resolver/resolver.cc index b1f5a95b1c..7bf0b25418 100644 --- a/src/tint/resolver/resolver.cc +++ b/src/tint/resolver/resolver.cc @@ -326,19 +326,19 @@ sem::Variable* Resolver::Variable(const ast::Variable* var, // If the variable has no declared type, infer it from the RHS if (!storage_ty) { if (!var->is_const && kind == VariableKind::kGlobal) { - AddError("global var declaration must specify a type", var->source); + AddError("module-scope 'var' declaration must specify a type", var->source); return nullptr; } storage_ty = rhs->Type()->UnwrapRef(); // Implicit load of RHS } } else if (var->is_const && !var->is_overridable && kind != VariableKind::kParameter) { - AddError("let declaration must have an initializer", var->source); + AddError("'let' declaration must have an initializer", var->source); return nullptr; } else if (!var->type) { AddError((kind == VariableKind::kGlobal) - ? "module scope var declaration requires a type and initializer" - : "function scope var declaration requires a type or initializer", + ? "module-scope 'var' declaration requires a type or initializer" + : "function-scope 'var' declaration requires a type or initializer", var->source); return nullptr; } @@ -368,7 +368,7 @@ sem::Variable* Resolver::Variable(const ast::Variable* var, storage_class != ast::StorageClass::kFunction && validator_.IsValidationEnabled(var->attributes, ast::DisabledValidation::kIgnoreStorageClass)) { - AddError("function variable has a non-function storage class", var->source); + AddError("function-scope 'var' declaration must use 'function' storage class", var->source); return nullptr; } @@ -519,11 +519,13 @@ sem::GlobalVariable* Resolver::GlobalVariable(const ast::Variable* var) { auto storage_class = sem->StorageClass(); if (!var->is_const && storage_class == ast::StorageClass::kNone) { - AddError("global variables must have a storage class", var->source); + AddError("module-scope 'var' declaration must have a storage class", var->source); return nullptr; } if (var->is_const && storage_class != ast::StorageClass::kNone) { - AddError("global constants shouldn't have a storage class", var->source); + AddError(var->is_overridable ? "'override' declaration must not have a storage class" + : "'let' declaration must not have a storage class", + var->source); return nullptr; } @@ -2069,21 +2071,16 @@ sem::Array* Resolver::Array(const ast::Array* arr) { if (auto* ident = count_expr->As()) { // Make sure the identifier is a non-overridable module-scope constant. auto* var = sem_.ResolvedSymbol(ident); - if (!var || !var->Declaration()->is_const) { - AddError("array size identifier must be a module-scope constant", size_source); - return nullptr; - } - if (var->IsOverridable()) { - AddError("array size expression must not be pipeline-overridable", size_source); + if (!var || !var->Declaration()->is_const || var->IsOverridable()) { + AddError("array size identifier must be a literal or a module-scope 'let'", + size_source); return nullptr; } count_expr = var->Declaration()->constructor; } else if (!count_expr->Is()) { - AddError( - "array size expression must be either a literal or a module-scope " - "constant", - size_source); + AddError("array size identifier must be a literal or a module-scope 'let'", + size_source); return nullptr; } diff --git a/src/tint/resolver/storage_class_validation_test.cc b/src/tint/resolver/storage_class_validation_test.cc index 2d75167ea8..a5e7d12c67 100644 --- a/src/tint/resolver/storage_class_validation_test.cc +++ b/src/tint/resolver/storage_class_validation_test.cc @@ -30,7 +30,8 @@ TEST_F(ResolverStorageClassValidationTest, GlobalVariableNoStorageClass_Fail) { Global(Source{{12, 34}}, "g", ty.f32(), ast::StorageClass::kNone); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "12:34 error: global variables must have a storage class"); + EXPECT_EQ(r()->error(), + "12:34 error: module-scope 'var' declaration must have a storage class"); } TEST_F(ResolverStorageClassValidationTest, GlobalVariableFunctionStorageClass_Fail) { @@ -39,8 +40,7 @@ TEST_F(ResolverStorageClassValidationTest, GlobalVariableFunctionStorageClass_Fa EXPECT_FALSE(r()->Resolve()); EXPECT_EQ(r()->error(), - "12:34 error: variables declared at module scope must not be in " - "the function storage class"); + "12:34 error: module-scope 'var' must not use storage class 'function'"); } TEST_F(ResolverStorageClassValidationTest, Private_RuntimeArray) { diff --git a/src/tint/resolver/type_validation_test.cc b/src/tint/resolver/type_validation_test.cc index 119c310e89..27ee04d606 100644 --- a/src/tint/resolver/type_validation_test.cc +++ b/src/tint/resolver/type_validation_test.cc @@ -90,11 +90,21 @@ TEST_F(ResolverTypeValidationTest, GlobalVariableWithStorageClass_Pass) { TEST_F(ResolverTypeValidationTest, GlobalLetWithStorageClass_Fail) { // let global_var: f32; AST().AddGlobalVariable(create( - Source{{12, 34}}, Symbols().Register("global_var"), ast::StorageClass::kPrivate, + Source{{12, 34}}, Symbols().Register("global_let"), ast::StorageClass::kPrivate, ast::Access::kUndefined, ty.f32(), true, false, Expr(1.23_f), ast::AttributeList{})); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "12:34 error: global constants shouldn't have a storage class"); + EXPECT_EQ(r()->error(), "12:34 error: 'let' declaration must not have a storage class"); +} + +TEST_F(ResolverTypeValidationTest, OverrideWithStorageClass_Fail) { + // let global_var: f32; + AST().AddGlobalVariable(create( + Source{{12, 34}}, Symbols().Register("global_override"), ast::StorageClass::kPrivate, + ast::Access::kUndefined, ty.f32(), true, true, Expr(1.23_f), ast::AttributeList{})); + + EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ(r()->error(), "12:34 error: 'override' declaration must not have a storage class"); } TEST_F(ResolverTypeValidationTest, GlobalConstNoStorageClass_Pass) { @@ -334,7 +344,8 @@ TEST_F(ResolverTypeValidationTest, ArraySize_Overridable) { Override("size", nullptr, Expr(10_i)); Global("a", ty.array(ty.f32(), Expr(Source{{12, 34}}, "size")), ast::StorageClass::kPrivate); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "12:34 error: array size expression must not be pipeline-overridable"); + EXPECT_EQ(r()->error(), + "12:34 error: array size identifier must be a literal or a module-scope 'let'"); } TEST_F(ResolverTypeValidationTest, ArraySize_ModuleVar) { @@ -343,7 +354,8 @@ TEST_F(ResolverTypeValidationTest, ArraySize_ModuleVar) { Global("size", ty.i32(), Expr(10_i), ast::StorageClass::kPrivate); Global("a", ty.array(ty.f32(), Expr(Source{{12, 34}}, "size")), ast::StorageClass::kPrivate); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "12:34 error: array size identifier must be a module-scope constant"); + EXPECT_EQ(r()->error(), + "12:34 error: array size identifier must be a literal or a module-scope 'let'"); } TEST_F(ResolverTypeValidationTest, ArraySize_FunctionLet) { @@ -355,7 +367,8 @@ TEST_F(ResolverTypeValidationTest, ArraySize_FunctionLet) { auto* a = Var("a", ty.array(ty.f32(), Expr(Source{{12, 34}}, "size"))); WrapInFunction(Block(Decl(size), Decl(a))); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "12:34 error: array size identifier must be a module-scope constant"); + EXPECT_EQ(r()->error(), + "12:34 error: array size identifier must be a literal or a module-scope 'let'"); } TEST_F(ResolverTypeValidationTest, ArraySize_InvalidExpr) { @@ -364,8 +377,7 @@ TEST_F(ResolverTypeValidationTest, ArraySize_InvalidExpr) { WrapInFunction(Block(Decl(a))); EXPECT_FALSE(r()->Resolve()); EXPECT_EQ(r()->error(), - "12:34 error: array size expression must be either a literal or a " - "module-scope constant"); + "12:34 error: array size identifier must be a literal or a module-scope 'let'"); } TEST_F(ResolverTypeValidationTest, RuntimeArrayInFunction_Fail) { diff --git a/src/tint/resolver/validation_test.cc b/src/tint/resolver/validation_test.cc index 8cc3a8f2c3..9c25620ecd 100644 --- a/src/tint/resolver/validation_test.cc +++ b/src/tint/resolver/validation_test.cc @@ -304,7 +304,8 @@ TEST_F(ResolverValidationTest, StorageClass_FunctionVariableWorkgroupClass) { EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "error: function variable has a non-function storage class"); + EXPECT_EQ(r()->error(), + "error: function-scope 'var' declaration must use 'function' storage class"); } TEST_F(ResolverValidationTest, StorageClass_FunctionVariableI32) { @@ -317,7 +318,8 @@ TEST_F(ResolverValidationTest, StorageClass_FunctionVariableI32) { EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "error: function variable has a non-function storage class"); + EXPECT_EQ(r()->error(), + "error: function-scope 'var' declaration must use 'function' storage class"); } TEST_F(ResolverValidationTest, StorageClass_SamplerExplicitStorageClass) { diff --git a/src/tint/resolver/validator.cc b/src/tint/resolver/validator.cc index 4d1ee075b7..c8e1a0b6e5 100644 --- a/src/tint/resolver/validator.cc +++ b/src/tint/resolver/validator.cc @@ -506,25 +506,29 @@ bool Validator::GlobalVariable( for (auto* attr : decl->attributes) { if (decl->is_const) { - if (auto* id_attr = attr->As()) { - uint32_t id = id_attr->value; - auto it = constant_ids.find(id); - if (it != constant_ids.end() && it->second != var) { - AddError("pipeline constant IDs must be unique", attr->source); - AddNote( - "a pipeline constant with an ID of " + std::to_string(id) + - " was previously declared " - "here:", - ast::GetAttribute(it->second->Declaration()->attributes) - ->source); - return false; - } - if (id > 65535) { - AddError("pipeline constant IDs must be between 0 and 65535", attr->source); + if (decl->is_overridable) { + if (auto* id_attr = attr->As()) { + uint32_t id = id_attr->value; + auto it = constant_ids.find(id); + if (it != constant_ids.end() && it->second != var) { + AddError("pipeline constant IDs must be unique", attr->source); + AddNote("a pipeline constant with an ID of " + std::to_string(id) + + " was previously declared here:", + ast::GetAttribute( + it->second->Declaration()->attributes) + ->source); + return false; + } + if (id > 65535) { + AddError("pipeline constant IDs must be between 0 and 65535", attr->source); + return false; + } + } else { + AddError("attribute is not valid for 'override' declaration", attr->source); return false; } } else { - AddError("attribute is not valid for constants", attr->source); + AddError("attribute is not valid for module-scope 'let' declaration", attr->source); return false; } } else { @@ -536,17 +540,14 @@ bool Validator::GlobalVariable( if (!(attr->IsAnyOf()) && (!is_shader_io_attribute || !has_io_storage_class)) { - AddError("attribute is not valid for variables", attr->source); + AddError("attribute is not valid for module-scope 'var'", attr->source); return false; } } } if (var->StorageClass() == ast::StorageClass::kFunction) { - AddError( - "variables declared at module scope must not be in the function " - "storage class", - decl->source); + AddError("module-scope 'var' must not use storage class 'function'", decl->source); return false; } @@ -559,10 +560,7 @@ bool Validator::GlobalVariable( // Each resource variable must be declared with both group and binding // attributes. if (!binding_point) { - AddError( - "resource variables require @group and @binding " - "attributes", - decl->source); + AddError("resource variables require @group and @binding attributes", decl->source); return false; } break; @@ -571,10 +569,8 @@ bool Validator::GlobalVariable( if (binding_point.binding || binding_point.group) { // https://gpuweb.github.io/gpuweb/wgsl/#attribute-binding // Must only be applied to a resource variable - AddError( - "non-resource variables must not have @group or @binding " - "attributes", - decl->source); + AddError("non-resource variables must not have @group or @binding attributes", + decl->source); return false; } } @@ -2162,7 +2158,9 @@ bool Validator::Assignment(const ast::Statement* a, const sem::Type* rhs_ty) con return false; } if (decl->is_const) { - AddError("cannot assign to const", lhs->source); + AddError( + decl->is_overridable ? "cannot assign to 'override'" : "cannot assign to 'let'", + lhs->source); AddNote("'" + symbols_.NameFor(decl->symbol) + "' is declared here:", decl->source); return false; } @@ -2210,7 +2208,8 @@ bool Validator::IncrementDecrementStatement(const ast::IncrementDecrementStateme return false; } if (decl->is_const) { - AddError("cannot modify constant value", lhs->source); + AddError(decl->is_overridable ? "cannot modify 'override'" : "cannot modify 'let'", + lhs->source); AddNote("'" + symbols_.NameFor(decl->symbol) + "' is declared here:", decl->source); return false; } diff --git a/src/tint/resolver/var_let_validation_test.cc b/src/tint/resolver/var_let_validation_test.cc index e1dc3435a4..eea5e72f59 100644 --- a/src/tint/resolver/var_let_validation_test.cc +++ b/src/tint/resolver/var_let_validation_test.cc @@ -29,7 +29,7 @@ TEST_F(ResolverVarLetValidationTest, LetNoInitializer) { WrapInFunction(Let(Source{{12, 34}}, "a", ty.i32(), nullptr)); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "12:34 error: let declaration must have an initializer"); + EXPECT_EQ(r()->error(), "12:34 error: 'let' declaration must have an initializer"); } TEST_F(ResolverVarLetValidationTest, GlobalLetNoInitializer) { @@ -37,7 +37,7 @@ TEST_F(ResolverVarLetValidationTest, GlobalLetNoInitializer) { GlobalConst(Source{{12, 34}}, "a", ty.i32(), nullptr); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "12:34 error: let declaration must have an initializer"); + EXPECT_EQ(r()->error(), "12:34 error: 'let' declaration must have an initializer"); } TEST_F(ResolverVarLetValidationTest, VarNoInitializerNoType) { @@ -46,8 +46,7 @@ TEST_F(ResolverVarLetValidationTest, VarNoInitializerNoType) { EXPECT_FALSE(r()->Resolve()); EXPECT_EQ(r()->error(), - "12:34 error: function scope var declaration requires a type or " - "initializer"); + "12:34 error: function-scope 'var' declaration requires a type or initializer"); } TEST_F(ResolverVarLetValidationTest, GlobalVarNoInitializerNoType) { @@ -56,8 +55,7 @@ TEST_F(ResolverVarLetValidationTest, GlobalVarNoInitializerNoType) { EXPECT_FALSE(r()->Resolve()); EXPECT_EQ(r()->error(), - "12:34 error: module scope var declaration requires a type and " - "initializer"); + "12:34 error: module-scope 'var' declaration requires a type or initializer"); } TEST_F(ResolverVarLetValidationTest, VarTypeNotStorable) {