C#: use nullable types for non-nested options (bytecodealliance#977)

dicej · web-flow · commit 43362373db40 · 2024-06-19T00:26:00.000Z
We now generate bindings which use nullable types (e.g. `uint?`) to represent non-nested `option` types. Nested `option`s, such as `option<option<T>>` still use the `Option` class except for the innermost `option` in order to avoid ambiguity. Fixes bytecodealliance#964 Signed-off-by: Joel Dice <joel.dice@fermyon.com>
diff --git a/crates/csharp/src/lib.rs b/crates/csharp/src/lib.rs
@@ -1298,16 +1298,16 @@ impl InterfaceGenerator<'_> {
                     }
                     TypeDefKind::Option(base_ty) => {
                         self.gen.needs_option = true;
-                        if let Some(_name) = &ty.name {
-                            format!(
-                                "Option<{}>",
-                                self.type_name_with_qualifier(base_ty, qualifier)
-                            )
+                        let nesting = if let Type::Id(id) = base_ty {
+                            matches!(&self.resolve.types[*id].kind, TypeDefKind::Option(_))
                         } else {
-                            format!(
-                                "Option<{}>",
-                                self.type_name_with_qualifier(base_ty, qualifier)
-                            )
+                            false
+                        };
+                        let base_ty = self.type_name_with_qualifier(base_ty, qualifier);
+                        if nesting {
+                            format!("Option<{base_ty}>")
+                        } else {
+                            format!("{base_ty}?")
                         }
                     }
                     TypeDefKind::Result(result) => {
@@ -2300,9 +2300,20 @@ impl Bindgen for FunctionBindgen<'_, '_> {
 
                 let op = &operands[0];
 
-                let block = |ty: Option<&Type>, Block { body, results, .. }, payload| {
-                    let payload = if let Some(_ty) = self.gen.non_empty_type(ty) {
-                        format!("var {payload} = {op}.Value;")
+                let nesting = if let Type::Id(id) = payload {
+                    matches!(&self.gen.resolve.types[*id].kind, TypeDefKind::Option(_))
+                } else {
+                    false
+                };
+
+                let mut block = |ty: Option<&Type>, Block { body, results, .. }, payload, nesting| {
+                    let payload = if let Some(ty) = self.gen.non_empty_type(ty) {
+                        let ty = self.gen.type_name_with_qualifier(ty, true);
+                        if nesting {
+                            format!("var {payload} = {op}.Value;")
+                        } else {
+                            format!("var {payload} = ({ty}) {op};")
+                        }
                     } else {
                         String::new()
                     };
@@ -2321,15 +2332,21 @@ impl Bindgen for FunctionBindgen<'_, '_> {
                     )
                 };
 
-                let none = block(None, none, none_payload);
-                let some = block(Some(payload), some, some_payload);
+                let none = block(None, none, none_payload, nesting);
+                let some = block(Some(payload), some, some_payload, nesting);
+
+                let test = if nesting {
+                    ".HasValue"
+                } else {
+                    " != null"
+                };
 
                 uwrite!(
                     self.src,
                     r#"
                     {declarations}
 
-                    if ({op}.HasValue) {{
+                    if ({op}{test}) {{
                         {some}
                     }} else {{
                         {none}
@@ -2346,6 +2363,12 @@ impl Bindgen for FunctionBindgen<'_, '_> {
                 let lifted = self.locals.tmp("lifted");
                 let op = &operands[0];
 
+                let nesting = if let Type::Id(id) = payload {
+                    matches!(&self.gen.resolve.types[*id].kind, TypeDefKind::Option(_))
+                } else {
+                    false
+                };
+
                 let payload = if self.gen.non_empty_type(Some(*payload)).is_some() {
                     some.results.into_iter().next().unwrap()
                 } else {
@@ -2354,20 +2377,26 @@ impl Bindgen for FunctionBindgen<'_, '_> {
 
                 let some = some.body;
 
+                let (none_value, some_value) = if nesting {
+                    (format!("{ty}.None"), format!("new ({payload})"))
+                } else {
+                    ("null".into(), payload)
+                };
+
                 uwrite!(
                     self.src,
                     r#"
                     {ty} {lifted};
 
                     switch ({op}) {{
                         case 0: {{
-                            {lifted} = {ty}.None;
+                            {lifted} = {none_value};
                             break;
                         }}
 
                         case 1: {{
                             {some}
-                            {lifted} = new ({payload});
+                            {lifted} = {some_value};
                             break;
                         }}
 
diff --git a/tests/runtime/options.rs b/tests/runtime/options.rs
@@ -28,6 +28,10 @@ impl test::options::test::Host for MyImports {
     fn option_roundtrip(&mut self, a: Option<String>) -> Result<Option<String>> {
         Ok(a)
     }
+
+    fn double_option_roundtrip(&mut self, a: Option<Option<u32>>) -> Result<Option<Option<u32>>> {
+        Ok(a)
+    }
 }
 
 #[test]
@@ -54,5 +58,17 @@ fn run_test(exports: Options, store: &mut Store<crate::Wasi<MyImports>>) -> Resu
         exports.call_option_roundtrip(&mut *store, Some("foo"))?,
         Some("foo".to_string())
     );
+    assert_eq!(
+        exports.call_double_option_roundtrip(&mut *store, Some(Some(42)))?,
+        Some(Some(42))
+    );
+    assert_eq!(
+        exports.call_double_option_roundtrip(&mut *store, Some(None))?,
+        Some(None)
+    );
+    assert_eq!(
+        exports.call_double_option_roundtrip(&mut *store, None)?,
+        None
+    );
     Ok(())
 }
diff --git a/tests/runtime/options/wasm.cs b/tests/runtime/options/wasm.cs
@@ -0,0 +1,56 @@
+using System.Diagnostics;
+using OptionsWorld.wit.imports.test.options;
+
+namespace OptionsWorld.wit.exports.test.options
+{
+    public class TestImpl : ITest
+    {
+        public static void OptionNoneParam(string? a)
+        {
+            Debug.Assert(a == null);
+        }
+        
+        public static string? OptionNoneResult()
+        {
+            return null;
+        }
+        
+        public static void OptionSomeParam(string? a)
+        {
+            Debug.Assert(a == "foo");
+        }
+        
+        public static string? OptionSomeResult()
+        {
+            return "foo";
+        }
+
+        public static string? OptionRoundtrip(string? a)
+        {
+            return a;
+        }        
+
+        public static Option<uint?> DoubleOptionRoundtrip(Option<uint?> a)
+        {
+            return a;
+        }        
+    }
+}
+
+namespace OptionsWorld
+{
+    public class OptionsWorldImpl : IOptionsWorld
+    {
+        public static void TestImports()
+        {
+            TestInterop.OptionNoneParam(null);
+            TestInterop.OptionSomeParam("foo");
+            Debug.Assert(TestInterop.OptionNoneResult() == null);
+            Debug.Assert(TestInterop.OptionSomeResult() == "foo");
+            Debug.Assert(TestInterop.OptionRoundtrip("foo") == "foo");
+            Debug.Assert(TestInterop.DoubleOptionRoundtrip(new Option<uint?>(42)).Value == 42);
+            Debug.Assert(TestInterop.DoubleOptionRoundtrip(new Option<uint?>(null)).Value == null);
+            Debug.Assert(!TestInterop.DoubleOptionRoundtrip(Option<uint?>.None).HasValue);
+        }
+    }
+}
diff --git a/tests/runtime/options/wasm.rs b/tests/runtime/options/wasm.rs
@@ -15,6 +15,9 @@ impl Guest for Component {
         assert!(option_none_result().is_none());
         assert_eq!(option_some_result(), Some("foo".to_string()));
         assert_eq!(option_roundtrip(Some("foo")), Some("foo".to_string()));
+        assert_eq!(double_option_roundtrip(Some(Some(42))), Some(Some(42)));
+        assert_eq!(double_option_roundtrip(Some(None)), Some(None));
+        assert_eq!(double_option_roundtrip(None), None);
     }
 }
 
@@ -38,4 +41,8 @@ impl exports::test::options::test::Guest for Component {
     fn option_roundtrip(a: Option<String>) -> Option<String> {
         a
     }
+
+    fn double_option_roundtrip(a: Option<Option<u32>>) -> Option<Option<u32>> {
+        a
+    }
 }
diff --git a/tests/runtime/options/world.wit b/tests/runtime/options/world.wit
@@ -7,6 +7,8 @@ interface test {
   option-some-result: func() -> option<string>;
 
   option-roundtrip: func(a: option<string>) -> option<string>;
+
+  double-option-roundtrip: func(a: option<option<u32>>) -> option<option<u32>>;
 }
 
 world options {
diff --git a/tests/runtime/resource_aggregates/wasm.cs b/tests/runtime/resource_aggregates/wasm.cs
@@ -22,8 +22,8 @@ public static uint Foo(
 	    ITest.V2 v2,
 	    List<ITest.Thing> l1,
 	    List<ITest.Thing> l2,
-	    Option<ITest.Thing> o1,
-	    Option<ITest.Thing> o2,
+	    ITest.Thing? o1,
+	    ITest.Thing? o2,
 	    Result<ITest.Thing, None> result1,
 	    Result<ITest.Thing, None> result2
 	)
@@ -45,12 +45,12 @@ public static uint Foo(
 	    {
 		il2.Add(((Thing) thing).val);
 	    }
-	    var io1 = o1.HasValue
-		? new Option<Import.Thing>(((Thing) o1.Value).val)
-		: Option<Import.Thing>.None;
-	    var io2 = o2.HasValue
-		? new Option<Import.Thing>(((Thing) o2.Value).val)
-		: Option<Import.Thing>.None;
+	    var io1 = o1 != null
+		? ((Thing) o1).val
+		: null;
+	    var io2 = o2 != null
+		? ((Thing) o2).val
+		: null;
 	    var iresult1 = result1.IsOk
 		? Result<Import.Thing, None>.ok(((Thing) result1.AsOk).val)
 		: Result<Import.Thing, None>.err(new None());

Original file line number	Diff line number	Diff line change
`@@ -15,6 +15,9 @@ impl Guest for Component {`
`15`	`15`	`assert!(option_none_result().is_none());`
`16`	`16`	`assert_eq!(option_some_result(), Some("foo".to_string()));`
`17`	`17`	`assert_eq!(option_roundtrip(Some("foo")), Some("foo".to_string()));`
	`18`	`+ assert_eq!(double_option_roundtrip(Some(Some(42))), Some(Some(42)));`
	`19`	`+ assert_eq!(double_option_roundtrip(Some(None)), Some(None));`
	`20`	`+ assert_eq!(double_option_roundtrip(None), None);`
`18`	`21`	`}`
`19`	`22`	`}`
`20`	`23`
`@@ -38,4 +41,8 @@ impl exports::test::options::test::Guest for Component {`
`38`	`41`	`fn option_roundtrip(a: Option<String>) -> Option<String> {`
`39`	`42`	`a`
`40`	`43`	`}`
	`44`	`+`
	`45`	`+ fn double_option_roundtrip(a: Option<Option<u32>>) -> Option<Option<u32>> {`
	`46`	`+ a`
	`47`	`+ }`
`41`	`48`	`}`
Original file line number	Diff line number	Diff line change
`@@ -7,6 +7,8 @@ interface test {`
`7`	`7`	`option-some-result: func() -> option<string>;`
`8`	`8`
`9`	`9`	`option-roundtrip: func(a: option<string>) -> option<string>;`
	`10`	`+`
	`11`	`+ double-option-roundtrip: func(a: option<option<u32>>) -> option<option<u32>>;`
`10`	`12`	`}`
`11`	`13`
`12`	`14`	`world options {`