From 107aa81c891a5a5938375d7bdbae028b579ebfd6 Mon Sep 17 00:00:00 2001 From: Ben Clayton Date: Thu, 23 Feb 2023 20:50:36 +0000 Subject: [PATCH] tint: Improve error for assignment to immutable variable Fixed: tint:1835 Change-Id: I48d1380fd6d9d5fdaae4a210e3c8695e26894905 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/121320 Commit-Queue: Ben Clayton Reviewed-by: Dan Sinclair Commit-Queue: Ben Clayton Kokoro: Kokoro --- .../resolver/assignment_validation_test.cc | 88 ++++++++++++++++--- .../compound_assignment_validation_test.cc | 9 +- src/tint/resolver/expression_kind_test.cc | 32 ++++--- src/tint/resolver/function_validation_test.cc | 12 --- src/tint/resolver/sem_helper.cc | 73 ++++++++------- src/tint/resolver/sem_helper.h | 4 + src/tint/resolver/validator.cc | 53 +++++++---- 7 files changed, 175 insertions(+), 96 deletions(-) diff --git a/src/tint/resolver/assignment_validation_test.cc b/src/tint/resolver/assignment_validation_test.cc index 1d0cd2769e..1c57463e1d 100644 --- a/src/tint/resolver/assignment_validation_test.cc +++ b/src/tint/resolver/assignment_validation_test.cc @@ -143,17 +143,6 @@ TEST_F(ResolverAssignmentValidationTest, AssignIncompatibleTypesInNestedBlockSta EXPECT_EQ(r()->error(), "12:34 error: cannot assign 'f32' to 'i32'"); } -TEST_F(ResolverAssignmentValidationTest, AssignToScalar_Fail) { - // var my_var : i32 = 2i; - // 1 = my_var; - - WrapInFunction(Var("my_var", ty.i32(), Expr(2_i)), // - Assign(Expr(Source{{12, 34}}, 1_i), "my_var")); - - EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "12:34 error: cannot assign to value of type 'i32'"); -} - TEST_F(ResolverAssignmentValidationTest, AssignCompatibleTypes_Pass) { // var a : i32 = 1i; // a = 2i; @@ -213,16 +202,87 @@ TEST_F(ResolverAssignmentValidationTest, AssignMaterializedThroughPointer_Pass) EXPECT_TRUE(r()->Resolve()) << r()->error(); } +TEST_F(ResolverAssignmentValidationTest, AssignToScalar_Fail) { + // var my_var : i32 = 2i; + // 1 = my_var; + + WrapInFunction(Var("my_var", ty.i32(), Expr(2_i)), // + Assign(Expr(Source{{12, 34}}, 1_i), "my_var")); + + EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ(r()->error(), R"(12:34 error: cannot assign to value expression of type 'i32')"); +} + +TEST_F(ResolverAssignmentValidationTest, AssignToOverride_Fail) { + // override a : i32 = 2i; + // { + // a = 2i + // } + Override(Source{{56, 78}}, "a", ty.i32(), Expr(2_i)); + WrapInFunction(Assign(Expr(Source{{12, 34}}, "a"), 2_i)); + + EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ(r()->error(), R"(12:34 error: cannot assign to override 'a' +12:34 note: 'override' variables are immutable +56:78 note: override 'a' declared here)"); +} + TEST_F(ResolverAssignmentValidationTest, AssignToLet_Fail) { // { // let a : i32 = 2i; // a = 2i // } - auto* var = Let("a", ty.i32(), Expr(2_i)); - WrapInFunction(var, Assign(Expr(Source{{12, 34}}, "a"), 2_i)); + WrapInFunction(Let(Source{{56, 78}}, "a", ty.i32(), Expr(2_i)), // + Assign(Expr(Source{{12, 34}}, "a"), 2_i)); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "12:34 error: cannot assign to 'let'\nnote: 'a' is declared here:"); + EXPECT_EQ(r()->error(), R"(12:34 error: cannot assign to let 'a' +12:34 note: 'let' variables are immutable +56:78 note: let 'a' declared here)"); +} + +TEST_F(ResolverAssignmentValidationTest, AssignToConst_Fail) { + // { + // const a : i32 = 2i; + // a = 2i + // } + WrapInFunction(Const(Source{{56, 78}}, "a", ty.i32(), Expr(2_i)), // + Assign(Expr(Source{{12, 34}}, "a"), 2_i)); + + EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ(r()->error(), R"(12:34 error: cannot assign to const 'a' +12:34 note: 'const' variables are immutable +56:78 note: const 'a' declared here)"); +} + +TEST_F(ResolverAssignmentValidationTest, AssignToParam_Fail) { + Func("foo", utils::Vector{Param(Source{{56, 78}}, "arg", ty.i32())}, ty.void_(), + utils::Vector{ + Assign(Expr(Source{{12, 34}}, "arg"), Expr(1_i)), + Return(), + }); + + EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ(r()->error(), + R"(12:34 error: cannot assign to parameter 'arg' +12:34 note: parameters are immutable +56:78 note: parameter 'arg' declared here)"); +} + +TEST_F(ResolverAssignmentValidationTest, AssignToLetMember_Fail) { + // struct S { i : i32 } + // { + // let a : S; + // a.i = 2i + // } + Structure("S", utils::Vector{Member("i", ty.i32())}); + WrapInFunction(Let(Source{{98, 76}}, "a", ty("S"), Call("S")), // + Assign(MemberAccessor(Source{{12, 34}}, Expr(Source{{56, 78}}, "a"), "i"), 2_i)); + + EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ(r()->error(), R"(12:34 error: cannot assign to value expression of type 'i32' +56:78 note: 'let' variables are immutable +98:76 note: let 'a' declared here)"); } TEST_F(ResolverAssignmentValidationTest, AssignNonConstructible_Handle) { diff --git a/src/tint/resolver/compound_assignment_validation_test.cc b/src/tint/resolver/compound_assignment_validation_test.cc index cb42adee14..ee18de35d5 100644 --- a/src/tint/resolver/compound_assignment_validation_test.cc +++ b/src/tint/resolver/compound_assignment_validation_test.cc @@ -242,15 +242,16 @@ TEST_F(ResolverCompoundAssignmentValidationTest, ReadOnlyBuffer) { "56:78 error: cannot store into a read-only type 'ref'"); } -TEST_F(ResolverCompoundAssignmentValidationTest, LhsConstant) { +TEST_F(ResolverCompoundAssignmentValidationTest, LhsLet) { // let a = 1i; // a += 1i; auto* a = Let(Source{{12, 34}}, "a", Expr(1_i)); 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 'let' -12:34 note: 'a' is declared here:)"); + EXPECT_EQ(r()->error(), R"(56:78 error: cannot assign to let 'a' +56:78 note: 'let' variables are immutable +12:34 note: let 'a' declared here)"); } TEST_F(ResolverCompoundAssignmentValidationTest, LhsLiteral) { @@ -258,7 +259,7 @@ TEST_F(ResolverCompoundAssignmentValidationTest, LhsLiteral) { WrapInFunction(CompoundAssign(Expr(Source{{56, 78}}, 1_i), 1_i, ast::BinaryOp::kAdd)); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "56:78 error: cannot assign to value of type 'i32'"); + EXPECT_EQ(r()->error(), R"(56:78 error: cannot assign to value expression of type 'i32')"); } TEST_F(ResolverCompoundAssignmentValidationTest, LhsAtomic) { diff --git a/src/tint/resolver/expression_kind_test.cc b/src/tint/resolver/expression_kind_test.cc index 0b4584d2b1..2ceffa2c50 100644 --- a/src/tint/resolver/expression_kind_test.cc +++ b/src/tint/resolver/expression_kind_test.cc @@ -598,30 +598,36 @@ INSTANTIATE_TEST_SUITE_P( 1:2 note: parameter 'PARAMETER' declared here)"}, {Def::kParameter, Use::kUnaryOp, kPass}, - {Def::kStruct, Use::kAccess, R"(5:6 error: cannot use type 'STRUCT' as access)"}, - {Def::kStruct, Use::kAddressSpace, - R"(5:6 error: cannot use type 'STRUCT' as address space)"}, - {Def::kStruct, Use::kBinaryOp, R"(5:6 error: cannot use type 'STRUCT' as value -7:8 note: are you missing '()' for value constructor? + {Def::kStruct, Use::kAccess, R"(5:6 error: cannot use type 'STRUCT' as access 1:2 note: struct 'STRUCT' declared here)"}, + {Def::kStruct, Use::kAddressSpace, + R"(5:6 error: cannot use type 'STRUCT' as address space +1:2 note: struct 'STRUCT' declared here)"}, + {Def::kStruct, Use::kBinaryOp, R"(5:6 error: cannot use type 'STRUCT' as value +1:2 note: struct 'STRUCT' declared here +7:8 note: are you missing '()' for value constructor?)"}, {Def::kStruct, Use::kBuiltinValue, - R"(5:6 error: cannot use type 'STRUCT' as builtin value)"}, + R"(5:6 error: cannot use type 'STRUCT' as builtin value +1:2 note: struct 'STRUCT' declared here)"}, {Def::kStruct, Use::kFunctionReturnType, kPass}, {Def::kStruct, Use::kInterpolationSampling, - R"(5:6 error: cannot use type 'STRUCT' as interpolation sampling)"}, + R"(5:6 error: cannot use type 'STRUCT' as interpolation sampling +1:2 note: struct 'STRUCT' declared here)"}, {Def::kStruct, Use::kInterpolationType, - R"(5:6 error: cannot use type 'STRUCT' as interpolation type)"}, + R"(5:6 error: cannot use type 'STRUCT' as interpolation type +1:2 note: struct 'STRUCT' declared here)"}, {Def::kStruct, Use::kMemberType, kPass}, - {Def::kStruct, Use::kTexelFormat, R"(5:6 error: cannot use type 'STRUCT' as texel format)"}, + {Def::kStruct, Use::kTexelFormat, R"(5:6 error: cannot use type 'STRUCT' as texel format +1:2 note: struct 'STRUCT' declared here)"}, {Def::kStruct, Use::kValueExpression, R"(5:6 error: cannot use type 'STRUCT' as value -7:8 note: are you missing '()' for value constructor? -1:2 note: struct 'STRUCT' declared here)"}, +1:2 note: struct 'STRUCT' declared here +7:8 note: are you missing '()' for value constructor?)"}, {Def::kStruct, Use::kVariableType, kPass}, {Def::kStruct, Use::kUnaryOp, R"(5:6 error: cannot use type 'STRUCT' as value -7:8 note: are you missing '()' for value constructor? -1:2 note: struct 'STRUCT' declared here)"}, +1:2 note: struct 'STRUCT' declared here +7:8 note: are you missing '()' for value constructor?)"}, {Def::kTexelFormat, Use::kAccess, R"(5:6 error: cannot use texel format 'rgba8unorm' as access)"}, diff --git a/src/tint/resolver/function_validation_test.cc b/src/tint/resolver/function_validation_test.cc index b18ff2a74f..3a0275feef 100644 --- a/src/tint/resolver/function_validation_test.cc +++ b/src/tint/resolver/function_validation_test.cc @@ -487,18 +487,6 @@ TEST_F(ResolverFunctionValidationTest, FunctionConstInitWithParam) { ASSERT_TRUE(r()->Resolve()) << r()->error(); } -TEST_F(ResolverFunctionValidationTest, FunctionParamsConst) { - Func("foo", utils::Vector{Param(Sym("arg"), ty.i32())}, ty.void_(), - utils::Vector{ - Assign(Expr(Source{{12, 34}}, "arg"), Expr(1_i)), - Return(), - }); - - EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), - "12:34 error: cannot assign to function parameter\nnote: 'arg' is declared here:"); -} - TEST_F(ResolverFunctionValidationTest, WorkgroupSize_GoodType_ConstU32) { // const x = 4u; // const y = 8u; diff --git a/src/tint/resolver/sem_helper.cc b/src/tint/resolver/sem_helper.cc index d11d790cf7..45bdf4156c 100644 --- a/src/tint/resolver/sem_helper.cc +++ b/src/tint/resolver/sem_helper.cc @@ -39,77 +39,67 @@ type::Type* SemHelper::TypeOf(const ast::Expression* expr) const { return sem ? const_cast(sem->Type()) : nullptr; } -void SemHelper::ErrorUnexpectedExprKind(const sem::Expression* expr, - std::string_view wanted) const { - Switch( +std::string SemHelper::Describe(const sem::Expression* expr) const { + return Switch( expr, // [&](const sem::VariableUser* var_expr) { auto* variable = var_expr->Variable()->Declaration(); auto name = builder_->Symbols().NameFor(variable->name->symbol); - std::string kind = Switch( + auto* kind = Switch( variable, // [&](const ast::Var*) { return "var"; }, // + [&](const ast::Let*) { return "let"; }, // [&](const ast::Const*) { return "const"; }, // [&](const ast::Parameter*) { return "parameter"; }, // [&](const ast::Override*) { return "override"; }, // [&](Default) { return "variable"; }); - AddError("cannot use " + kind + " '" + name + "' as " + std::string(wanted), - var_expr->Declaration()->source); - NoteDeclarationSource(variable); + return std::string(kind) + " '" + name + "'"; }, [&](const sem::ValueExpression* val_expr) { auto type = val_expr->Type()->FriendlyName(builder_->Symbols()); - AddError("cannot use expression of type '" + type + "' as " + std::string(wanted), - val_expr->Declaration()->source); + return "value expression of type '" + type + "'"; }, [&](const sem::TypeExpression* ty_expr) { auto name = ty_expr->Type()->FriendlyName(builder_->Symbols()); - AddError("cannot use type '" + name + "' as " + std::string(wanted), - ty_expr->Declaration()->source); + return "type '" + name + "'"; }, [&](const sem::FunctionExpression* fn_expr) { auto* fn = fn_expr->Function()->Declaration(); auto name = builder_->Symbols().NameFor(fn->name->symbol); - AddError("cannot use function '" + name + "' as " + std::string(wanted), - fn_expr->Declaration()->source); - NoteDeclarationSource(fn); + return "function '" + name + "'"; }, [&](const sem::BuiltinEnumExpression* access) { - AddError("cannot use access '" + utils::ToString(access->Value()) + "' as " + - std::string(wanted), - access->Declaration()->source); + return "access '" + utils::ToString(access->Value()) + "'"; }, [&](const sem::BuiltinEnumExpression* addr) { - AddError("cannot use address space '" + utils::ToString(addr->Value()) + "' as " + - std::string(wanted), - addr->Declaration()->source); + return "address space '" + utils::ToString(addr->Value()) + "'"; }, [&](const sem::BuiltinEnumExpression* builtin) { - AddError("cannot use builtin value '" + utils::ToString(builtin->Value()) + "' as " + - std::string(wanted), - builtin->Declaration()->source); + return "builtin value '" + utils::ToString(builtin->Value()) + "'"; }, [&](const sem::BuiltinEnumExpression* fmt) { - AddError("cannot use interpolation sampling '" + utils::ToString(fmt->Value()) + - "' as " + std::string(wanted), - fmt->Declaration()->source); + return "interpolation sampling '" + utils::ToString(fmt->Value()) + "'"; }, [&](const sem::BuiltinEnumExpression* fmt) { - AddError("cannot use interpolation type '" + utils::ToString(fmt->Value()) + "' as " + - std::string(wanted), - fmt->Declaration()->source); + return "interpolation type '" + utils::ToString(fmt->Value()) + "'"; }, [&](const sem::BuiltinEnumExpression* fmt) { - AddError("cannot use texel format '" + utils::ToString(fmt->Value()) + "' as " + - std::string(wanted), - fmt->Declaration()->source); + return "texel format '" + utils::ToString(fmt->Value()) + "'"; }, - [&](Default) { + [&](Default) -> std::string { TINT_ICE(Resolver, builder_->Diagnostics()) << "unhandled sem::Expression type: " << (expr ? expr->TypeInfo().name : ""); + return ""; }); } +void SemHelper::ErrorUnexpectedExprKind(const sem::Expression* expr, + std::string_view wanted) const { + AddError("cannot use " + Describe(expr) + " as " + std::string(wanted), + expr->Declaration()->source); + NoteDeclarationSource(expr->Declaration()); +} + void SemHelper::ErrorExpectedValueExpr(const sem::Expression* expr) const { ErrorUnexpectedExprKind(expr, "value"); if (auto* ty_expr = expr->As()) { @@ -117,14 +107,23 @@ void SemHelper::ErrorExpectedValueExpr(const sem::Expression* expr) const { AddNote("are you missing '()' for value constructor?", Source{{ident->source.range.end}}); } - if (auto* str = ty_expr->Type()->As()) { - AddNote("struct '" + str->FriendlyName(builder_->Symbols()) + "' declared here", - str->Source()); - } } } void SemHelper::NoteDeclarationSource(const ast::Node* node) const { + if (!node) { + return; + } + + Switch( + Get(node), // + [&](const sem::VariableUser* var_expr) { node = var_expr->Variable()->Declaration(); }, + [&](const sem::TypeExpression* ty_expr) { + Switch(ty_expr->Type(), // + [&](const sem::Struct* s) { node = s->Declaration(); }); + }, + [&](const sem::FunctionExpression* fn_expr) { node = fn_expr->Function()->Declaration(); }); + Switch( node, [&](const ast::Struct* n) { diff --git a/src/tint/resolver/sem_helper.h b/src/tint/resolver/sem_helper.h index 2ba85023e1..514367b6c1 100644 --- a/src/tint/resolver/sem_helper.h +++ b/src/tint/resolver/sem_helper.h @@ -226,6 +226,10 @@ class SemHelper { /// @param node the AST node. void NoteDeclarationSource(const ast::Node* node) const; + /// @param expr the expression to describe + /// @return a string that describes @p expr. Useful for diagnostics. + std::string Describe(const sem::Expression* expr) const; + private: /// Adds the given error message to the diagnostics void AddError(const std::string& msg, const Source& source) const; diff --git a/src/tint/resolver/validator.cc b/src/tint/resolver/validator.cc index 952213287e..b272b15ab1 100644 --- a/src/tint/resolver/validator.cc +++ b/src/tint/resolver/validator.cc @@ -2391,26 +2391,47 @@ bool Validator::Assignment(const ast::Statement* a, const type::Type* rhs_ty) co } // https://gpuweb.github.io/gpuweb/wgsl/#assignment-statement - auto const* lhs_ty = sem_.TypeOf(lhs); - - if (auto* var_user = sem_.Get(lhs)) { - auto* v = var_user->Variable()->Declaration(); - const char* err = Switch( - v, // - [&](const ast::Parameter*) { return "cannot assign to function parameter"; }, - [&](const ast::Let*) { return "cannot assign to 'let'"; }, - [&](const ast::Override*) { return "cannot assign to 'override'"; }); - if (err) { - AddError(err, lhs->source); - AddNote("'" + symbols_.NameFor(v->name->symbol) + "' is declared here:", v->source); - return false; - } - } + auto const* lhs_sem = sem_.GetVal(lhs); + auto const* lhs_ty = lhs_sem->Type(); auto* lhs_ref = lhs_ty->As(); if (!lhs_ref) { // LHS is not a reference, so it has no storage. - AddError("cannot assign to value of type '" + sem_.TypeNameOf(lhs_ty) + "'", lhs->source); + AddError("cannot assign to " + sem_.Describe(lhs_sem), lhs->source); + + auto* expr = lhs; + while (expr) { + expr = Switch( + expr, // + [&](const ast::AccessorExpression* e) { return e->object; }, + [&](const ast::IdentifierExpression* i) { + if (auto user = sem_.Get(i)) { + Switch( + user->Variable()->Declaration(), // + [&](const ast::Let* v) { + AddNote("'let' variables are immutable", + user->Declaration()->source); + sem_.NoteDeclarationSource(v); + }, + [&](const ast::Const* v) { + AddNote("'const' variables are immutable", + user->Declaration()->source); + sem_.NoteDeclarationSource(v); + }, + [&](const ast::Override* v) { + AddNote("'override' variables are immutable", + user->Declaration()->source); + sem_.NoteDeclarationSource(v); + }, + [&](const ast::Parameter* v) { + AddNote("parameters are immutable", user->Declaration()->source); + sem_.NoteDeclarationSource(v); + }); + } + return nullptr; + }); + } + return false; }