Add default literals for `optional` arguments #283

jhugman · 2020-09-16T16:51:18Z

Fixes #263.

This PR fleshes out the parsing and output of Literals, and used in default arguments and defaulted values for dictionaries. This corresponds to the WebIDL keyword optional.

jhugman

Leaving this here for feedback before a rebase.

jhugman · 2020-09-16T16:55:28Z

uniffi_bindgen/src/bindings/python/gen_python.rs

+            Literal::EmptyMap => "{}".into(),
+
+            _ => panic!("Literal unsupported by python"),
+        })


I'd do more with numbers, but the test_rondpoint.py is quite far behind the kotlin and swift versions right now.

I think my list of "things I want to do before revisiting this python patch" is back down to zero, fingers crossed...

Add this to the list of things to do after #214 has landed.

jhugman · 2020-09-16T17:01:47Z

uniffi_bindgen/src/bindings/swift/gen_swift.rs

+                match radix {
+                    Radix::Octal => format!("0o{:o}", i),
+                    Radix::Decimal => format!("{}", i),
+                    Radix::Hexadecimal => format!("{:#x}", i),
+                },
+            )?,
+            Literal::UInt(i, radix, type_) => typed_number(
+                type_,
+                match radix {
+                    Radix::Octal => format!("0o{:o}", i),
+                    Radix::Decimal => format!("{}", i),
+                    Radix::Hexadecimal => format!("{:#x}", i),
+                },


This level of duplication felt acceptable.

Using the 8 byte u64, i64 and f64 as internal representations for number literals was the compromise between lots of duplication, and flexibility of representation.

That sounds like a fine idea! This reads very cleanly, and I think the duplication trade-off is more than okay.

jhugman · 2020-09-16T17:02:47Z

uniffi_bindgen/src/bindings/swift/templates/RustBufferHelper.swift

@@ -358,7 +358,7 @@ extension Dictionary: ViaFfiUsingByteBuffer, ViaFfi, Serializable where Key == S
        let len: UInt32 = try buf.readInt()
        var dict = [String: Value]()
        dict.reserveCapacity(Int(len))
-        for _ in 1...len {
+        for _ in 0..<len {


Tests found that empty lists crashes the swift bindings.

jhugman · 2020-09-16T17:05:44Z

uniffi_bindgen/src/interface/mod.rs

+            };
+
+            let trimmer = Regex::new("^(-?)(0x)?").unwrap();
+            let string = trimmer.replace_all(string, "$1").to_lowercase();


Possible improvement/optimization: putting this in a lazy_static! macro.

uniffi_bindgen/src/interface/mod.rs

jhugman · 2020-09-16T17:26:18Z

uniffi_bindgen/src/interface/mod.rs

+            (weedle::literal::DefaultValue::String(s), Type::Enum(_)) => {
+                Literal::Enum(s.0.to_string(), type_.clone())
+            }


I was very pleased to work out we could default enums.

rfk

I didn't dig too much into the nitty-gritty parsing details, but did an initial high-level pass and left some comments. Overall, seems to be coming together nicely 👍

rfk · 2020-09-18T07:52:26Z

examples/rondpoint/src/lib.rs

@@ -168,4 +168,93 @@ impl Stringifier {
    }
 }

+#[derive(Debug, Clone)]
+struct Optionneur;
+impl Optionneur {


This rondpoint example continues to be a thing of great beauty 👍

rfk · 2020-09-18T07:57:43Z

uniffi_bindgen/src/bindings/python/gen_python.rs

+            Literal::EmptyMap => "{}".into(),
+
+            _ => panic!("Literal unsupported by python"),
+        })


I think my list of "things I want to do before revisiting this python patch" is back down to zero, fingers crossed...

rfk · 2020-09-18T08:00:08Z

uniffi_bindgen/src/interface/mod.rs

        })
    }
 }

+#[derive(Debug, Clone, Copy, Serialize, Deserialize, Hash)]


Note: as of latest main, you'll need to remove Serialize and Deserialize from here.

rfk · 2020-09-18T08:02:48Z

uniffi_bindgen/src/interface/mod.rs

+            let src_radix = radix as u32;
+            // This radix tells the backends how to represent the number in the output languages.
+            let dest_radix = if string == "0" || string.starts_with('-') {
+                // 1. weedle parses "0" as an octal literal, but we most likely want to treat this as a decimal.


wow; octal zero I hope 😬

:D Yeah. Thought that'd be a bit surprising to our API consumers finding 0o0 in the API.

rfk · 2020-09-18T08:04:46Z

uniffi_bindgen/src/interface/mod.rs

+    Decimal = 10,
+    Octal = 8,
+    Hexadecimal = 16,
+}


Is preserving the radix necessary for correctness in some cases, or is it entirely about readability of the generated code? (I'm not opposed to doing it for readability reasons, just want to make sure I understand).

Preserving the radix is only for readabilty/documentation for the generated interfaces.

There are cases when we can't preserve radix (octal numbers, anyone?), or our choice of representation mean that it's not possible (negative hex and octal numbers); in both these cases we use decimal because correctness beats readability.

Added a comment about radices.

rfk · 2020-09-18T08:08:13Z

uniffi_bindgen/src/interface/mod.rs

-            weedle::literal::DefaultValue::Boolean(b) => Literal::Boolean(b.0),
-            weedle::literal::DefaultValue::String(s) => Literal::String(s.0.to_string()),
-            _ => bail!("no support for {:?} literal yet", self),
+impl TypedAPIConverter<Literal> for weedle::literal::DefaultValue<'_> {


Since you only impl this for a single type, I wonder if it would be simpler to have convert_default_value as a standalone function to begin with, rather than making it a trait. Or, do you anticipate adding more impls for this shortly?

I'm not anticipating any additional implementations. I've made the change to a standalone function as suggested.

rfk · 2020-09-18T08:11:23Z

uniffi_bindgen/src/interface/mod.rs

-    // TODO: more types of literal
+    UInt(u64, Radix, Type),
+    Int(i64, Radix, Type),
+    Float(String, Type),


Naively, I was expecting to see separate UInt8, UInt16 etc variants on this enum. I don't object to this more succinct representation if it makes the code cleaner, but it may be worth a short comment here to explain the choice since it stands out as being a bit different from other type-related enums.

Added extra comments around these representations.

Thanks for the extra comments! I also like your approach of using the widest representation possible, and truncating to the actual type in the binding backends, to simplify the code.

linabutler

Wow, this is fantastic, @jhugman! Everything looks solid, and the battery of tests is great.

It would be super helpful to have for the JS and C++ backends, too, which already have some machinery for returning a dummy default value for methods that throw a JS exception—all that code can piggyback on your default literals now.

Ship it!

linabutler · 2020-09-21T19:27:42Z

examples/rondpoint/src/rondpoint.idl

+  u64 sinon_u64_dec(optional u64 value = 42);
+  i64 sinon_i64_dec(optional i64 value = 42);
+
+  // Hexadecimal, including negatgives.


Suggested change

// Hexadecimal, including negatgives.

// Hexadecimal, including negatives.

linabutler · 2020-09-21T19:33:10Z

uniffi_bindgen/src/bindings/kotlin/gen_kotlin.rs

+
+        Ok(match literal {
+            Literal::Boolean(v) => format!("{}", v),
+            Literal::String(s) => format!("\"{}\"", s),


Do we need to worry about escaping internal quotes (maybe more of an issue for Python single-quoted strings), or handling binary or Unicode escape sequences? It's definitely an edge case, especially for Nimbus and our other Rust components, so "no" is a totally acceptable answer—but it came to mind reading this.

😷

It appears weedle doesn't let us escape quotes,

/// Represents a string value /// /// Follow `/"[^"]*"/` #[derive(Copy)] struct StringLit<'a>( &'a str = ws!(do_parse!( char!('"') >> s: take_while!(|c| c != '"') >> char!('"') >> (s) )), )

I added some comments at the frontend, and changed the quotes in the python backend.

linabutler · 2020-09-21T19:37:27Z

uniffi_bindgen/src/bindings/kotlin/gen_kotlin.rs

+    pub fn literal_kt(literal: &Literal) -> Result<String, askama::Error> {
+        fn typed_number(type_: &Type, num_str: String) -> Result<String, askama::Error> {
+            Ok(match type_ {
+                // special case Int32.


Hmmm, did this comment come from the Swift backend?

linabutler · 2020-09-21T19:39:23Z

uniffi_bindgen/src/bindings/swift/gen_swift.rs

+                match radix {
+                    Radix::Octal => format!("0o{:o}", i),
+                    Radix::Decimal => format!("{}", i),
+                    Radix::Hexadecimal => format!("{:#x}", i),
+                },
+            )?,
+            Literal::UInt(i, radix, type_) => typed_number(
+                type_,
+                match radix {
+                    Radix::Octal => format!("0o{:o}", i),
+                    Radix::Decimal => format!("{}", i),
+                    Radix::Hexadecimal => format!("{:#x}", i),
+                },


That sounds like a fine idea! This reads very cleanly, and I think the duplication trade-off is more than okay.

linabutler · 2020-09-21T19:43:48Z

uniffi_bindgen/src/interface/mod.rs

+        let src_radix = radix as u32;
+        // This radix tells the backends how to represent the number in the output languages.
+        let dest_radix = if string == "0" || string.starts_with('-') {
+            // 1. weedle parses "0" as an octal literal, but we most likely want to treat this as a decimal.


😬 at "parses '0' as an octal literal"

linabutler · 2020-09-21T20:22:36Z

uniffi_bindgen/src/interface/mod.rs

+            radix
+        };
+
+        let trimmer = Regex::new("^(-?)(0x)?").unwrap();


I'm not too worried about this, since we're still prototyping and might eventually use more complex regexes elsewhere—but bringing in regex can bump up compile times. For prefix-stripping like this, I wonder if we can use plain-ol' string functions, maybe something like this:

let string = if string.starts_with('-') { ("-".to_string() + string[1..].strip_prefix("0x").unwrap_or(&string[1..])).to_lowercase() } else { string.strip_prefix("0x").unwrap_or(&string).to_lowercase() };

WDYT?

Oh, magic.

Not since Java 1.3 have I had to think about not having regular expressions.

linabutler · 2020-09-21T20:26:40Z

uniffi_bindgen/src/interface/mod.rs

-    // TODO: more types of literal
+    UInt(u64, Radix, Type),
+    Int(i64, Radix, Type),
+    Float(String, Type),


Thanks for the extra comments! I also like your approach of using the widest representation possible, and truncating to the actual type in the binding backends, to simplify the code.

…ull)

jhugman commented Sep 16, 2020

View reviewed changes

jhugman requested review from linabutler and eoger September 16, 2020 17:33

jhugman self-assigned this Sep 16, 2020

jhugman force-pushed the 263-default-arguments branch from a477d03 to b2ce9ee Compare September 16, 2020 18:10

rfk reviewed Sep 18, 2020

View reviewed changes

jhugman force-pushed the 263-default-arguments branch from b2ce9ee to 330588d Compare September 21, 2020 13:46

linabutler approved these changes Sep 21, 2020

View reviewed changes

jhugman added 14 commits September 21, 2020 22:44

Add non-number literals for defaults (String, bool, empty sequence, n…

9476dcd

…ull)

Whitespace

5f79f9d

Add non-literals for defaults in swift and python

bff668e

Add support for default arguments, for bool, string, null, and integers

319bdde

Add support for default arguments of float types

79a495f

Add support for default arguments of empty map

091bb30

Add support for enums

64f959a

Add support of nullable types, -ve hex numbers, octal

f1e347d

Swift bindings tests

75775ff

Kotlin bindings + tests

6fdfb97

cargo fmt

0c52928

Moved Float literals to use the string typed by the user

933e999

Address reviewer comments

65a5700

Add reviewer 2 comments

eed96d0

jhugman force-pushed the 263-default-arguments branch from 330588d to eed96d0 Compare September 21, 2020 22:28

jhugman merged commit 3913c61 into mozilla:main Sep 21, 2020

jhugman deleted the 263-default-arguments branch September 21, 2020 22:31

This was referenced Sep 28, 2020

Add optional defaults for arguments and properties in Python #300

Closed

Figure out optional dictionary fields #188

Closed

jhugman mentioned this pull request Oct 6, 2020

Add defaults for properties in Records #316

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add default literals for `optional` arguments #283

Add default literals for `optional` arguments #283

jhugman commented Sep 16, 2020

jhugman left a comment

jhugman Sep 16, 2020

rfk Sep 18, 2020 •

edited

Loading

jhugman Sep 21, 2020

jhugman Sep 16, 2020

linabutler Sep 21, 2020

jhugman Sep 16, 2020

jhugman Sep 16, 2020

jhugman Sep 16, 2020

rfk left a comment

rfk Sep 18, 2020

rfk Sep 18, 2020 •

edited

Loading

rfk Sep 18, 2020

rfk Sep 18, 2020

jhugman Sep 21, 2020

rfk Sep 18, 2020

jhugman Sep 21, 2020

jhugman Sep 21, 2020

rfk Sep 18, 2020

jhugman Sep 21, 2020

rfk Sep 18, 2020

jhugman Sep 21, 2020

linabutler Sep 21, 2020

linabutler left a comment

linabutler Sep 21, 2020

linabutler Sep 21, 2020

jhugman Sep 21, 2020

linabutler Sep 21, 2020

linabutler Sep 21, 2020

linabutler Sep 21, 2020

linabutler Sep 21, 2020

jhugman Sep 21, 2020

linabutler Sep 21, 2020

	// Hexadecimal, including negatgives.
	// Hexadecimal, including negatives.

Add default literals for optional arguments #283

Add default literals for optional arguments #283

Conversation

jhugman commented Sep 16, 2020

jhugman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfk Sep 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfk Sep 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linabutler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add default literals for `optional` arguments #283

Add default literals for `optional` arguments #283

rfk Sep 18, 2020 •

edited

Loading

rfk Sep 18, 2020 •

edited

Loading