Resolver: clean up how NamedTypes are handled

Also add validation for when a named type is declared more than once.

Bug: tint:803
Change-Id: Ifa93b34bc5afd31eba9bfdc4514791ec07c767ec
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/51622
Commit-Queue: Ben Clayton <bclayton@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Ben Clayton <bclayton@chromium.org>

src/resolver/resolver.cc

@@ -212,28 +212,12 @@ bool Resolver::IsHostShareable(const sem::Type* type) {
 bool Resolver::ResolveInternal() {
   Mark(&builder_->AST());
 
-  auto register_named_type = [this](Symbol name, sem::Type* type,
-                                    const Source& source) {
-    auto added = named_types_.emplace(name, type).second;
-    if (!added) {
-      diagnostics_.add_error("type with the name '" +
-                                 builder_->Symbols().NameFor(name) +
-                                 "' was already declared",
-                             source);
-      return false;
-    }
-    return true;
-  };
-
   // Process everything else in the order they appear in the module. This is
   // necessary for validation of use-before-declaration.
   for (auto* decl : builder_->AST().GlobalDeclarations()) {
     if (auto* ty = decl->As<ast::NamedType>()) {
-      auto* sem_ty = Type(ty);
-      if (sem_ty == nullptr) {
-        return false;
-      }
-      if (!register_named_type(ty->name(), sem_ty, ty->source())) {
+      Mark(ty);
+      if (!NamedType(ty)) {
         return false;
       }
     } else if (auto* func = decl->As<ast::Function>()) {
@@ -299,14 +283,6 @@ sem::Type* Resolver::Type(const ast::Type* ty) {
     if (ty->Is<ast::F32>()) {
       return builder_->create<sem::F32>();
     }
-    if (auto* t = ty->As<ast::Alias>()) {
-      auto added = name_to_ast_type_.emplace(t->name(), t->type()).second;
-      // TODO(crbug.com/tint/803): Remove this.
-      if (!added) {
-        return nullptr;
-      }
-      return Type(t->type());
-    }
     if (auto* t = ty->As<ast::AccessControl>()) {
       TINT_SCOPED_ASSIGNMENT(curent_access_control_, t);
       if (auto* el = Type(t->type())) {
@@ -339,9 +315,6 @@ sem::Type* Resolver::Type(const ast::Type* ty) {
       }
       return nullptr;
     }
-    if (auto* t = ty->As<ast::Struct>()) {
-      return Structure(t);
-    }
     if (auto* t = ty->As<ast::Sampler>()) {
       return builder_->create<sem::Sampler>(t->kind());
     }
@@ -383,14 +356,14 @@ sem::Type* Resolver::Type(const ast::Type* ty) {
       return builder_->create<sem::ExternalTexture>();
     }
     if (auto* t = ty->As<ast::TypeName>()) {
-      auto it = named_types_.find(t->name());
-      if (it == named_types_.end()) {
+      auto it = named_type_info_.find(t->name());
+      if (it == named_type_info_.end()) {
         diagnostics_.add_error(
             "unknown type '" + builder_->Symbols().NameFor(t->name()) + "'",
             t->source());
         return nullptr;
       }
-      return it->second;
+      return it->second.sem;
     }
     TINT_UNREACHABLE(diagnostics_)
         << "Unhandled ast::Type: " << ty->TypeInfo().name;
@@ -488,19 +461,23 @@ Resolver::VariableInfo* Resolver::Variable(ast::Variable* var,
 
   // TODO(crbug.com/tint/802): Temporary while ast::AccessControl exits.
   auto find_first_access_control =
-      [this](ast::Type* ty) -> ast::AccessControl* {
+      [this](const ast::Type* ty) -> const ast::AccessControl* {
     if (ty == nullptr) {
       return nullptr;
     }
-    if (ast::AccessControl* ac = ty->As<ast::AccessControl>()) {
+    if (const ast::AccessControl* ac = ty->As<ast::AccessControl>()) {
       return ac;
     }
     while (auto* tn = ty->As<ast::TypeName>()) {
-      auto it = name_to_ast_type_.find(tn->name());
-      if (it == name_to_ast_type_.end()) {
+      auto it = named_type_info_.find(tn->name());
+      if (it == named_type_info_.end()) {
         break;
       }
-      ty = it->second;
+      auto* alias = it->second.ast->As<ast::Alias>();
+      if (!alias) {
+        break;
+      }
+      ty = alias->type();
       if (auto* ac = ty->As<ast::AccessControl>()) {
         return ac;
       }
@@ -2425,6 +2402,47 @@ bool Resolver::VariableDeclStatement(const ast::VariableDeclStatement* stmt) {
   return true;
 }
 
+sem::Type* Resolver::NamedType(const ast::NamedType* named_type) {
+  sem::Type* result = nullptr;
+  if (auto* alias = named_type->As<ast::Alias>()) {
+    result = Type(alias->type());
+  } else if (auto* str = named_type->As<ast::Struct>()) {
+    result = Structure(str);
+  } else {
+    TINT_UNREACHABLE(diagnostics_) << "Unhandled NamedType";
+  }
+
+  if (!result) {
+    return nullptr;
+  }
+
+  named_type_info_.emplace(named_type->name(),
+                           NamedTypeInfo{named_type, result});
+
+  if (!ValidateNamedType(named_type)) {
+    return nullptr;
+  }
+
+  builder_->Sem().Add(named_type, result);
+  return result;
+}
+
+bool Resolver::ValidateNamedType(const ast::NamedType* named_type) const {
+  auto iter = named_type_info_.find(named_type->name());
+  if (iter == named_type_info_.end()) {
+    TINT_ICE(diagnostics_) << "ValidateNamedType called() before NamedType()";
+  }
+  if (iter->second.ast != named_type) {
+    diagnostics_.add_error("type with the name '" +
+                               builder_->Symbols().NameFor(named_type->name()) +
+                               "' was already declared",
+                           named_type->source());
+    diagnostics_.add_note("first declared here", iter->second.ast->source());
+    return false;
+  }
+  return true;
+}
+
 sem::Type* Resolver::TypeOf(const ast::Expression* expr) {
   auto it = expr_info_.find(expr);
   if (it != expr_info_.end()) {

src/resolver/resolver.h

@@ -169,6 +169,12 @@ class Resolver {
     size_t first_continue = kNoContinue;
   };
 
+  // Structure holding information for a NamedType
+  struct NamedTypeInfo {
+    ast::NamedType const* const ast;
+    sem::Type* const sem;
+  };
+
   /// Describes the context in which a variable is declared
   enum class VariableKind { kParameter, kLocal, kGlobal };
 
@@ -247,6 +253,7 @@ class Resolver {
                                    const std::string& rhs_type_name);
   bool ValidateVectorConstructor(const ast::TypeConstructorExpression* ctor,
                                  const sem::Vector* vec_type);
+  bool ValidateNamedType(const ast::NamedType* named_type) const;
 
   /// @returns the sem::Type for the ast::Type `ty`, building it if it
   /// hasn't been constructed already. If an error is raised, nullptr is
@@ -254,6 +261,10 @@ class Resolver {
   /// @param ty the ast::Type
   sem::Type* Type(const ast::Type* ty);
 
+  /// @param named_type the named type to resolve
+  /// @returns the resolved semantic type
+  sem::Type* NamedType(const ast::NamedType* named_type);
+
   /// Builds and returns the semantic information for the array `arr`.
   /// This method does not mark the ast::Array node, nor attach the generated
   /// semantic information to the AST node.
@@ -358,10 +369,10 @@ class Resolver {
   std::unordered_map<const ast::Variable*, VariableInfo*> variable_to_info_;
   std::unordered_map<ast::CallExpression*, FunctionCallInfo> function_calls_;
   std::unordered_map<const ast::Expression*, ExpressionInfo> expr_info_;
-  std::unordered_map<Symbol, sem::Type*> named_types_;
+  std::unordered_map<Symbol, NamedTypeInfo> named_type_info_;
+
   std::unordered_set<const ast::Node*> marked_;
   std::unordered_map<uint32_t, const VariableInfo*> constant_ids_;
-  std::unordered_map<Symbol, ast::Type*> name_to_ast_type_;
 
   FunctionInfo* current_function_ = nullptr;
   sem::Statement* current_statement_ = nullptr;

src/resolver/resolver_test.cc

@@ -277,6 +277,19 @@ TEST_F(ResolverTest, Stmt_VariableDecl_Alias) {
   EXPECT_TRUE(TypeOf(init)->Is<sem::I32>());
 }
 
+TEST_F(ResolverTest, Stmt_VariableDecl_AliasRedeclared) {
+  auto* my_int1 = ty.alias(Source{{12, 34}}, "MyInt", ty.i32());
+  auto* my_int2 = ty.alias(Source{{56, 78}}, "MyInt", ty.i32());
+  AST().AddConstructedType(my_int1);
+  AST().AddConstructedType(my_int2);
+  WrapInFunction();
+
+  EXPECT_FALSE(r()->Resolve());
+  EXPECT_EQ(r()->error(),
+            "56:78 error: type with the name 'MyInt' was already declared\n"
+            "12:34 note: first declared here");
+}
+
 TEST_F(ResolverTest, Stmt_VariableDecl_ModuleScope) {
   auto* init = Expr(2);
   Global("my_var", ty.i32(), ast::StorageClass::kInput, init);