From 7b6bcb6e0bbcf50b015ad2ebe9e2059a977f96f2 Mon Sep 17 00:00:00 2001
From: Ben Clayton <bclayton@google.com>
Date: Thu, 11 Feb 2021 20:23:03 +0000
Subject: [PATCH] Fix AST declaration order when cloning Programs

Change 41302 correctly fixed up Module::Clone(), but this wasn't actually called by the CloneContext, as Module::Clone() returns a new Module, where as the CloneContext needs to clone into an existing Module.

Refactor the code so that this duplicated logic is moved into a single Module::Copy() method.

Fixed: 1177275
Change-Id: Ia8c45ef05e03b2891b5785ee6f425dd01cb989c6
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/41542
Reviewed-by: James Price <jrprice@google.com>
Reviewed-by: dan sinclair <dsinclair@chromium.org>
Commit-Queue: dan sinclair <dsinclair@chromium.org>
---
 src/ast/module.cc            | 16 ++++++++++------
 src/ast/module.h             |  5 +++++
 src/ast/module_clone_test.cc | 16 +++++++++++++---
 src/clone_context.cc         | 10 +---------
 4 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/src/ast/module.cc b/src/ast/module.cc
index 326dd3a02f..a5c19c9852 100644
--- a/src/ast/module.cc
+++ b/src/ast/module.cc
@@ -91,20 +91,24 @@ bool Module::IsValid() const {
 }
 
 Module* Module::Clone(CloneContext* ctx) const {
-  std::vector<CastableBase*> global_decls;
-  for (auto* decl : global_declarations_) {
+  auto* out = ctx->dst->create<Module>();
+  out->Copy(ctx, this);
+  return out;
+}
+
+void Module::Copy(CloneContext* ctx, const Module* src) {
+  for (auto* decl : src->global_declarations_) {
     assert(decl);
     if (auto* ty = decl->As<type::Type>()) {
-      global_decls.push_back(ctx->Clone(ty));
+      AddConstructedType(ctx->Clone(ty));
     } else if (auto* func = decl->As<Function>()) {
-      global_decls.push_back(ctx->Clone(func));
+      AddFunction(ctx->Clone(func));
     } else if (auto* var = decl->As<Variable>()) {
-      global_decls.push_back(ctx->Clone(var));
+      AddGlobalVariable(ctx->Clone(var));
     } else {
       assert(false /* unreachable */);
     }
   }
-  return ctx->dst->create<Module>(global_decls);
 }
 
 void Module::to_str(const semantic::Info& sem,
diff --git a/src/ast/module.h b/src/ast/module.h
index 416eb80b3c..0a5470e509 100644
--- a/src/ast/module.h
+++ b/src/ast/module.h
@@ -95,6 +95,11 @@ class Module : public Castable<Module, Node> {
   /// @return the newly cloned node
   Module* Clone(CloneContext* ctx) const override;
 
+  /// Copy copies the content of the Module src into this module.
+  /// @param ctx the clone context
+  /// @param src the module to copy into this module
+  void Copy(CloneContext* ctx, const Module* src);
+
   /// Writes a representation of the node to the output stream
   /// @param sem the semantic info for the program
   /// @param out the stream to write to
diff --git a/src/ast/module_clone_test.cc b/src/ast/module_clone_test.cc
index a0c3211006..8afbf57b7d 100644
--- a/src/ast/module_clone_test.cc
+++ b/src/ast/module_clone_test.cc
@@ -38,12 +38,12 @@ struct S {
   m1 : array<u32>;
 };
 
-type t0 = [[stride(16)]] array<vec4<f32>>;
-type t1 = [[stride(32)]] array<vec4<f32>>;
-
 const c0 : i32 = 10;
 const c1 : bool = true;
 
+type t0 = [[stride(16)]] array<vec4<f32>>;
+type t1 = [[stride(32)]] array<vec4<f32>>;
+
 var<uniform> g0 : u32 = 20u;
 var<out> g1 : f32 = 123.0;
 var g2 : texture_2d<f32>;
@@ -107,6 +107,16 @@ fn main() -> void {
   f1(1.0, 2);
 }
 
+const declaration_order_check_0 : i32 = 1;
+
+type declaration_order_check_1 = f32;
+
+fn declaration_order_check_2() -> void {}
+
+type declaration_order_check_2 = f32;
+
+const declaration_order_check_3 : i32 = 1;
+
 )");
 
   // Parse the wgsl, create the src program
diff --git a/src/clone_context.cc b/src/clone_context.cc
index 7991540da0..f56ec78deb 100644
--- a/src/clone_context.cc
+++ b/src/clone_context.cc
@@ -30,15 +30,7 @@ Symbol CloneContext::Clone(const Symbol& s) const {
 }
 
 void CloneContext::Clone() {
-  for (auto* ty : src->AST().ConstructedTypes()) {
-    dst->AST().AddConstructedType(Clone(ty));
-  }
-  for (auto* var : src->AST().GlobalVariables()) {
-    dst->AST().AddGlobalVariable(Clone(var));
-  }
-  for (auto* func : src->AST().Functions()) {
-    dst->AST().AddFunction(Clone(func));
-  }
+  dst->AST().Copy(this, &src->AST());
 }
 
 ast::FunctionList CloneContext::Clone(const ast::FunctionList& v) {