Merge pull request #1153 from MarijnS95/bounds-impl

GuillaumeGomez · web-flow · commit ede0c31f5a5d · 2021-09-20T13:29:32.000+02:00
Use `impl Trait` instead of generic type names with trait bound
diff --git a/src/analysis/bounds.rs b/src/analysis/bounds.rs
@@ -28,15 +28,24 @@ pub enum BoundType {
 
 impl BoundType {
     pub fn need_isa(&self) -> bool {
-        matches!(*self, BoundType::IsA(_))
+        matches!(*self, Self::IsA(_))
+    }
+
+    // TODO: This is just a heuristic for now, based on what we do in codegen!
+    // Theoretically the surrounding function should determine whether it needs to
+    // reuse an alias (ie. to use in `call_func::<P, Q, R>`) or not.
+    // In the latter case an `impl` is generated instead of a type name/alias.
+    pub fn has_alias(&self) -> bool {
+        matches!(*self, Self::NoWrapper)
     }
 }
 
 #[derive(Clone, Eq, Debug, PartialEq)]
 pub struct Bound {
     pub bound_type: BoundType,
     pub parameter_name: String,
-    pub alias: char,
+    /// Bound does not have an alias when `param: impl type_str` is used
+    pub alias: Option<char>,
     pub type_str: String,
     pub callback_modified: bool,
 }
@@ -217,7 +226,9 @@ impl Bounds {
         if self.used.iter().any(|n| n.parameter_name == name) {
             return;
         }
-        let alias = self.unused.pop_front().expect("No free type aliases!");
+        let alias = bound_type
+            .has_alias()
+            .then(|| self.unused.pop_front().expect("No free type aliases!"));
         self.used.push(Bound {
             bound_type,
             parameter_name: name.to_owned(),
@@ -375,15 +386,36 @@ mod tests {
     #[test]
     fn get_parameter_bound() {
         let mut bounds: Bounds = Default::default();
-        let typ = BoundType::IsA(None);
+        let typ = BoundType::NoWrapper;
         bounds.add_parameter("a", "", typ.clone(), false);
         bounds.add_parameter("b", "", typ.clone(), false);
         let bound = bounds.get_parameter_bound("a").unwrap();
-        assert_eq!(bound.alias, 'P');
+        // `NoWrapper `bounds are expected to have an alias:
+        assert_eq!(bound.alias, Some('P'));
         assert_eq!(bound.bound_type, typ);
         let bound = bounds.get_parameter_bound("b").unwrap();
-        assert_eq!(bound.alias, 'Q');
+        assert_eq!(bound.alias, Some('Q'));
         assert_eq!(bound.bound_type, typ);
         assert_eq!(bounds.get_parameter_bound("c"), None);
     }
+
+    #[test]
+    fn impl_bound() {
+        let mut bounds: Bounds = Default::default();
+        let typ = BoundType::IsA(None);
+        bounds.add_parameter("a", "", typ.clone(), false);
+        bounds.add_parameter("b", "", typ.clone(), false);
+        let bound = bounds.get_parameter_bound("a").unwrap();
+        // `IsA` is simplified to an inline `foo: impl IsA<Bar>` and
+        // lacks an alias/type-parameter:
+        assert_eq!(bound.alias, None);
+        assert_eq!(bound.bound_type, typ);
+
+        let typ = BoundType::AsRef(None);
+        bounds.add_parameter("c", "", typ.clone(), false);
+        let bound = bounds.get_parameter_bound("c").unwrap();
+        // Same `impl AsRef<Foo>` simplification as `IsA`:
+        assert_eq!(bound.alias, None);
+        assert_eq!(bound.bound_type, typ);
+    }
 }
diff --git a/src/analysis/child_properties.rs b/src/analysis/child_properties.rs
@@ -1,7 +1,12 @@
 use crate::{
-    analysis::{bounds::Bounds, imports::Imports, ref_mode::RefMode, rust_type::RustType},
-    codegen::function,
+    analysis::{
+        bounds::{Bound, Bounds},
+        imports::Imports,
+        ref_mode::RefMode,
+        rust_type::RustType,
+    },
     config,
+    consts::TYPE_PARAMETERS_START,
     env::Env,
     library::{self, ParameterDirection},
     nameutil,
@@ -123,14 +128,25 @@ fn analyze_property(
                 .ref_mode(RefMode::ByRefFake)
                 .try_build()
                 .into_string();
-            let mut bounds = Bounds::default();
-            bounds.add_parameter("P", &r_type, bound, false);
-            let (s_bounds, _) = function::bounds(&bounds, &[], false, false);
-            // Because the bounds won't necessarily be added into the final function, we
-            // only keep the "inner" part to make the string computation easier. So
-            // `<T: X>` becomes `T: X`.
-            bounds_str.push_str(&s_bounds[1..s_bounds.len() - 1]);
-            format!("{}: {}", prop_name, bounds.iter().last().unwrap().alias)
+
+            let _bound = Bound {
+                bound_type: bound,
+                parameter_name: TYPE_PARAMETERS_START.to_string(),
+                alias: Some(TYPE_PARAMETERS_START.to_owned()),
+                type_str: r_type,
+                callback_modified: false,
+            };
+            // TODO: bounds_str push?!?!
+            bounds_str.push_str("TODO");
+            format!("{}: {}", prop_name, TYPE_PARAMETERS_START)
+            // let mut bounds = Bounds::default();
+            // bounds.add_parameter("P", &r_type, bound, false);
+            // let (s_bounds, _) = function::bounds(&bounds, &[], false);
+            // // Because the bounds won't necessarily be added into the final function, we
+            // // only keep the "inner" part to make the string computation easier. So
+            // // `<T: X>` becomes `T: X`.
+            // bounds_str.push_str(&s_bounds[1..s_bounds.len() - 1]);
+            // format!("{}: {}", prop_name, bounds.iter().last().unwrap().alias)
         } else {
             format!(
                 "{}: {}",
diff --git a/src/codegen/bound.rs b/src/codegen/bound.rs
@@ -9,7 +9,7 @@ use crate::{
 impl Bound {
     /// Returns the type parameter reference.
     /// Currently always returns the alias.
-    pub(super) fn type_parameter_reference(&self) -> char {
+    pub(super) fn type_parameter_reference(&self) -> Option<char> {
         self.alias
     }
 
@@ -19,22 +19,48 @@ impl Bound {
         &self,
         ref_mode: RefMode,
         nullable: Nullable,
+        r#async: bool,
     ) -> String {
-        let t = self.type_parameter_reference();
         let ref_str = ref_mode.for_rust_type();
+
+        // Generate `impl Trait` if this bound does not have an alias
+        let trait_bound = match self.type_parameter_reference() {
+            Some(t) => t.to_string(),
+            None => {
+                let trait_bound = self.trait_bound(r#async);
+                let trait_bound = format!("impl {}", trait_bound);
+
+                // Combining a ref mode and lifetime requires parentheses for disambiguation
+                match self.bound_type {
+                    BoundType::IsA(lifetime) => {
+                        // TODO: This is fragile
+                        let has_lifetime = r#async || lifetime.is_some();
+
+                        if !ref_str.is_empty() && has_lifetime {
+                            format!("({})", trait_bound)
+                        } else {
+                            trait_bound
+                        }
+                    }
+                    _ => trait_bound,
+                }
+            }
+        };
+
         match self.bound_type {
             BoundType::IsA(_) if *nullable => {
-                format!("Option<{}{}>", ref_str, t)
+                format!("Option<{}{}>", ref_str, trait_bound)
             }
-            BoundType::IsA(_) => format!("{}{}", ref_str, t),
-            BoundType::NoWrapper | BoundType::AsRef(_) => t.to_string(),
+            BoundType::IsA(_) => format!("{}{}", ref_str, trait_bound),
+            BoundType::NoWrapper | BoundType::AsRef(_) => trait_bound,
         }
     }
 
     /// Returns the type parameter definition for this bound, usually
     /// of the form `T: SomeTrait` or `T: IsA<Foo>`.
-    pub(super) fn type_parameter_definition(&self, r#async: bool) -> String {
-        format!("{}: {}", self.alias, self.trait_bound(r#async))
+    pub(super) fn type_parameter_definition(&self, r#async: bool) -> Option<String> {
+        self.alias
+            .map(|alias| format!("{}: {}", alias, self.trait_bound(r#async)))
     }
 
     /// Returns the trait bound, usually of the form `SomeTrait`
@@ -47,6 +73,7 @@ impl Bound {
                     assert!(lifetime.is_none(), "Async overwrites lifetime");
                 }
                 let is_a = format!("IsA<{}>", self.type_str);
+
                 let lifetime = r#async
                     .then(|| " + Clone + 'static".to_string())
                     .or_else(|| lifetime.map(|l| format!(" + '{}", l)))
diff --git a/src/codegen/function.rs b/src/codegen/function.rs
@@ -180,7 +180,7 @@ pub fn declaration(env: &Env, analysis: &analysis::functions::Info) -> String {
             param_str.push_str(", ")
         }
         let c_par = &analysis.parameters.c_parameters[par.ind_c];
-        let s = c_par.to_parameter(env, &analysis.bounds);
+        let s = c_par.to_parameter(env, &analysis.bounds, false);
         param_str.push_str(&s);
     }
 
@@ -210,8 +210,12 @@ pub fn declaration_futures(env: &Env, analysis: &analysis::functions::Info) -> S
 
         if c_par.name == "callback" || c_par.name == "cancellable" {
             skipped += 1;
-            if let Some(bound) = analysis.bounds.get_parameter_bound(&c_par.name) {
-                skipped_bounds.push(bound.type_parameter_reference());
+            if let Some(alias) = analysis
+                .bounds
+                .get_parameter_bound(&c_par.name)
+                .and_then(|bound| bound.type_parameter_reference())
+            {
+                skipped_bounds.push(alias);
             }
             continue;
         }
@@ -220,7 +224,7 @@ pub fn declaration_futures(env: &Env, analysis: &analysis::functions::Info) -> S
             param_str.push_str(", ")
         }
 
-        let s = c_par.to_parameter(env, &analysis.bounds);
+        let s = c_par.to_parameter(env, &analysis.bounds, true);
         param_str.push_str(&s);
     }
 
@@ -246,7 +250,8 @@ pub fn bounds(
 
     let skip_lifetimes = bounds
         .iter()
-        .filter(|bound| skip.contains(&bound.alias))
+        // TODO: False or true?
+        .filter(|bound| bound.alias.map_or(false, |alias| skip.contains(&alias)))
         .filter_map(|bound| match bound.bound_type {
             IsA(lifetime) | AsRef(lifetime) => lifetime,
             _ => None,
@@ -260,13 +265,18 @@ pub fn bounds(
         .collect::<Vec<_>>();
 
     let bounds = bounds.iter().filter(|bound| {
-        !skip.contains(&bound.alias) && (!filter_callback_modified || !bound.callback_modified)
+        bound.alias.map_or(true, |alias| !skip.contains(&alias))
+            && (!filter_callback_modified || !bound.callback_modified)
     });
 
     let type_names = lifetimes
         .iter()
         .cloned()
-        .chain(bounds.clone().map(|b| b.type_parameter_definition(r#async)))
+        .chain(
+            bounds
+                .clone()
+                .filter_map(|b| b.type_parameter_definition(r#async)),
+        )
         .collect::<Vec<_>>();
 
     let type_names = if type_names.is_empty() {
@@ -277,7 +287,9 @@ pub fn bounds(
 
     let bounds = lifetimes
         .into_iter()
-        .chain(bounds.map(|b| b.alias.to_string()))
+        // TODO: enforce that this is only used on NoWrapper!
+        // TODO: Analyze if alias is used in function, otherwise set to None!
+        .chain(bounds.filter_map(|b| b.alias).map(|a| a.to_string()))
         .collect::<Vec<_>>();
 
     (type_names, bounds)
diff --git a/src/codegen/object.rs b/src/codegen/object.rs
@@ -249,6 +249,7 @@ fn generate_builder(w: &mut dyn Write, env: &Env, analysis: &analysis::object::I
                                     bound.full_type_parameter_reference(
                                         RefMode::ByRef,
                                         Nullable(false),
+                                        false,
                                     )
                                 });
                         (alias, bounds, ".clone().upcast()")
diff --git a/src/codegen/parameter.rs b/src/codegen/parameter.rs
@@ -8,16 +8,18 @@ use crate::{
 };
 
 pub trait ToParameter {
-    fn to_parameter(&self, env: &Env, bounds: &Bounds) -> String;
+    fn to_parameter(&self, env: &Env, bounds: &Bounds, r#async: bool) -> String;
 }
 
 impl ToParameter for CParameter {
-    fn to_parameter(&self, env: &Env, bounds: &Bounds) -> String {
+    fn to_parameter(&self, env: &Env, bounds: &Bounds, r#async: bool) -> String {
         if self.instance_parameter {
             format!("{}self", self.ref_mode.for_rust_type())
         } else {
             let type_str = match bounds.get_parameter_bound(&self.name) {
-                Some(bound) => bound.full_type_parameter_reference(self.ref_mode, self.nullable),
+                Some(bound) => {
+                    bound.full_type_parameter_reference(self.ref_mode, self.nullable, r#async)
+                }
                 None => {
                     let type_name = RustType::builder(env, self.typ)
                         .direction(self.direction)
diff --git a/src/codegen/trampoline.rs b/src/codegen/trampoline.rs
@@ -24,13 +24,18 @@ pub fn generate(
     in_trait: bool,
     indent: usize,
 ) -> Result<()> {
-    let self_bound = in_trait
-        .then(|| format!("{}: IsA<{}>, ", TYPE_PARAMETERS_START, analysis.type_name))
+    let (self_bound, fn_self_bound) = in_trait
+        .then(|| {
+            (
+                format!("{}: IsA<{}>, ", TYPE_PARAMETERS_START, analysis.type_name),
+                Some(TYPE_PARAMETERS_START.to_string()),
+            )
+        })
         .unwrap_or_default();
 
     let prepend = tabs(indent);
     let params_str = trampoline_parameters(env, analysis);
-    let func_str = func_string(env, analysis, None, true);
+    let func_str = func_string(env, analysis, fn_self_bound, true);
     let ret_str = trampoline_returns(env, analysis);
 
     writeln!(
@@ -56,7 +61,7 @@ pub fn generate(
 pub fn func_string(
     env: &Env,
     analysis: &Trampoline,
-    replace_self_bound: Option<&str>,
+    replace_self_bound: Option<impl AsRef<str>>,
     closure: bool,
 ) -> String {
     let param_str = func_parameters(env, analysis, replace_self_bound, closure);
@@ -89,7 +94,7 @@ pub fn func_string(
 fn func_parameters(
     env: &Env,
     analysis: &Trampoline,
-    replace_self_bound: Option<&str>,
+    replace_self_bound: Option<impl AsRef<str>>,
     closure: bool,
 ) -> String {
     let mut param_str = String::with_capacity(100);
@@ -124,7 +129,10 @@ fn func_parameter(env: &Env, par: &RustParameter, bounds: &Bounds) -> String {
     };
 
     match bounds.get_parameter_bound(&par.name) {
-        Some(bound) => bound.full_type_parameter_reference(ref_mode, par.nullable),
+        // TODO: ASYNC??
+        Some(bound) => bound.full_type_parameter_reference(ref_mode, par.nullable, false),
+        // TODO
+        // Some((None, _)) => panic!("Trampoline expects type name"),
         None => RustType::builder(env, par.typ)
             .direction(par.direction)
             .nullable(par.nullable)

Original file line number	Diff line number	Diff line change
`@@ -249,6 +249,7 @@ fn generate_builder(w: &mut dyn Write, env: &Env, analysis: &analysis::object::I`
`249`	`249`	`bound.full_type_parameter_reference(`
`250`	`250`	`RefMode::ByRef,`
`251`	`251`	`Nullable(false),`
	`252`	`+ false,`
`252`	`253`	`)`
`253`	`254`	`});`
`254`	`255`	`(alias, bounds, ".clone().upcast()")`