From 9535f7220913acd92d59c76009cb31b9410b1e21 Mon Sep 17 00:00:00 2001 From: Ben Clayton Date: Wed, 2 Nov 2022 18:14:59 +0000 Subject: [PATCH] tint/validator: Hint 'var' instead of 'const' Involves expanding the source range of a variable declaration so we can point at something that can include the 'const'. Fixed: tint:1740 Change-Id: Ie8f784de34a1792002aaa708c1b77053be54f1b5 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/108120 Kokoro: Kokoro Reviewed-by: David Neto Commit-Queue: Ben Clayton --- src/tint/reader/wgsl/parser_impl.cc | 58 +++++++++++-------- .../wgsl/parser_impl_variable_stmt_test.cc | 24 ++++---- src/tint/resolver/validator.cc | 8 +++ src/tint/resolver/variable_validation_test.cc | 39 +++++++++---- .../tint/bug/tint/1369.wgsl.expected.dxc.hlsl | 4 +- .../tint/bug/tint/1369.wgsl.expected.fxc.hlsl | 4 +- test/tint/bug/tint/1369.wgsl.expected.glsl | 4 +- test/tint/bug/tint/1369.wgsl.expected.msl | 4 +- test/tint/bug/tint/1369.wgsl.expected.spvasm | 4 +- test/tint/bug/tint/1369.wgsl.expected.wgsl | 4 +- .../bug/tint/1474-b.wgsl.expected.dxc.hlsl | 4 +- .../bug/tint/1474-b.wgsl.expected.fxc.hlsl | 4 +- test/tint/bug/tint/1474-b.wgsl.expected.glsl | 4 +- test/tint/bug/tint/1474-b.wgsl.expected.msl | 4 +- .../tint/bug/tint/1474-b.wgsl.expected.spvasm | 4 +- test/tint/bug/tint/1474-b.wgsl.expected.wgsl | 4 +- 16 files changed, 106 insertions(+), 71 deletions(-) diff --git a/src/tint/reader/wgsl/parser_impl.cc b/src/tint/reader/wgsl/parser_impl.cc index 62352ee1e2..976706a115 100644 --- a/src/tint/reader/wgsl/parser_impl.cc +++ b/src/tint/reader/wgsl/parser_impl.cc @@ -154,21 +154,26 @@ class ParserImpl::MultiTokenSource { /// Constructor that starts with the input `start` Source /// @param parser the parser /// @param start the start source of the range - MultiTokenSource(ParserImpl* parser, const Source& start) : parser_(parser), start_(start) {} + MultiTokenSource(ParserImpl* parser, const tint::Source& start) + : parser_(parser), start_(start) {} - /// Implicit conversion to Source that returns the combined source from start - /// to the current last token's source. - operator Source() const { - Source end = parser_->last_source().End(); + /// @returns the Source that returns the combined source from start to the current last token's + /// source. + tint::Source Source() const { + auto end = parser_->last_source().End(); if (end < start_) { end = start_; } return Source::Combine(start_, end); } + /// Implicit conversion to Source that returns the combined source from start to the current + /// last token's source. + operator tint::Source() const { return Source(); } + private: ParserImpl* parser_; - Source start_; + tint::Source start_; }; ParserImpl::TypedIdentifier::TypedIdentifier() = default; @@ -1901,12 +1906,15 @@ Maybe ParserImpl::return_statement() { // | LET optionally_typed_ident EQUAL expression // | CONST optionally_typed_ident EQUAL expression Maybe ParserImpl::variable_statement() { + auto decl_source_range = make_source_range(); if (match(Token::Type::kConst)) { - auto decl = expect_optionally_typed_ident("'const' declaration"); - if (decl.errored) { + auto typed_ident = expect_optionally_typed_ident("'const' declaration"); + if (typed_ident.errored) { return Failure::kErrored; } + auto decl_source = decl_source_range.Source(); + if (!expect("'const' declaration", Token::Type::kEqual)) { return Failure::kErrored; } @@ -1919,21 +1927,23 @@ Maybe ParserImpl::variable_statement() { return add_error(peek(), "missing initializer for 'const' declaration"); } - auto* const_ = create(decl->source, // source - builder_.Symbols().Register(decl->name), // symbol - decl->type, // type - initializer.value, // initializer - utils::Empty); // attributes + auto* const_ = create(typed_ident->source, // source + builder_.Symbols().Register(typed_ident->name), // symbol + typed_ident->type, // type + initializer.value, // initializer + utils::Empty); // attributes - return create(decl->source, const_); + return create(decl_source, const_); } if (match(Token::Type::kLet)) { - auto decl = expect_optionally_typed_ident("'let' declaration"); - if (decl.errored) { + auto typed_ident = expect_optionally_typed_ident("'let' declaration"); + if (typed_ident.errored) { return Failure::kErrored; } + auto decl_source = decl_source_range.Source(); + if (!expect("'let' declaration", Token::Type::kEqual)) { return Failure::kErrored; } @@ -1946,13 +1956,13 @@ Maybe ParserImpl::variable_statement() { return add_error(peek(), "missing initializer for 'let' declaration"); } - auto* let = create(decl->source, // source - builder_.Symbols().Register(decl->name), // symbol - decl->type, // type - initializer.value, // initializer - utils::Empty); // attributes + auto* let = create(typed_ident->source, // source + builder_.Symbols().Register(typed_ident->name), // symbol + typed_ident->type, // type + initializer.value, // initializer + utils::Empty); // attributes - return create(decl->source, let); + return create(decl_source, let); } auto decl = variable_decl(); @@ -1963,6 +1973,8 @@ Maybe ParserImpl::variable_statement() { return Failure::kNoMatch; } + auto decl_source = decl_source_range.Source(); + const ast::Expression* initializer = nullptr; if (match(Token::Type::kEqual)) { auto initializer_expr = expression(); @@ -1976,7 +1988,7 @@ Maybe ParserImpl::variable_statement() { initializer = initializer_expr.value; } - auto* var = create(decl->source, // source + auto* var = create(decl_source, // source builder_.Symbols().Register(decl->name), // symbol decl->type, // type decl->address_space, // address space 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 703eb572e3..05f2976bf7 100644 --- a/src/tint/reader/wgsl/parser_impl_variable_stmt_test.cc +++ b/src/tint/reader/wgsl/parser_impl_variable_stmt_test.cc @@ -28,10 +28,10 @@ TEST_F(ParserImplTest, VariableStmt_VariableDecl) { ASSERT_NE(e->variable, nullptr); EXPECT_EQ(e->variable->symbol, p->builder().Symbols().Get("a")); - ASSERT_EQ(e->source.range.begin.line, 1u); - ASSERT_EQ(e->source.range.begin.column, 5u); - ASSERT_EQ(e->source.range.end.line, 1u); - ASSERT_EQ(e->source.range.end.column, 6u); + EXPECT_EQ(e->source.range.begin.line, 1u); + EXPECT_EQ(e->source.range.begin.column, 1u); + EXPECT_EQ(e->source.range.end.line, 1u); + EXPECT_EQ(e->source.range.end.column, 12u); EXPECT_EQ(e->variable->initializer, nullptr); } @@ -47,10 +47,10 @@ TEST_F(ParserImplTest, VariableStmt_VariableDecl_WithInit) { ASSERT_NE(e->variable, nullptr); EXPECT_EQ(e->variable->symbol, p->builder().Symbols().Get("a")); - ASSERT_EQ(e->source.range.begin.line, 1u); - ASSERT_EQ(e->source.range.begin.column, 5u); - ASSERT_EQ(e->source.range.end.line, 1u); - ASSERT_EQ(e->source.range.end.column, 6u); + EXPECT_EQ(e->source.range.begin.line, 1u); + EXPECT_EQ(e->source.range.begin.column, 1u); + EXPECT_EQ(e->source.range.end.line, 1u); + EXPECT_EQ(e->source.range.end.column, 12u); ASSERT_NE(e->variable->initializer, nullptr); EXPECT_TRUE(e->variable->initializer->Is()); @@ -147,10 +147,10 @@ TEST_F(ParserImplTest, VariableStmt_Let) { ASSERT_NE(e.value, nullptr); ASSERT_TRUE(e->Is()); - ASSERT_EQ(e->source.range.begin.line, 1u); - ASSERT_EQ(e->source.range.begin.column, 5u); - ASSERT_EQ(e->source.range.end.line, 1u); - ASSERT_EQ(e->source.range.end.column, 6u); + EXPECT_EQ(e->source.range.begin.line, 1u); + EXPECT_EQ(e->source.range.begin.column, 1u); + EXPECT_EQ(e->source.range.end.line, 1u); + EXPECT_EQ(e->source.range.end.column, 12u); } TEST_F(ParserImplTest, VariableStmt_Let_ComplexExpression) { diff --git a/src/tint/resolver/validator.cc b/src/tint/resolver/validator.cc index 0d6bfe65bd..f96af05ebc 100644 --- a/src/tint/resolver/validator.cc +++ b/src/tint/resolver/validator.cc @@ -1382,6 +1382,14 @@ bool Validator::EvaluationStage(const sem::Expression* expr, AddError(std::string(constraint) + " requires " + stage_name(latest_stage) + ", but expression is " + stage_name(expr->Stage()), expr->Declaration()->source); + + if (auto* stmt = expr->Stmt()) { + if (auto* decl = As(stmt->Declaration())) { + if (decl->variable->Is()) { + AddNote("consider changing 'const' to 'let'", decl->source); + } + } + } return false; } return true; diff --git a/src/tint/resolver/variable_validation_test.cc b/src/tint/resolver/variable_validation_test.cc index d302264244..964c8d9713 100644 --- a/src/tint/resolver/variable_validation_test.cc +++ b/src/tint/resolver/variable_validation_test.cc @@ -417,10 +417,8 @@ TEST_F(ResolverVariableValidationTest, MatrixVarNoType) { EXPECT_EQ(r()->error(), "12:34 error: missing matrix element type"); } -TEST_F(ResolverVariableValidationTest, ConstInitWithVar) { - auto* v = Var("v", Expr(1_i)); - auto* c = Const("c", Expr(Source{{12, 34}}, v)); - WrapInFunction(v, c); +TEST_F(ResolverVariableValidationTest, GlobalConstWithRuntimeExpression) { + GlobalConst("c", Call(Source{{12, 34}}, "dpdx", 1._a)); EXPECT_FALSE(r()->Resolve()); EXPECT_EQ( @@ -428,47 +426,64 @@ TEST_F(ResolverVariableValidationTest, ConstInitWithVar) { R"(12:34 error: const initializer requires a const-expression, but expression is a runtime-expression)"); } +TEST_F(ResolverVariableValidationTest, ConstInitWithVar) { + auto* v = Var("v", Expr(1_i)); + auto* c = Const("c", Expr(Source{{12, 34}}, v)); + WrapInFunction(v, Decl(Source{{56, 78}}, c)); + + EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ( + r()->error(), + R"(12:34 error: const initializer requires a const-expression, but expression is a runtime-expression +56:78 note: consider changing 'const' to 'let')"); +} + TEST_F(ResolverVariableValidationTest, ConstInitWithOverride) { auto* o = Override("v", Expr(1_i)); auto* c = Const("c", Expr(Source{{12, 34}}, o)); - WrapInFunction(c); + WrapInFunction(Decl(Source{{56, 78}}, c)); EXPECT_FALSE(r()->Resolve()); EXPECT_EQ( r()->error(), - R"(12:34 error: const initializer requires a const-expression, but expression is an override-expression)"); + R"(12:34 error: const initializer requires a const-expression, but expression is an override-expression +56:78 note: consider changing 'const' to 'let')"); } TEST_F(ResolverVariableValidationTest, ConstInitWithLet) { auto* l = Let("v", Expr(1_i)); auto* c = Const("c", Expr(Source{{12, 34}}, l)); - WrapInFunction(l, c); + WrapInFunction(l, Decl(Source{{56, 78}}, c)); EXPECT_FALSE(r()->Resolve()); EXPECT_EQ( r()->error(), - R"(12:34 error: const initializer requires a const-expression, but expression is a runtime-expression)"); + R"(12:34 error: const initializer requires a const-expression, but expression is a runtime-expression +56:78 note: consider changing 'const' to 'let')"); } TEST_F(ResolverVariableValidationTest, ConstInitWithRuntimeExpr) { // const c = clamp(2, dpdx(0.5), 3); - WrapInFunction(Const("c", Call("clamp", 2_a, Call(Source{{12, 34}}, "dpdx", 0.5_a), 3_a))); + auto* c = Const("c", Call("clamp", 2_a, Call(Source{{12, 34}}, "dpdx", 0.5_a), 3_a)); + WrapInFunction(Decl(Source{{56, 78}}, c)); EXPECT_FALSE(r()->Resolve()); EXPECT_EQ( r()->error(), - R"(12:34 error: const initializer requires a const-expression, but expression is a runtime-expression)"); + R"(12:34 error: const initializer requires a const-expression, but expression is a runtime-expression +56:78 note: consider changing 'const' to 'let')"); } TEST_F(ResolverVariableValidationTest, ConstInitWithOverrideExpr) { auto* o = Override("v", Expr(1_i)); auto* c = Const("c", Add(10_a, Expr(Source{{12, 34}}, o))); - WrapInFunction(c); + WrapInFunction(Decl(Source{{56, 78}}, c)); EXPECT_FALSE(r()->Resolve()); EXPECT_EQ( r()->error(), - R"(12:34 error: const initializer requires a const-expression, but expression is an override-expression)"); + R"(12:34 error: const initializer requires a const-expression, but expression is an override-expression +56:78 note: consider changing 'const' to 'let')"); } } // namespace diff --git a/test/tint/bug/tint/1369.wgsl.expected.dxc.hlsl b/test/tint/bug/tint/1369.wgsl.expected.dxc.hlsl index d0fffc5379..0981f3857f 100644 --- a/test/tint/bug/tint/1369.wgsl.expected.dxc.hlsl +++ b/test/tint/bug/tint/1369.wgsl.expected.dxc.hlsl @@ -2,9 +2,9 @@ bug/tint/1369.wgsl:3:3 warning: code is unreachable return true; ^^^^^^ -bug/tint/1369.wgsl:9:9 warning: code is unreachable +bug/tint/1369.wgsl:9:5 warning: code is unreachable var also_unreachable : bool; - ^^^^^^^^^^^^^^^^ + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ static bool tint_discard = false; diff --git a/test/tint/bug/tint/1369.wgsl.expected.fxc.hlsl b/test/tint/bug/tint/1369.wgsl.expected.fxc.hlsl index d0fffc5379..0981f3857f 100644 --- a/test/tint/bug/tint/1369.wgsl.expected.fxc.hlsl +++ b/test/tint/bug/tint/1369.wgsl.expected.fxc.hlsl @@ -2,9 +2,9 @@ bug/tint/1369.wgsl:3:3 warning: code is unreachable return true; ^^^^^^ -bug/tint/1369.wgsl:9:9 warning: code is unreachable +bug/tint/1369.wgsl:9:5 warning: code is unreachable var also_unreachable : bool; - ^^^^^^^^^^^^^^^^ + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ static bool tint_discard = false; diff --git a/test/tint/bug/tint/1369.wgsl.expected.glsl b/test/tint/bug/tint/1369.wgsl.expected.glsl index dbecc854bb..82b1904238 100644 --- a/test/tint/bug/tint/1369.wgsl.expected.glsl +++ b/test/tint/bug/tint/1369.wgsl.expected.glsl @@ -2,9 +2,9 @@ bug/tint/1369.wgsl:3:3 warning: code is unreachable return true; ^^^^^^ -bug/tint/1369.wgsl:9:9 warning: code is unreachable +bug/tint/1369.wgsl:9:5 warning: code is unreachable var also_unreachable : bool; - ^^^^^^^^^^^^^^^^ + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ #version 310 es precision mediump float; diff --git a/test/tint/bug/tint/1369.wgsl.expected.msl b/test/tint/bug/tint/1369.wgsl.expected.msl index 4fc75b1fea..32624c3edd 100644 --- a/test/tint/bug/tint/1369.wgsl.expected.msl +++ b/test/tint/bug/tint/1369.wgsl.expected.msl @@ -2,9 +2,9 @@ bug/tint/1369.wgsl:3:3 warning: code is unreachable return true; ^^^^^^ -bug/tint/1369.wgsl:9:9 warning: code is unreachable +bug/tint/1369.wgsl:9:5 warning: code is unreachable var also_unreachable : bool; - ^^^^^^^^^^^^^^^^ + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ #include diff --git a/test/tint/bug/tint/1369.wgsl.expected.spvasm b/test/tint/bug/tint/1369.wgsl.expected.spvasm index f9013117ed..803df82849 100644 --- a/test/tint/bug/tint/1369.wgsl.expected.spvasm +++ b/test/tint/bug/tint/1369.wgsl.expected.spvasm @@ -2,9 +2,9 @@ bug/tint/1369.wgsl:3:3 warning: code is unreachable return true; ^^^^^^ -bug/tint/1369.wgsl:9:9 warning: code is unreachable +bug/tint/1369.wgsl:9:5 warning: code is unreachable var also_unreachable : bool; - ^^^^^^^^^^^^^^^^ + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ ; SPIR-V ; Version: 1.3 diff --git a/test/tint/bug/tint/1369.wgsl.expected.wgsl b/test/tint/bug/tint/1369.wgsl.expected.wgsl index 818f5a33fa..5b3ca83259 100644 --- a/test/tint/bug/tint/1369.wgsl.expected.wgsl +++ b/test/tint/bug/tint/1369.wgsl.expected.wgsl @@ -2,9 +2,9 @@ bug/tint/1369.wgsl:3:3 warning: code is unreachable return true; ^^^^^^ -bug/tint/1369.wgsl:9:9 warning: code is unreachable +bug/tint/1369.wgsl:9:5 warning: code is unreachable var also_unreachable : bool; - ^^^^^^^^^^^^^^^^ + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ fn call_discard() -> bool { discard; diff --git a/test/tint/bug/tint/1474-b.wgsl.expected.dxc.hlsl b/test/tint/bug/tint/1474-b.wgsl.expected.dxc.hlsl index 7a78805707..3b0cc0cf0d 100644 --- a/test/tint/bug/tint/1474-b.wgsl.expected.dxc.hlsl +++ b/test/tint/bug/tint/1474-b.wgsl.expected.dxc.hlsl @@ -1,6 +1,6 @@ -bug/tint/1474-b.wgsl:7:9 warning: code is unreachable +bug/tint/1474-b.wgsl:7:5 warning: code is unreachable let non_uniform_cond = non_uniform_value == 0; - ^^^^^^^^^^^^^^^^ + ^^^^^^^^^^^^^^^^^^^^ RWByteAddressBuffer non_uniform_value : register(u0, space0); diff --git a/test/tint/bug/tint/1474-b.wgsl.expected.fxc.hlsl b/test/tint/bug/tint/1474-b.wgsl.expected.fxc.hlsl index 7a78805707..3b0cc0cf0d 100644 --- a/test/tint/bug/tint/1474-b.wgsl.expected.fxc.hlsl +++ b/test/tint/bug/tint/1474-b.wgsl.expected.fxc.hlsl @@ -1,6 +1,6 @@ -bug/tint/1474-b.wgsl:7:9 warning: code is unreachable +bug/tint/1474-b.wgsl:7:5 warning: code is unreachable let non_uniform_cond = non_uniform_value == 0; - ^^^^^^^^^^^^^^^^ + ^^^^^^^^^^^^^^^^^^^^ RWByteAddressBuffer non_uniform_value : register(u0, space0); diff --git a/test/tint/bug/tint/1474-b.wgsl.expected.glsl b/test/tint/bug/tint/1474-b.wgsl.expected.glsl index ba0b9bd5f6..17a64ce519 100644 --- a/test/tint/bug/tint/1474-b.wgsl.expected.glsl +++ b/test/tint/bug/tint/1474-b.wgsl.expected.glsl @@ -1,6 +1,6 @@ -bug/tint/1474-b.wgsl:7:9 warning: code is unreachable +bug/tint/1474-b.wgsl:7:5 warning: code is unreachable let non_uniform_cond = non_uniform_value == 0; - ^^^^^^^^^^^^^^^^ + ^^^^^^^^^^^^^^^^^^^^ #version 310 es diff --git a/test/tint/bug/tint/1474-b.wgsl.expected.msl b/test/tint/bug/tint/1474-b.wgsl.expected.msl index 02e9750ad8..d76c784161 100644 --- a/test/tint/bug/tint/1474-b.wgsl.expected.msl +++ b/test/tint/bug/tint/1474-b.wgsl.expected.msl @@ -1,6 +1,6 @@ -bug/tint/1474-b.wgsl:7:9 warning: code is unreachable +bug/tint/1474-b.wgsl:7:5 warning: code is unreachable let non_uniform_cond = non_uniform_value == 0; - ^^^^^^^^^^^^^^^^ + ^^^^^^^^^^^^^^^^^^^^ #include diff --git a/test/tint/bug/tint/1474-b.wgsl.expected.spvasm b/test/tint/bug/tint/1474-b.wgsl.expected.spvasm index 034c79a989..ada4b0431f 100644 --- a/test/tint/bug/tint/1474-b.wgsl.expected.spvasm +++ b/test/tint/bug/tint/1474-b.wgsl.expected.spvasm @@ -1,6 +1,6 @@ -bug/tint/1474-b.wgsl:7:9 warning: code is unreachable +bug/tint/1474-b.wgsl:7:5 warning: code is unreachable let non_uniform_cond = non_uniform_value == 0; - ^^^^^^^^^^^^^^^^ + ^^^^^^^^^^^^^^^^^^^^ ; SPIR-V ; Version: 1.3 diff --git a/test/tint/bug/tint/1474-b.wgsl.expected.wgsl b/test/tint/bug/tint/1474-b.wgsl.expected.wgsl index de5fa51166..3429d58dc6 100644 --- a/test/tint/bug/tint/1474-b.wgsl.expected.wgsl +++ b/test/tint/bug/tint/1474-b.wgsl.expected.wgsl @@ -1,6 +1,6 @@ -bug/tint/1474-b.wgsl:7:9 warning: code is unreachable +bug/tint/1474-b.wgsl:7:5 warning: code is unreachable let non_uniform_cond = non_uniform_value == 0; - ^^^^^^^^^^^^^^^^ + ^^^^^^^^^^^^^^^^^^^^ @group(0) @binding(0) var non_uniform_value : i32;