Resolver: Enforce vector constructor type rules

Added enforcement for vector constructor type rules according to the
table in https://gpuweb.github.io/gpuweb/wgsl.html#type-constructor-expr.

This surfaced a number of existing tests that violated some of these
rules or had a type-declaration related bug, so this CL fixes those as
well (these tests either passed the incorrect number of arguments to a
vector constructor or relied on implicit conversions between numeric
types).

Fixed: tint:632
Fixed: tint:476
Change-Id: I8279be3eeae50b64db486ee7a91a43bd94fdff62
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/44480
Commit-Queue: Arman Uguray <armansito@chromium.org>
Reviewed-by: Ben Clayton <bclayton@google.com>

src/ast/module_clone_test.cc

@@ -65,7 +65,7 @@ fn f1(p0 : f32, p1 : i32) -> f32 {
   var l0 : i32 = 3;
   var l1 : f32 = 8.0;
   var l2 : u32 = bitcast<u32>(4);
-  var l3 : vec2<u32> = vec2<u32>(l0, l1);
+  var l3 : vec2<u32> = vec2<u32>(u32(l0), u32(l1));
   var l4 : S;
   var l5 : u32 = l4.m1[5];
   var l6 : ptr<private, u32>;

src/reader/wgsl/parser_test.cc

@@ -38,7 +38,7 @@ TEST_F(ParserTest, Parses) {
 
 [[stage(vertex)]]
 fn main() -> void {
-  gl_FragColor = vec4<f32>(.4, .2, .3, 1);
+  gl_FragColor = vec4<f32>(.4, .2, .3, 1.);
 }
 )");
   auto program = Parse(&file);

src/resolver/resolver.cc

@@ -597,16 +597,97 @@ bool Resolver::IntrinsicCall(ast::CallExpression* call,
 }
 
 bool Resolver::Constructor(ast::ConstructorExpression* expr) {
-  if (auto* ty = expr->As<ast::TypeConstructorExpression>()) {
+  if (auto* type_ctor = expr->As<ast::TypeConstructorExpression>()) {
-    for (auto* value : ty->values()) {
+    for (auto* value : type_ctor->values()) {
       if (!Expression(value)) {
         return false;
       }
     }
-    SetType(expr, ty->type());
+    SetType(expr, type_ctor->type());
+
+    // Now that the argument types have been determined, make sure that they
+    // obey the constructor type rules laid out in
+    // https://gpuweb.github.io/gpuweb/wgsl.html#type-constructor-expr.
+    if (auto* vec_type = type_ctor->type()->As<type::Vector>()) {
+      return VectorConstructor(*vec_type, type_ctor->values());
+    }
+    // TODO(crbug.com/tint/633): Validate matrix constructor
+    // TODO(crbug.com/tint/634): Validate array constructor
+  } else if (auto* scalar_ctor = expr->As<ast::ScalarConstructorExpression>()) {
+    SetType(expr, scalar_ctor->literal()->type());
   } else {
-    SetType(expr,
+    TINT_ICE(diagnostics_) << "unexpected constructor expression type";
-            expr->As<ast::ScalarConstructorExpression>()->literal()->type());
+  }
+  return true;
+}
+
+bool Resolver::VectorConstructor(const type::Vector& vec_type,
+                                 const ast::ExpressionList& values) {
+  type::Type* elem_type = vec_type.type()->UnwrapAll();
+  size_t value_cardinality_sum = 0;
+  for (auto* value : values) {
+    type::Type* value_type = TypeOf(value)->UnwrapAll();
+    if (value_type->is_scalar()) {
+      if (elem_type != value_type) {
+        diagnostics_.add_error(
+            "type in vector constructor does not match vector type: "
+            "expected '" +
+                elem_type->FriendlyName(builder_->Symbols()) + "', found '" +
+                value_type->FriendlyName(builder_->Symbols()) + "'",
+            value->source());
+        return false;
+      }
+
+      value_cardinality_sum++;
+    } else if (auto* value_vec = value_type->As<type::Vector>()) {
+      type::Type* value_elem_type = value_vec->type()->UnwrapAll();
+      // A mismatch of vector type parameter T is only an error if multiple
+      // arguments are present. A single argument constructor constitutes a
+      // type conversion expression.
+      // NOTE: A conversion expression from a vec<bool> to any other vecN<T>
+      // is disallowed (see
+      // https://gpuweb.github.io/gpuweb/wgsl.html#conversion-expr).
+      if (elem_type != value_elem_type &&
+          (values.size() > 1u || value_vec->is_bool_vector())) {
+        diagnostics_.add_error(
+            "type in vector constructor does not match vector type: "
+            "expected '" +
+                elem_type->FriendlyName(builder_->Symbols()) + "', found '" +
+                value_elem_type->FriendlyName(builder_->Symbols()) + "'",
+            value->source());
+        return false;
+      }
+
+      value_cardinality_sum += value_vec->size();
+    } else {
+      // A vector constructor can only accept vectors and scalars.
+      diagnostics_.add_error(
+          "expected vector or scalar type in vector constructor; found: " +
+              value_type->FriendlyName(builder_->Symbols()),
+          value->source());
+      return false;
+    }
+  }
+
+  // A correct vector constructor must either be a zero-value expression
+  // or the number of components of all constructor arguments must add up
+  // to the vector cardinality.
+  if (value_cardinality_sum > 0 && value_cardinality_sum != vec_type.size()) {
+    if (values.empty()) {
+      TINT_ICE(diagnostics_)
+          << "constructor arguments expected to be non-empty!";
+    }
+    const Source& values_start = values[0]->source();
+    const Source& values_end = values[values.size() - 1]->source();
+    const Source src(
+        Source::Range(values_start.range.begin, values_end.range.end),
+        values_start.file_path, values_start.file_content);
+    diagnostics_.add_error(
+        "attempted to construct '" +
+            vec_type.FriendlyName(builder_->Symbols()) + "' with " +
+            std::to_string(value_cardinality_sum) + " component(s)",
+        src);
+    return false;
   }
   return true;
 }

src/resolver/resolver.h

@@ -177,6 +177,8 @@ class Resolver {
   bool Call(ast::CallExpression*);
   bool CaseStatement(ast::CaseStatement*);
   bool Constructor(ast::ConstructorExpression*);
+  bool VectorConstructor(const type::Vector& vec_type,
+                         const ast::ExpressionList& values);
   bool Expression(ast::Expression*);
   bool Expressions(const ast::ExpressionList&);
   bool Function(ast::Function*);

src/resolver/resolver_test.cc

@@ -578,8 +578,20 @@ TEST_F(ResolverTest, Expr_Constructor_Scalar) {
   EXPECT_TRUE(TypeOf(s)->Is<type::F32>());
 }
 
-TEST_F(ResolverTest, Expr_Constructor_Type) {
+TEST_F(ResolverTest, Expr_Constructor_Type_Vec2) {
-  auto* tc = vec3<f32>(1.0f, 1.0f, 3.0f);
+  auto* tc = vec2<f32>(1.0f, 1.0f);
+  WrapInFunction(tc);
+
+  EXPECT_TRUE(r()->Resolve()) << r()->error();
+
+  ASSERT_NE(TypeOf(tc), nullptr);
+  ASSERT_TRUE(TypeOf(tc)->Is<type::Vector>());
+  EXPECT_TRUE(TypeOf(tc)->As<type::Vector>()->type()->Is<type::F32>());
+  EXPECT_EQ(TypeOf(tc)->As<type::Vector>()->size(), 2u);
+}
+
+TEST_F(ResolverTest, Expr_Constructor_Type_Vec3) {
+  auto* tc = vec3<f32>(1.0f, 1.0f, 1.0f);
   WrapInFunction(tc);
 
   EXPECT_TRUE(r()->Resolve()) << r()->error();
@@ -590,6 +602,18 @@ TEST_F(ResolverTest, Expr_Constructor_Type) {
   EXPECT_EQ(TypeOf(tc)->As<type::Vector>()->size(), 3u);
 }
 
+TEST_F(ResolverTest, Expr_Constructor_Type_Vec4) {
+  auto* tc = vec4<f32>(1.0f, 1.0f, 1.0f, 1.0f);
+  WrapInFunction(tc);
+
+  EXPECT_TRUE(r()->Resolve()) << r()->error();
+
+  ASSERT_NE(TypeOf(tc), nullptr);
+  ASSERT_TRUE(TypeOf(tc)->Is<type::Vector>());
+  EXPECT_TRUE(TypeOf(tc)->As<type::Vector>()->type()->Is<type::F32>());
+  EXPECT_EQ(TypeOf(tc)->As<type::Vector>()->size(), 4u);
+}
+
 TEST_F(ResolverTest, Expr_Identifier_GlobalVariable) {
   auto* my_var = Global("my_var", ty.f32(), ast::StorageClass::kNone);
 

src/transform/hlsl_test.cc

@@ -108,7 +108,7 @@ struct Uniforms {
 [[stage(vertex)]]
 fn main() -> void {
   const transform : mat2x2<f32> = ubo.transform;
-  var coord : array<vec2<f32>, 3> = array<vec2<f32>, 3>(
+  var coord : vec2<f32> = array<vec2<f32>, 3>(
       vec2<f32>(-1.0,  1.0),
       vec2<f32>( 1.0,  1.0),
       vec2<f32>(-1.0, -1.0)
@@ -133,7 +133,7 @@ struct Uniforms {
 fn main() -> void {
   const transform : mat2x2<f32> = ubo.transform;
   const tint_symbol_1 : array<vec2<f32>, 3> = array<vec2<f32>, 3>(vec2<f32>(-1.0, 1.0), vec2<f32>(1.0, 1.0), vec2<f32>(-1.0, -1.0));
-  var coord : array<vec2<f32>, 3> = tint_symbol_1[vertex_index];
+  var coord : vec2<f32> = tint_symbol_1[vertex_index];
   position = vec4<f32>((transform * coord), 0.0, 1.0);
 }
 )";

src/validator/validator_builtins_test.cc

@@ -509,7 +509,7 @@ TEST_P(IntegerAllMatching, Vec2Unsigned) {
 
   ast::ExpressionList params;
   for (uint32_t i = 0; i < num_params; ++i) {
-    params.push_back(vec2<uint32_t>(1, 1));
+    params.push_back(vec2<uint32_t>(1u, 1u));
   }
   auto* builtin = Call(name, params);
   WrapInFunction(builtin);
@@ -526,7 +526,7 @@ TEST_P(IntegerAllMatching, Vec3Unsigned) {
 
   ast::ExpressionList params;
   for (uint32_t i = 0; i < num_params; ++i) {
-    params.push_back(vec3<uint32_t>(1, 1, 1));
+    params.push_back(vec3<uint32_t>(1u, 1u, 1u));
   }
   auto* builtin = Call(name, params);
   WrapInFunction(builtin);
@@ -543,7 +543,7 @@ TEST_P(IntegerAllMatching, Vec4Unsigned) {
 
   ast::ExpressionList params;
   for (uint32_t i = 0; i < num_params; ++i) {
-    params.push_back(vec4<uint32_t>(1, 1, 1, 1));
+    params.push_back(vec4<uint32_t>(1u, 1u, 1u, 1u));
   }
   auto* builtin = Call(name, params);
   WrapInFunction(builtin);

src/writer/spirv/builder_constructor_expression_test.cc

@@ -141,7 +141,7 @@ TEST_F(SpvBuilderConstructorTest, Type_IdentifierExpression_Param) {
 }
 
 TEST_F(SpvBuilderConstructorTest, Vector_Bitcast_Params) {
-  auto* t = vec2<u32>(1, 1);
+  auto* t = vec2<u32>(Construct<u32>(1), Construct<u32>(1));
   WrapInFunction(t);
 
   spirv::Builder& b = Build();
@@ -153,14 +153,14 @@ TEST_F(SpvBuilderConstructorTest, Vector_Bitcast_Params) {
 
   EXPECT_EQ(DumpInstructions(b.types()), R"(%2 = OpTypeInt 32 0
 %1 = OpTypeVector %2 2
-%3 = OpTypeInt 32 1
+%4 = OpTypeInt 32 1
-%4 = OpConstant %3 1
+%5 = OpConstant %4 1
 )");
 
   EXPECT_EQ(DumpInstructions(b.functions()[0].instructions()),
-            R"(%5 = OpBitcast %2 %4
+            R"(%3 = OpBitcast %2 %5
-%6 = OpBitcast %2 %4
+%6 = OpBitcast %2 %5
-%7 = OpCompositeConstruct %1 %5 %6
+%7 = OpCompositeConstruct %1 %3 %6
 )");
 }
 
@@ -1450,7 +1450,7 @@ TEST_F(SpvBuilderConstructorTest,
 
 TEST_F(SpvBuilderConstructorTest,
        IsConstructorConst_GlobalVectorWithMatchingTypeConstructors) {
-  // vec3<f32>(f32(1.0), f32(2.0))  -> false
+  // vec2<f32>(f32(1.0), f32(2.0))  -> false
 
   auto* t = vec2<f32>(Construct<f32>(1.f), Construct<f32>(2.f));
   WrapInFunction(t);
@@ -1463,7 +1463,7 @@ TEST_F(SpvBuilderConstructorTest,
 
 TEST_F(SpvBuilderConstructorTest,
        IsConstructorConst_GlobalWithTypeCastConstructor) {
-  // vec3<f32>(f32(1), f32(2)) -> false
+  // vec2<f32>(f32(1), f32(2)) -> false
 
   auto* t = vec2<f32>(Construct<f32>(1), Construct<f32>(2));
   WrapInFunction(t);
@@ -1521,22 +1521,10 @@ TEST_F(SpvBuilderConstructorTest,
 }
 
 TEST_F(SpvBuilderConstructorTest,
-       IsConstructorConst_VectorWith_TypeCastConstConstructors) {
+       IsConstructorConst_VectorWithTypeCastConstConstructors) {
   // vec2<f32>(f32(1), f32(2))  -> false
 
-  auto* t = vec2<f32>(1, 2);
+  auto* t = vec2<f32>(Construct<f32>(1), Construct<f32>(2));
-  WrapInFunction(t);
-
-  spirv::Builder& b = Build();
-
-  EXPECT_FALSE(b.is_constructor_const(t, false));
-  EXPECT_FALSE(b.has_error());
-}
-
-TEST_F(SpvBuilderConstructorTest, IsConstructorConst_WithTypeCastConstructor) {
-  // vec3<f32>(f32(1), f32(2)) -> false
-
-  auto* t = vec3<f32>(1, 2);
   WrapInFunction(t);
 
   spirv::Builder& b = Build();
@@ -1546,7 +1534,7 @@ TEST_F(SpvBuilderConstructorTest, IsConstructorConst_WithTypeCastConstructor) {
 }
 
 TEST_F(SpvBuilderConstructorTest, IsConstructorConst_BitCastScalars) {
-  auto* t = vec2<u32>(1, 1);
+  auto* t = vec2<u32>(Construct<u32>(1), Construct<u32>(1));
   WrapInFunction(t);
 
   spirv::Builder& b = Build();