Make CI deterministic try 2 (rustwasm#4190)

Author: RunDevelopment

crates/cli-support/src/externref.rs

@@ -26,7 +26,7 @@ pub fn process(module: &mut Module) -> Result<()> {
 
     // Transform all exported functions in the module, using the bindings listed
     // for each exported function.
-    for (id, adapter) in crate::sorted_iter_mut(&mut section.adapters) {
+    for (id, adapter) in &mut section.adapters {
         let instructions = match &mut adapter.kind {
             AdapterKind::Local { instructions } => instructions,
             AdapterKind::Import { .. } => continue,
@@ -78,7 +78,7 @@ pub fn process(module: &mut Module) -> Result<()> {
     // Additionally we may need to update some adapter instructions other than
     // those found for the externref pass. These are some general "fringe support"
     // things necessary to get absolutely everything working.
-    for (_, adapter) in crate::sorted_iter_mut(&mut section.adapters) {
+    for adapter in &mut section.adapters.values_mut() {
         let instrs = match &mut adapter.kind {
             AdapterKind::Local { instructions } => instructions,
             AdapterKind::Import { .. } => continue,

crates/cli-support/src/js/mod.rs

@@ -325,7 +325,7 @@ impl<'a> Context<'a> {
 
         wasm_import_object.push_str(&format!("  {}: {{\n", crate::PLACEHOLDER_MODULE));
 
-        for (id, js) in crate::sorted_iter(&self.wasm_import_definitions) {
+        for (id, js) in iter_by_import(&self.wasm_import_definitions, self.module) {
             let import = self.module.imports.get(*id);
             wasm_import_object.push_str(&format!("{}: {},\n", &import.name, js.trim()));
         }
@@ -424,8 +424,8 @@ impl<'a> Context<'a> {
 
                 js.push_str("let wasm;\n");
 
-                for (id, js) in crate::sorted_iter(&self.wasm_import_definitions) {
-                    let import = self.module.imports.get_mut(*id);
+                for (id, js) in iter_by_import(&self.wasm_import_definitions, self.module) {
+                    let import = self.module.imports.get(*id);
                     footer.push_str("\nmodule.exports.");
                     footer.push_str(&import.name);
                     footer.push_str(" = ");
@@ -463,7 +463,7 @@ impl<'a> Context<'a> {
             // and let the bundler/runtime take care of it.
             // With Node we manually read the Wasm file from the filesystem and instantiate it.
             OutputMode::Bundler { .. } | OutputMode::Node { module: true } => {
-                for (id, js) in crate::sorted_iter(&self.wasm_import_definitions) {
+                for (id, js) in iter_by_import(&self.wasm_import_definitions, self.module) {
                     let import = self.module.imports.get_mut(*id);
                     import.module = format!("./{}_bg.js", module_name);
                     if let Some(body) = js.strip_prefix("function") {
@@ -791,7 +791,7 @@ __wbg_set_wasm(wasm);"
         imports_init.push_str(module_name);
         imports_init.push_str(" = {};\n");
 
-        for (id, js) in crate::sorted_iter(&self.wasm_import_definitions) {
+        for (id, js) in iter_by_import(&self.wasm_import_definitions, self.module) {
             let import = self.module.imports.get_mut(*id);
             import.module = module_name.to_string();
             imports_init.push_str("imports.");
@@ -2531,12 +2531,13 @@ __wbg_set_wasm(wasm);"
         if self.config.symbol_dispose && !self.aux.structs.is_empty() {
             self.expose_symbol_dispose()?;
         }
-        for (id, adapter) in crate::sorted_iter(&self.wit.adapters) {
+
+        for (id, adapter, kind) in iter_adapeter(self.aux, self.wit, self.module) {
             let instrs = match &adapter.kind {
                 AdapterKind::Import { .. } => continue,
                 AdapterKind::Local { instructions } => instructions,
             };
-            self.generate_adapter(*id, adapter, instrs)?;
+            self.generate_adapter(id, adapter, instrs, kind)?;
         }
 
         let mut pairs = self.aux.export_map.iter().collect::<Vec<_>>();
@@ -2614,26 +2615,10 @@ __wbg_set_wasm(wasm);"
         id: AdapterId,
         adapter: &Adapter,
         instrs: &[InstructionData],
+        kind: ContextAdapterKind,
     ) -> Result<(), Error> {
-        enum Kind<'a> {
-            Export(&'a AuxExport),
-            Import(walrus::ImportId),
-            Adapter,
-        }
-
-        let kind = match self.aux.export_map.get(&id) {
-            Some(export) => Kind::Export(export),
-            None => {
-                let core = self.wit.implements.iter().find(|pair| pair.2 == id);
-                match core {
-                    Some((core, _, _)) => Kind::Import(*core),
-                    None => Kind::Adapter,
-                }
-            }
-        };
-
         let catch = self.aux.imports_with_catch.contains(&id);
-        if let Kind::Import(core) = kind {
+        if let ContextAdapterKind::Import(core) = kind {
             if !catch && self.attempt_direct_import(core, instrs)? {
                 return Ok(());
             }
@@ -2643,16 +2628,16 @@ __wbg_set_wasm(wasm);"
         // export that we're generating.
         let mut builder = binding::Builder::new(self);
         builder.log_error(match kind {
-            Kind::Export(_) | Kind::Adapter => false,
-            Kind::Import(_) => builder.cx.config.debug,
+            ContextAdapterKind::Export(_) | ContextAdapterKind::Adapter => false,
+            ContextAdapterKind::Import(_) => builder.cx.config.debug,
         });
         builder.catch(catch);
         let mut arg_names = &None;
         let mut asyncness = false;
         let mut variadic = false;
         let mut generate_jsdoc = false;
         match kind {
-            Kind::Export(export) => {
+            ContextAdapterKind::Export(export) => {
                 arg_names = &export.arg_names;
                 asyncness = export.asyncness;
                 variadic = export.variadic;
@@ -2667,8 +2652,8 @@ __wbg_set_wasm(wasm);"
                     },
                 }
             }
-            Kind::Import(_) => {}
-            Kind::Adapter => {}
+            ContextAdapterKind::Import(_) => {}
+            ContextAdapterKind::Adapter => {}
         }
 
         // Process the `binding` and generate a bunch of JS/TypeScript/etc.
@@ -2692,23 +2677,27 @@ __wbg_set_wasm(wasm);"
                 generate_jsdoc,
             )
             .with_context(|| match kind {
-                Kind::Export(e) => format!("failed to generate bindings for `{}`", e.debug_name),
-                Kind::Import(i) => {
+                ContextAdapterKind::Export(e) => {
+                    format!("failed to generate bindings for `{}`", e.debug_name)
+                }
+                ContextAdapterKind::Import(i) => {
                     let i = builder.cx.module.imports.get(i);
                     format!(
                         "failed to generate bindings for import of `{}::{}`",
                         i.module, i.name
                     )
                 }
-                Kind::Adapter => "failed to generates bindings for adapter".to_string(),
+                ContextAdapterKind::Adapter => {
+                    "failed to generates bindings for adapter".to_string()
+                }
             })?;
 
         self.typescript_refs.extend(ts_refs);
 
         // Once we've got all the JS then put it in the right location depending
         // on what's being exported.
         match kind {
-            Kind::Export(export) => {
+            ContextAdapterKind::Export(export) => {
                 assert!(!catch);
                 assert!(!log_error);
 
@@ -2795,7 +2784,7 @@ __wbg_set_wasm(wasm);"
                     }
                 }
             }
-            Kind::Import(core) => {
+            ContextAdapterKind::Import(core) => {
                 let code = if catch {
                     format!(
                         "function() {{ return handleError(function {}, arguments) }}",
@@ -2812,7 +2801,7 @@ __wbg_set_wasm(wasm);"
 
                 self.wasm_import_definitions.insert(core, code);
             }
-            Kind::Adapter => {
+            ContextAdapterKind::Adapter => {
                 assert!(!catch);
                 assert!(!log_error);
 
@@ -4137,6 +4126,102 @@ __wbg_set_wasm(wasm);"
     }
 }
 
+/// A categorization of adapters for the purpose of code generation.
+///
+/// This is different from [`AdapterKind`] and is only used internally in the
+/// code generation process.
+enum ContextAdapterKind<'a> {
+    /// An exported function, method, constrctor, or getter/setter.
+    Export(&'a AuxExport),
+    /// An imported function or intrinsic.
+    Import(walrus::ImportId),
+    Adapter,
+}
+impl<'a> ContextAdapterKind<'a> {
+    fn get(id: AdapterId, aux: &'a WasmBindgenAux, wit: &'a NonstandardWitSection) -> Self {
+        match aux.export_map.get(&id) {
+            Some(export) => ContextAdapterKind::Export(export),
+            None => {
+                let core = wit.implements.iter().find(|pair| pair.2 == id);
+                match core {
+                    Some((core, _, _)) => ContextAdapterKind::Import(*core),
+                    None => ContextAdapterKind::Adapter,
+                }
+            }
+        }
+    }
+}
+
+/// Iterate over the adapters in a deterministic order.
+fn iter_adapeter<'a>(
+    aux: &'a WasmBindgenAux,
+    wit: &'a NonstandardWitSection,
+    module: &Module,
+) -> Vec<(AdapterId, &'a Adapter, ContextAdapterKind<'a>)> {
+    let mut adapters: Vec<_> = wit
+        .adapters
+        .iter()
+        .map(|(id, adapter)| {
+            // we need the kind of the adapter to properly sort them
+            let kind = ContextAdapterKind::get(*id, aux, wit);
+            (*id, adapter, kind)
+        })
+        .collect();
+
+    // Since `wit.adapters` is a BTreeMap, the adapters are already sorted by
+    // their ID. This is good enough for exports and adapters, but imports need
+    // to be sorted by their name.
+    //
+    // Note: we do *NOT* want to sort exports by name. By default, exports are
+    // the order in which they were defined in the Rust code. Sorting them by
+    // name would break that order and take away control from the user.
+
+    adapters.sort_by(|(_, _, a), (_, _, b)| {
+        fn get_kind_order(kind: &ContextAdapterKind) -> u8 {
+            match kind {
+                ContextAdapterKind::Import(_) => 0,
+                ContextAdapterKind::Export(_) => 1,
+                ContextAdapterKind::Adapter => 2,
+            }
+        }
+
+        match (a, b) {
+            (ContextAdapterKind::Import(a), ContextAdapterKind::Import(b)) => {
+                let a = module.imports.get(*a);
+                let b = module.imports.get(*b);
+                a.name.cmp(&b.name)
+            }
+            _ => get_kind_order(a).cmp(&get_kind_order(b)),
+        }
+    });
+
+    adapters
+}
+
+/// Iterate over the imports in a deterministic order.
+fn iter_by_import<'a, T>(
+    map: &'a HashMap<ImportId, T>,
+    module: &Module,
+) -> Vec<(&'a ImportId, &'a T)> {
+    let mut items: Vec<_> = map.iter().collect();
+
+    // Sort by import name.
+    //
+    // Imports have a name and a module, and it's important that we *ignore*
+    // the module. The module of an import is set to its final value *during*
+    // code generation, so using it here would cause the imports to be sorted
+    // differently depending on which part of the code generation process we're
+    // in.
+    items.sort_by(|&(a, _), &(b, _)| {
+        let a = module.imports.get(*a);
+        let b = module.imports.get(*b);
+
+        a.name.cmp(&b.name)
+    });
+
+    items
+}
+
 fn check_duplicated_getter_and_setter_names(
     exports: &[(&AdapterId, &AuxExport)],
 ) -> Result<(), Error> {

crates/cli-support/src/multivalue.rs

@@ -14,7 +14,7 @@ pub fn run(module: &mut Module) -> Result<(), Error> {
     let mut to_xform = Vec::new();
     let mut slots = Vec::new();
 
-    for (_, adapter) in crate::sorted_iter_mut(&mut adapters.adapters) {
+    for adapter in adapters.adapters.values_mut() {
         extract_xform(module, adapter, &mut to_xform, &mut slots);
     }
     if to_xform.is_empty() {

crates/cli-support/src/wit/mod.rs

@@ -4,7 +4,7 @@ use crate::descriptors::WasmBindgenDescriptorsSection;
 use crate::intrinsic::Intrinsic;
 use crate::{decode, Bindgen, PLACEHOLDER_MODULE};
 use anyhow::{anyhow, bail, Error};
-use std::collections::{HashMap, HashSet};
+use std::collections::{BTreeSet, HashMap};
 use std::str;
 use walrus::MemoryId;
 use walrus::{ExportId, FunctionId, ImportId, Module};
@@ -106,7 +106,9 @@ impl<'a> Context<'a> {
         // location listed of what to import there for each item.
         let mut intrinsics = Vec::new();
         let mut duplicate_import_map = HashMap::new();
-        let mut imports_to_delete = HashSet::new();
+        // The order in which imports are deleted later might matter, so we
+        // use an ordered set here to make everything deterministic.
+        let mut imports_to_delete = BTreeSet::new();
         for import in self.module.imports.iter() {
             if import.module != PLACEHOLDER_MODULE {
                 continue;

crates/cli-support/src/wit/standard.rs

@@ -1,13 +1,17 @@
 use crate::descriptor::VectorKind;
 use crate::wit::{AuxImport, WasmBindgenAux};
 use std::borrow::Cow;
-use std::collections::{HashMap, HashSet};
+use std::collections::{BTreeMap, HashSet};
 use walrus::{FunctionId, ImportId, RefType, TypedCustomSectionId};
 
 #[derive(Default, Debug)]
 pub struct NonstandardWitSection {
     /// A list of adapter functions, keyed by their id.
-    pub adapters: HashMap<AdapterId, Adapter>,
+    ///
+    /// This map is iterated over in multiple places, so we use an ordered map
+    /// to ensure that the order of iteration is deterministic. This map affects
+    /// all parts of the generated code, so it's important to get this right.
+    pub adapters: BTreeMap<AdapterId, Adapter>,
 
     /// A list of pairs for adapter functions that implement core Wasm imports.
     pub implements: Vec<(ImportId, FunctionId, AdapterId)>,