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 <noreply+kokoro@google.com>
Reviewed-by: David Neto <dneto@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>
diff --git a/src/tint/reader/wgsl/parser_impl.cc b/src/tint/reader/wgsl/parser_impl.cc
index 62352ee..976706a 100644
--- a/src/tint/reader/wgsl/parser_impl.cc
+++ b/src/tint/reader/wgsl/parser_impl.cc
@@ -154,21 +154,26 @@
/// 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 @@
// | LET optionally_typed_ident EQUAL expression
// | CONST optionally_typed_ident EQUAL expression
Maybe<const ast::VariableDeclStatement*> 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 @@
return add_error(peek(), "missing initializer for 'const' declaration");
}
- auto* const_ = create<ast::Const>(decl->source, // source
- builder_.Symbols().Register(decl->name), // symbol
- decl->type, // type
- initializer.value, // initializer
- utils::Empty); // attributes
+ auto* const_ = create<ast::Const>(typed_ident->source, // source
+ builder_.Symbols().Register(typed_ident->name), // symbol
+ typed_ident->type, // type
+ initializer.value, // initializer
+ utils::Empty); // attributes
- return create<ast::VariableDeclStatement>(decl->source, const_);
+ return create<ast::VariableDeclStatement>(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 @@
return add_error(peek(), "missing initializer for 'let' declaration");
}
- auto* let = create<ast::Let>(decl->source, // source
- builder_.Symbols().Register(decl->name), // symbol
- decl->type, // type
- initializer.value, // initializer
- utils::Empty); // attributes
+ auto* let = create<ast::Let>(typed_ident->source, // source
+ builder_.Symbols().Register(typed_ident->name), // symbol
+ typed_ident->type, // type
+ initializer.value, // initializer
+ utils::Empty); // attributes
- return create<ast::VariableDeclStatement>(decl->source, let);
+ return create<ast::VariableDeclStatement>(decl_source, let);
}
auto decl = variable_decl();
@@ -1963,6 +1973,8 @@
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 @@
initializer = initializer_expr.value;
}
- auto* var = create<ast::Var>(decl->source, // source
+ auto* var = create<ast::Var>(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 703eb57..05f2976 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 @@
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 @@
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<ast::LiteralExpression>());
@@ -147,10 +147,10 @@
ASSERT_NE(e.value, nullptr);
ASSERT_TRUE(e->Is<ast::VariableDeclStatement>());
- 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 0d6bfe6..f96af05 100644
--- a/src/tint/resolver/validator.cc
+++ b/src/tint/resolver/validator.cc
@@ -1382,6 +1382,14 @@
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<ast::VariableDeclStatement>(stmt->Declaration())) {
+ if (decl->variable->Is<ast::Const>()) {
+ 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 d302264..964c8d9 100644
--- a/src/tint/resolver/variable_validation_test.cc
+++ b/src/tint/resolver/variable_validation_test.cc
@@ -417,10 +417,8 @@
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 @@
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 d0fffc5..0981f38 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 @@
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 d0fffc5..0981f38 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 @@
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 dbecc85..82b1904 100644
--- a/test/tint/bug/tint/1369.wgsl.expected.glsl
+++ b/test/tint/bug/tint/1369.wgsl.expected.glsl
@@ -2,9 +2,9 @@
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 4fc75b1..32624c3 100644
--- a/test/tint/bug/tint/1369.wgsl.expected.msl
+++ b/test/tint/bug/tint/1369.wgsl.expected.msl
@@ -2,9 +2,9 @@
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 <metal_stdlib>
diff --git a/test/tint/bug/tint/1369.wgsl.expected.spvasm b/test/tint/bug/tint/1369.wgsl.expected.spvasm
index f901311..803df82 100644
--- a/test/tint/bug/tint/1369.wgsl.expected.spvasm
+++ b/test/tint/bug/tint/1369.wgsl.expected.spvasm
@@ -2,9 +2,9 @@
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 818f5a3..5b3ca83 100644
--- a/test/tint/bug/tint/1369.wgsl.expected.wgsl
+++ b/test/tint/bug/tint/1369.wgsl.expected.wgsl
@@ -2,9 +2,9 @@
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 7a78805..3b0cc0c 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 7a78805..3b0cc0c 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 ba0b9bd..17a64ce 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 02e9750..d76c784 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 <metal_stdlib>
diff --git a/test/tint/bug/tint/1474-b.wgsl.expected.spvasm b/test/tint/bug/tint/1474-b.wgsl.expected.spvasm
index 034c79a..ada4b04 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 de5fa51..3429d58 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<storage, read_write> non_uniform_value : i32;
