Reimplement name disambiguation on overloading

This commit reimplements how we disambiguate function names on overloading.
Previously functions would be first be disambiguated if they had multiple
instances of the same name, and *then* functions would be disambiguated
aftewards by if their arguments expanded to more than one type to generate.

This commit instead collects everything into one list during the first pass.
This one list contains all signatures known for a given name. Later this list is
walked in one pass to generate all methods necessary, expanding names all at
once instead of two steps.

This should improve the naming of methods across multiple functions which also
have optional arguments. Support in this commit is just enough for namespaces,
but following commits will update the strategy for mixins/interfaces.
Additionally only new code was added in this commit which duplicates a lot of
functionality, but subsequent commits will remove the old code that will
eventually no longer be used.

crates/webidl/src/first_pass.rs

@@ -18,8 +18,8 @@ use weedle::namespace::OperationNamespaceMember;
 use weedle;
 
 use super::Result;
-use util;
 use util::camel_case_ident;
+use util;
 
 /// Collection of constructs that may use partial.
 #[derive(Default)]
@@ -44,6 +44,7 @@ pub(crate) struct InterfaceData<'src> {
     pub(crate) consts: Vec<&'src ConstMember<'src>>,
     pub(crate) methods: Vec<&'src OperationInterfaceMember<'src>>,
     pub(crate) operations: BTreeMap<OperationId<'src>, OperationData<'src>>,
+    pub(crate) operations2: BTreeMap<OperationId<'src>, OperationData2<'src>>,
     pub(crate) superclass: Option<&'src str>,
     pub(crate) definition_attributes: Option<&'src [ExtendedAttribute<'src>]>,
     pub(crate) constructors: Vec<(&'src str, &'src [Argument<'src>])>,
@@ -58,6 +59,7 @@ pub(crate) struct MixinData<'src> {
     pub(crate) consts: Vec<&'src ConstMember<'src>>,
     pub(crate) methods: Vec<&'src OperationMixinMember<'src>>,
     pub(crate) operations: BTreeMap<OperationId<'src>, OperationData<'src>>,
+    pub(crate) operations2: BTreeMap<OperationId<'src>, OperationData2<'src>>,
 }
 
 /// We need to collect namespace data during the first pass, to be used later.
@@ -66,6 +68,7 @@ pub(crate) struct NamespaceData<'src> {
     /// Whether only partial namespaces were encountered
     pub(crate) members: Vec<&'src OperationNamespaceMember<'src>>,
     pub(crate) operations: BTreeMap<OperationId<'src>, OperationData<'src>>,
+    pub(crate) operations2: BTreeMap<OperationId<'src>, OperationData2<'src>>,
 }
 
 #[derive(Default)]
@@ -91,6 +94,25 @@ pub(crate) struct OperationData<'src> {
     pub(crate) argument_names_same: BTreeMap<Vec<&'src str>, bool>,
 }
 
+#[derive(Default)]
+pub(crate) struct OperationData2<'src> {
+    pub(crate) signatures: Vec<Signature<'src>>,
+}
+
+#[derive(Clone)]
+pub(crate) struct Signature<'src> {
+    pub(crate) args: Vec<Arg<'src>>,
+    pub(crate) ret: weedle::types::ReturnType<'src>,
+    pub(crate) attrs: &'src Option<ExtendedAttributeList<'src>>,
+}
+
+#[derive(Clone)]
+pub(crate) struct Arg<'src> {
+    pub(crate) name: &'src str,
+    pub(crate) ty: &'src weedle::types::Type<'src>,
+    pub(crate) optional: bool,
+}
+
 /// Implemented on an AST node to populate the `FirstPassRecord` struct.
 pub(crate) trait FirstPass<'src, Ctx> {
     /// Populate `record` with any constructs in `self`.
@@ -207,7 +229,13 @@ fn first_pass_operation<'src>(
     self_name: &'src str,
     ids: &[OperationId<'src>],
     arguments: &'src [Argument<'src>],
-) {
+    ret: &weedle::types::ReturnType<'src>,
+    attrs: &'src Option<ExtendedAttributeList<'src>>,
+) -> bool {
+    if util::is_chrome_only(attrs) {
+        return false
+    }
+
     let mut names = Vec::with_capacity(arguments.len());
     for argument in arguments {
         match argument {
@@ -215,29 +243,46 @@ fn first_pass_operation<'src>(
             Argument::Variadic(variadic) => names.push(variadic.identifier.0),
         }
     }
-    let operations = match first_pass_operation_type{
+    let (operations, operations2) = match first_pass_operation_type{
         FirstPassOperationType::Interface => {
-            &mut record
+            let x = record
                 .interfaces
                 .get_mut(self_name)
-                .expect(&format!("not found {} interface", self_name))
+                .expect(&format!("not found {} interface", self_name));
-                .operations
+            (&mut x.operations, &mut x.operations2)
         },
         FirstPassOperationType::Mixin => {
-            &mut record
+            let x = record
                 .mixins
                 .get_mut(self_name)
-                .expect(&format!("not found {} mixin", self_name))
+                .expect(&format!("not found {} mixin", self_name));
-                .operations
+            (&mut x.operations, &mut x.operations2)
         },
         FirstPassOperationType::Namespace => {
-            &mut record
+            let x = record
                 .namespaces
                 .get_mut(self_name)
-                .expect(&format!("not found {} namesace", self_name))
+                .expect(&format!("not found {} namespace", self_name));
-                .operations
+            (&mut x.operations, &mut x.operations2)
         },
     };
+    let mut args = Vec::with_capacity(arguments.len());
+    for argument in arguments {
+        let arg = match argument {
+            Argument::Single(single) => single,
+            Argument::Variadic(v) => {
+                warn!("Unsupported variadic argument {} in {}",
+                      v.identifier.0,
+                      self_name);
+                return false
+            }
+        };
+        args.push(Arg {
+            name: arg.identifier.0,
+            ty: &arg.type_.type_,
+            optional: arg.optional.is_some(),
+        });
+    }
     for id in ids {
         operations
             .entry(*id)
@@ -247,7 +292,16 @@ fn first_pass_operation<'src>(
             .entry(names.clone())
             .and_modify(|same_argument_names| *same_argument_names = true)
             .or_insert(false);
+        operations2.entry(*id)
+            .or_default()
+            .signatures
+            .push(Signature {
+                args: args.clone(),
+                ret: ret.clone(),
+                attrs,
+            });
     }
+    true
 }
 
 impl<'src> FirstPass<'src, ()> for weedle::InterfaceDefinition<'src> {
@@ -294,49 +348,67 @@ fn process_interface_attribute<'src>(
     self_name: &'src str,
     attr: &'src ExtendedAttribute<'src>
 ) {
+    let ident = weedle::common::Identifier(self_name);
+    let non_null = weedle::types::MayBeNull { type_: ident, q_mark: None };
+    let non_any = weedle::types::NonAnyType::Identifier(non_null);
+    let single = weedle::types::SingleType::NonAny(non_any);
+    let ty = weedle::types::Type::Single(single);
+    let return_ty = weedle::types::ReturnType::Type(ty);
     match attr {
         ExtendedAttribute::ArgList(list)
             if list.identifier.0 == "Constructor" =>
         {
-            record.interfaces
+            if first_pass_operation(
-                .get_mut(self_name)
-                .unwrap()
-                .constructors.push((self_name, &list.args.body.list));
-            first_pass_operation(
                 record,
                 FirstPassOperationType::Interface,
                 self_name,
                 &[OperationId::Constructor],
                 &list.args.body.list,
-            )
+                &return_ty,
-        }
+                &None,
-        ExtendedAttribute::NoArgs(other) if (other.0).0 == "Constructor" => {
+            ) {
                 record.interfaces
                     .get_mut(self_name)
                     .unwrap()
-                .constructors.push((self_name, &[]));
+                    .constructors
-            first_pass_operation(
+                    .push((self_name, &list.args.body.list));
+            }
+        }
+        ExtendedAttribute::NoArgs(other) if (other.0).0 == "Constructor" => {
+            if first_pass_operation(
                 record,
                 FirstPassOperationType::Interface,
                 self_name,
                 &[OperationId::Constructor],
                 &[],
-            )
+                &return_ty,
+                &None,
+            ) {
+                record.interfaces
+                    .get_mut(self_name)
+                    .unwrap()
+                    .constructors
+                    .push((self_name, &[]));
+            }
         }
         ExtendedAttribute::NamedArgList(list)
             if list.lhs_identifier.0 == "NamedConstructor" =>
         {
-            record.interfaces
+            if first_pass_operation(
-                .get_mut(self_name)
-                .unwrap()
-                .constructors.push((list.rhs_identifier.0, &list.args.body.list));
-            first_pass_operation(
                 record,
                 FirstPassOperationType::Interface,
                 self_name,
                 &[OperationId::Constructor],
                 &list.args.body.list,
-            )
+                &return_ty,
+                &None,
+            ) {
+                record.interfaces
+                    .get_mut(self_name)
+                    .unwrap()
+                    .constructors
+                    .push((list.rhs_identifier.0, &list.args.body.list));
+            }
         }
         ExtendedAttribute::Ident(id) if id.lhs_identifier.0 == "Global" => {
             record.interfaces.get_mut(self_name).unwrap().global = true;
@@ -412,10 +484,6 @@ impl<'src> FirstPass<'src, &'src str> for weedle::interface::InterfaceMember<'sr
 
 impl<'src> FirstPass<'src, &'src str> for weedle::interface::OperationInterfaceMember<'src> {
     fn first_pass(&'src self, record: &mut FirstPassRecord<'src>, self_name: &'src str) -> Result<()> {
-        if util::is_chrome_only(&self.attributes) {
-            return Ok(());
-        }
-
         if self.specials.len() > 1 {
             warn!("Unsupported webidl operation: {:?}", self);
             return Ok(())
@@ -425,12 +493,6 @@ impl<'src> FirstPass<'src, &'src str> for weedle::interface::OperationInterfaceM
             return Ok(())
         }
 
-        record.interfaces
-            .get_mut(self_name)
-            .unwrap()
-            .methods
-            .push(self);
-
         let mut ids = vec![OperationId::Operation(self.identifier.map(|s| s.0))];
         for special in self.specials.iter() {
             ids.push(match special {
@@ -440,13 +502,21 @@ impl<'src> FirstPass<'src, &'src str> for weedle::interface::OperationInterfaceM
                 Special::LegacyCaller(_) => continue,
             });
         }
-        first_pass_operation(
+        if first_pass_operation(
             record,
             FirstPassOperationType::Interface,
             self_name,
             &ids,
             &self.args.body.list,
-        );
+            &self.return_type,
+            &self.attributes,
+        ) {
+            record.interfaces
+                .get_mut(self_name)
+                .unwrap()
+                .methods
+                .push(self);
+        }
         Ok(())
     }
 }
@@ -538,28 +608,26 @@ impl<'src> FirstPass<'src, &'src str> for weedle::mixin::MixinMember<'src> {
 
 impl<'src> FirstPass<'src, &'src str> for weedle::mixin::OperationMixinMember<'src> {
     fn first_pass(&'src self, record: &mut FirstPassRecord<'src>, self_name: &'src str) -> Result<()> {
-        if util::is_chrome_only(&self.attributes) {
-            return Ok(());
-        }
-
         if self.stringifier.is_some() {
             warn!("Unsupported webidl stringifier: {:?}", self);
             return Ok(())
         }
 
-        record.mixins
+        if first_pass_operation(
-            .get_mut(self_name)
-            .unwrap()
-            .methods
-            .push(self);
-
-        first_pass_operation(
             record,
             FirstPassOperationType::Mixin,
             self_name,
             &[OperationId::Operation(self.identifier.map(|s| s.0.clone()))],
             &self.args.body.list,
-        );
+            &self.return_type,
+            &self.attributes,
+        ) {
+            record.mixins
+                .get_mut(self_name)
+                .unwrap()
+                .methods
+                .push(self);
+        }
         Ok(())
     }
 }
@@ -643,19 +711,17 @@ impl<'src> FirstPass<'src, &'src str> for weedle::namespace::NamespaceMember<'sr
 
 impl<'src> FirstPass<'src, &'src str> for weedle::namespace::OperationNamespaceMember<'src> {
     fn first_pass(&'src self, record: &mut FirstPassRecord<'src>, self_name: &'src str) -> Result<()> {
-        if util::is_chrome_only(&self.attributes) {
+        if first_pass_operation(
-            return Ok(())
-        }
-
-        record.namespaces.get_mut(self_name).unwrap().members.push(self);
-
-        first_pass_operation(
             record,
             FirstPassOperationType::Namespace,
             self_name,
             &[OperationId::Operation(self.identifier.map(|s| s.0.clone()))],
             &self.args.body.list,
-        );
+            &self.return_type,
+            &self.attributes,
+        ) {
+            record.namespaces.get_mut(self_name).unwrap().members.push(self);
+        }
         Ok(())
     }
 }

crates/webidl/src/lib.rs

@@ -184,7 +184,7 @@ impl<'src> FirstPassRecord<'src> {
     fn append_dictionary(
         &self,
         program: &mut backend::ast::Program,
-        data: &first_pass::DictionaryData,
+        data: &first_pass::DictionaryData<'src>,
     ) {
         let def = match data.definition {
             Some(def) => def,
@@ -297,8 +297,8 @@ impl<'src> FirstPassRecord<'src> {
             imports: Default::default(),
         };
 
-        for member in ns.members.iter() {
+        for (id, data) in ns.operations2.iter() {
-            self.append_ns_member(&mut module, name, member);
+            self.append_ns_member(&mut module, name, id, data);
         }
 
         program.modules.push(module);
@@ -308,15 +308,30 @@ impl<'src> FirstPassRecord<'src> {
         &self,
         module: &mut backend::ast::Module,
         self_name: &'src str,
-        member: &'src weedle::namespace::OperationNamespaceMember<'src>,
+        id: &OperationId<'src>,
+        data: &first_pass::OperationData2<'src>,
     ) {
-        for import_function in self.create_namespace_operation(
+        let name = match id {
-            &member.args.body.list,
+            OperationId::Operation(Some(name)) => name,
-            member.identifier.as_ref().map(|id| id.0),
+            OperationId::Constructor |
-            &member.return_type,
+            OperationId::Operation(None) |
+            OperationId::IndexingGetter |
+            OperationId::IndexingSetter |
+            OperationId::IndexingDeleter => {
+                warn!("Unsupported unnamed operation: on {:?}", self_name);
+                return
+            }
+        };
+        let doc_comment = format!(
+            "The `{}.{}()` function\n\n{}",
             self_name,
-            util::throws(&member.attributes)
+            name,
-        ) {
+            mdn_doc(self_name, Some(&name))
+        );
+
+        let kind = backend::ast::ImportFunctionKind::Normal;
+        for mut import_function in self.create_imports(kind, id, data) {
+            import_function.doc_comment = Some(doc_comment.clone());
             module.imports.push(
                 backend::ast::Import {
                     module: None,
@@ -364,7 +379,7 @@ impl<'src> FirstPassRecord<'src> {
         &self,
         program: &mut backend::ast::Program,
         name: &'src str,
-        data: &first_pass::InterfaceData,
+        data: &first_pass::InterfaceData<'src>,
     ) {
         let doc_comment = Some(format!(
             "The `{}` object\n\n{}",

crates/webidl/src/util.rs

@@ -1,5 +1,6 @@
-use std::iter::FromIterator;
 use std::collections::BTreeMap;
+use std::iter::FromIterator;
+use std::ptr;
 
 use backend;
 use backend::util::{ident_ty, leading_colon_path_ty, raw_ident, rust_ident};
@@ -11,7 +12,7 @@ use weedle::attribute::{ExtendedAttributeList, ExtendedAttribute};
 use weedle::argument::Argument;
 use weedle::literal::{ConstValue, FloatLit, IntegerLit};
 
-use first_pass::{self, FirstPassRecord};
+use first_pass::{self, FirstPassRecord, OperationId, OperationData2, Signature};
 use idl_type::{IdlType, ToIdlType, flatten};
 
 /// Take a type and create an immutable shared reference to that type.
@@ -409,9 +410,9 @@ impl<'src> FirstPassRecord<'src> {
     /// Create a wasm-bindgen method, if possible.
     pub fn create_basic_method(
         &self,
-        arguments: &[Argument],
+        arguments: &[Argument<'src>],
         operation_id: first_pass::OperationId,
-        return_type: &weedle::types::ReturnType,
+        return_type: &weedle::types::ReturnType<'src>,
         self_name: &str,
         is_static: bool,
         structural: bool,
@@ -560,67 +561,11 @@ impl<'src> FirstPassRecord<'src> {
         )
     }
 
-    /// Create a wasm-bindgen operation (free function with no `self` type), if possible.
-    pub fn create_namespace_operation(
-        &self,
-        arguments: &[weedle::argument::Argument],
-        operation_name: Option<&str>,
-        return_type: &weedle::types::ReturnType,
-        self_name: &str,
-        catch: bool,
-    ) -> Vec<backend::ast::ImportFunction> {
-        let (overloaded, same_argument_names) = self.get_operation_overloading(
-            arguments,
-            &first_pass::OperationId::Operation(operation_name),
-            self_name,
-            true,
-        );
-
-        let name = match operation_name {
-            Some(name) => name.to_string(),
-            None => {
-                warn!("Unsupported unnamed operation: on {:?}", self_name);
-                return Vec::new();
-            }
-        };
-
-        let ret = match return_type.to_idl_type(self) {
-            None => return Vec::new(),
-            Some(idl_type) => idl_type,
-        };
-
-        let doc_comment = Some(
-            format!(
-                "The `{}.{}()` function\n\n{}",
-                self_name,
-                name,
-                mdn_doc(self_name, Some(&name))
-            )
-        );
-
-        let arguments = match self.convert_arguments(arguments) {
-            None => return Vec::new(),
-            Some(arguments) => arguments
-        };
-
-        self.create_function(
-            &name,
-            overloaded,
-            same_argument_names,
-            &arguments,
-            ret,
-            backend::ast::ImportFunctionKind::Normal,
-            false,
-            catch,
-            doc_comment,
-        )
-    }
-
     /// Create a wasm-bindgen getter method, if possible.
     pub fn create_getter(
         &self,
         name: &str,
-        ty: &weedle::types::Type,
+        ty: &weedle::types::Type<'src>,
         self_name: &str,
         is_static: bool,
         is_structural: bool,
@@ -646,7 +591,7 @@ impl<'src> FirstPassRecord<'src> {
     pub fn create_setter(
         &self,
         name: &str,
-        field_ty: weedle::types::Type,
+        field_ty: weedle::types::Type<'src>,
         self_name: &str,
         is_static: bool,
         is_structural: bool,
@@ -693,6 +638,149 @@ impl<'src> FirstPassRecord<'src> {
             }
         }
     }
+
+    pub fn create_imports(
+        &self,
+        kind: backend::ast::ImportFunctionKind,
+        id: &OperationId<'src>,
+        data: &OperationData2<'src>,
+    )
+        -> Vec<backend::ast::ImportFunction>
+    {
+        // First up expand all the signatures in `data` into all signatures that
+        // we're going to generate. These signatures will be used to determine
+        // the names for all the various functions.
+        #[derive(Clone)]
+        struct ExpandedSig<'a> {
+            orig: &'a Signature<'a>,
+            args: Vec<IdlType<'a>>,
+        }
+
+        let mut actual_signatures = Vec::new();
+        'outer:
+        for signature in data.signatures.iter() {
+            let start = actual_signatures.len();
+
+            // Start off with an empty signature, this'll handle zero-argument
+            // cases and otherwise the loop below will continue to add on to this.
+            actual_signatures.push(ExpandedSig {
+                orig: signature,
+                args: Vec::with_capacity(signature.args.len()),
+            });
+
+            for (i, arg) in signature.args.iter().enumerate() {
+                let cur = actual_signatures.len();
+                // If any argument in this signature can't be converted we have
+                // to throw out the entire signature, so revert back to the
+                // beginning and then keep going.
+                let idl_type = match arg.ty.to_idl_type(self) {
+                    Some(t) => t,
+                    None => {
+                        actual_signatures.truncate(start);
+                        continue 'outer
+                    }
+                };
+
+                // The first arugment gets pushed directly in-place, but all
+                // future expanded arguments will cause new signatures to be
+                // created. If we have an optional argument then we consider the
+                // already existing signature as the "none" case and the flatten
+                // below will produce the "some" case, so we've already
+                // processed the first argument effectively.
+                let mut first = !arg.optional;
+                for idl_type in idl_type.flatten() {
+                    for j in start..cur {
+                        if first {
+                            actual_signatures[j].args.push(idl_type.clone());
+                            first = false;
+                        } else {
+                            let mut sig = actual_signatures[j].clone();
+                            sig.args.truncate(i);
+                            sig.args.push(idl_type.clone());
+                            actual_signatures.push(sig);
+                        }
+                    }
+                }
+            }
+        }
+
+        let name = match id {
+            OperationId::Constructor => "new",
+            OperationId::Operation(Some(s)) => s,
+            OperationId::Operation(None) => {
+                warn!("unsupported unnamed operation");
+                return Vec::new()
+            }
+            OperationId::IndexingGetter => "get",
+            OperationId::IndexingSetter => "set",
+            OperationId::IndexingDeleter => "delete",
+        };
+
+        let mut ret = Vec::new();
+        for signature in actual_signatures.iter() {
+            // Ignore signatures with invalid return types
+            let ret_ty = match signature.orig.ret.to_idl_type(self) {
+                Some(ty) => ty,
+                None => continue,
+            };
+
+            let mut rust_name = name.to_snake_case();
+            let mut first = true;
+            for (i, arg) in signature.args.iter().enumerate() {
+                // Find out if any other known signature either has the same
+                // name for this argument or a different type for this argument.
+                let mut any_same_name = false;
+                let mut any_different_type = false;
+                let arg_name = signature.orig.args[i].name;
+                for other in actual_signatures.iter() {
+                    if other.orig.args.get(i).map(|s| s.name) == Some(arg_name) {
+                        if !ptr::eq(signature, other) {
+                            any_same_name = true;
+                        }
+                    }
+                    if other.args.get(i) != Some(arg) {
+                        any_different_type = true;
+                    }
+                }
+
+                // If all signatures have the exact same type for this argument,
+                // then there's nothing to disambiguate so we don't modify the
+                // name.
+                if !any_different_type {
+                    continue
+                }
+                if first {
+                    rust_name.push_str("_with_");
+                    first = false;
+                } else {
+                    rust_name.push_str("_and_");
+                }
+
+                // If this name of the argument for this signature is unique
+                // then that's a bit more human readable so we include it in the
+                // method name. Otherwise the type name should disambiguate
+                // correctly.
+                if !any_same_name {
+                    rust_name.push_str(&arg_name.to_snake_case());
+                } else {
+                    arg.push_type_name(&mut rust_name);
+                }
+            }
+            ret.extend(self.create_one_function(
+                name,
+                &rust_name,
+                signature.args.iter()
+                    .zip(&signature.orig.args)
+                    .map(|(ty, orig_arg)| (orig_arg.name, ty)),
+                &ret_ty,
+                kind.clone(),
+                is_structural(&signature.orig.attrs),
+                throws(&signature.orig.attrs),
+                None,
+            ));
+        }
+        return ret;
+    }
 }
 
 /// Search for an attribute by name in some webidl object's attributes.