Add methods in `Varargs` to perform arguments validation and type conversion #892

B-head · 2022-05-11T22:15:24Z

Feature

Add convenience methods to perform argument validation and type conversion. This makes manual binding easier to write than ever before.

Normal arguments only:

fn foo(args: Varargs) -> Result<(), Box<dyn Error>> {
    args.check_length(2)?;
    let a: usize = args.get(0)?;
    let b: i64 = args.get(1)?;
    Ok(())
}

Contains optional arguments:

fn foo(args: Varargs) -> Result<(), Box<dyn Error>> {
    args.check_length(1..=2)?;
    let a: usize = args.get(0)?;
    let b: i64 = args.get_opt(1)?.unwrap_or(72);
    Ok(())
}

Contains the rest of arguments:

fn foo(args: Varargs) -> Result<(), Box<dyn Error>> {
    args.check_length(1..)?;
    let a: usize = args.get(0)?;
    let rest: Vec<i64> = args.get_rest(1)?; // There is no `TryInto<Vec<T>>` implementation yet.
    Ok(())
}

List of APIs to add

`Varargs::check_length()`

Check the length of arguments. Returns an error if the length of arguments is outside the specified range.

`Varargs::get()`

Returns the type-converted value at the specified argument position. Returns an error if the conversion fails or the argument is not set.

`Varargs::get_opt()`

Returns the type-converted value at the specified argument position. Returns None if the argument is not set. Returns an error if the conversion fails.

`Varargs::get_rest()`

Returns the type-converted value from the specified argument position. Returns an error if the conversion fails.

Can be converted to any type that implements TryFrom<Varargs>.

`VarargsError`

An enumr for emitting multiple types of errors.

`ArgumentTypeError`

Error to incorrect type of argument. Displays a message containing the position of the argument and cause of the failure to convert.

`ArgumentLengthError`

Error to argument lengths do not match. Display a message containing the length of arguments passed and the expected range.

`ArgumentBounds`

~~The ArgumentBounds trait was redefined only because usize does not implement the RangeBounds<usize> trait.~~

Compatibility

In this PR, only changes that can maintain compatibility are made.

Bromeon · 2022-05-13T20:07:02Z

This is a nice addition!

I think we should be thoughtful in which direction we want to extend the Varargs API -- I already expressed my concerns here:

Btw, the Varargs API as it exists now in godot-rust is a bit strange. It is simultaneously a collection and iterator.

I think the proper way would have been implementing IntoIterator, which could reuse the slice iterator. This would also get rid of storing idx as the iterator state inside Varargs.

That's probably something we can adjust for v0.11.

In short, I think we should avoid maintaining iteration state in Varargs itself. It could simply become a wrapper around a slice:

pub struct Varargs<'a> {
    slice: &'a [&'a Variant], // or &'a [Variant]

    // index for compatibility, will be removed in 0.11
    // we probably don't need iter anymore
    idx: usize,
}

impl<'a> Varargs<'a> {
    /// Returns the remaining arguments as a slice of `Variant`s.
    #[inline]
    pub fn as_slice(&self) -> &'a [&'a Variant] {
        self.slice
    }
}

impl<'a> Iterator for Varargs<'a> {
    type Item = &'a Variant;

    #[inline]
    fn next(&mut self) -> Option<Self::Item> {
        if self.idx >= self.slice.len() {
            None
        } else {
            let variant = self.slice[self.idx]
            self.idx += 1;
            Some(variant)
        }
    }
}

That would mean that the get_rest() method is not needed -- we can retrieve a subslice using [T]::get with a range argument.

By the way, I really appreciate the high quality of your recent PRs, with feature lists, explanations and passing CI. Thanks a lot for that! I feel a bit bad when we can't directly merge things and code needs to be rewritten multiple times. Maybe to avoid this in the future, do you want to suggest an APIs first (in a GitHub issue or on Discord's #dev channel), so we can discuss it and converge on ideas before jumping to implementation? This is of course entirely up to you, sometimes it's worth having a proof-of-concept implementation. But I don't want you to waste time and effort 🙂

gdnative-core/src/export/method.rs

test/src/test_register.rs

This is instead of iterators. Better performance of the code I will write next.

Added `offset_index` so that actual position of arguments is displayed in error messages.

B-head · 2022-05-18T07:23:49Z

I think we should be thoughtful in which direction we want to extend the Varargs API
In short, I think we should avoid maintaining iteration state in Varargs itself. It could simply become a wrapper around a slice:

I agree.

This is of course entirely up to you, sometimes it's worth having a proof-of-concept implementation.

I had planned to create a proof of concept first, but I was too eager. 😅

Bromeon

Also, remaining points mentioned earlier (just for completeness, no worries if you haven't had time to address them yet 😉):

That would mean that the get_rest() method is not needed -- we can retrieve a subslice using [T]::get with a range argument.

Bromeon · 2022-05-21T21:16:22Z

gdnative-core/src/export/method.rs

+
+    /// Get the varargs's offset index.
+    #[inline]
+    #[must_use]


I would reserve #[must_use] for situations, where both:

there is a high chance that the user accidentally forgets a return value

such a forgetting can cause bugs, e.g. by swallowing errors

I don't think any of the uses in this file qualifies for these criteria. Simple getters or even new() do generally not fall in that category, otherwise we'd have to annotate half the library with #[must_use].

Some examples where we used it:

fn test_something() -> bool in Godot integration tests

forgetting to check the result would allow tests to fail silently.

MethodBuilder (struct attribute)

a user could register a method and miss the final done() invocation.

For example, code like this.

args.offset_index(); // No operation

This would be unintended behavior for the person who wrote this, so it makes sense to warn them.

Simple getters or even new() do generally not fall in that category, otherwise we'd have to annotate half the library with #[must_use].

The standard library recently did that. So even godot-rust will have to do so by the time the version reaches 1.0.0.

Actually, when I auto-generated the getter, it just generated the #[must_use] as well, which I didn't intend. 🙃

For example, code like this.

args.offset_index(); // No operation

This would be unintended behavior for the person who wrote this, so it makes sense to warn them.

That's actually a good example! offset in English can be understood as both a verb and noun.

However, I think we agreed that storing the iterator state in Varargs (and thus the offset) is not a great idea -- so I don't see why we add more methods like offset_index()? That only means more breaking code once we move away from it.

Simple getters or even new() do generally not fall in that category, otherwise we'd have to annotate half the library with #[must_use].

The standard library recently did that. So even godot-rust will have to do so by the time the version reaches 1.0.0.

Interesting, wasn't aware of the standard library doing that, as #[must_use] is not part of the documentation. It seems like rust-lang/rust#89692 was a big change. I've also seen the guidelines, but they're not very explicit on these border cases; they basically say "if it's legitimate to ignore the return value, don't add it" which is, well... very obvious.

But I don't understand why godot-rust would have to follow the standard library once it's 1.0.0? While the standard library can be a good inspiration for other libraries, the requirements are quite different -- it has a vast number of users across all audiences and must generally be very conservative regarding compatibility, potential bugs, safety, etc. As an example, we will likely deviate from some "standard practices" because godot-rust is such a FFI-heavy library which needs to deal with threading and unsafety out of its control. There's very little guidance about this use case, even in the Nomicon. So I don't see the standard library as "the one true library which is right in every sense". We definitely have the freedom to define our own conventions within certain bounds.

Apart from that, it would be nice if changes to code style (which affect the whole library) would take place outside of feature PRs, otherwise we end up with an inconsistent state and an unclear "set of rules". That said, I don't think #[must_use] a priority for godot-rust right now, let's get some other open issues solved first 😉

Actually, when I auto-generated the getter, it just generated the #[must_use] as well, which I didn't intend. 🙃

That's surprising, 've never seen that! Out of curiouity, which IDE do you use?

gdnative-core/src/export/method.rs

Bromeon · 2022-05-21T21:25:44Z

gdnative-core/src/export/method.rs

+    /// Returns the type-converted value at the specified argument position.
+    /// Returns `None` if the argument is not set.


Maybe mention that this is for optional parameters?

Also, we should probably clearly distinguish parameters (at declaration site) and arguments (at call site).

/// Returns the type-converted value at the specified (optional) parameter position. /// This is for optional parameters; the method returns `Ok(None)` if the argument is not set.

For get on the other hand, you could mention:

/// Returns the type-converted value at the specified parameter position. /// This is for required parameters; the method returns `Err` if the argument is missing.

Bromeon · 2022-05-21T21:31:07Z

gdnative-core/src/export/method.rs

+impl From<ArgumentTypeError> for VarargsError {
+    #[inline]
+    fn from(value: ArgumentTypeError) -> Self {
+        Self::ArgumentTypeError(value)
+    }
+}
+
+impl From<ArgumentLengthError> for VarargsError {
+    #[inline]
+    fn from(value: ArgumentLengthError) -> Self {
+        Self::ArgumentLengthError(value)
+    }
+}


Do we really need these From impls? I don't think it's a big issue to write VarargsError::ArgumentTypeError(my_error) instead of my_error.into() 🤔

See also my comment in the other PR #886 (comment)

Instead of this.

$(args.get::<$params>(inc())?,)*

Is this okay?

$( args.get::<$params>(inc()) .map_err(|e| VarargsError::ArgumentTypeError(e))?, )*

Yes, absolutely! It's anyway encapsulated in a macro 🙂

Bromeon · 2022-05-21T21:49:41Z

gdnative-core/src/export/method.rs

+/// Error to argument lengths do not match.
+/// Display a message containing the length of arguments passed and the expected range of lengths.
+#[derive(Debug)]
+pub struct ArgumentLengthError {
+    passed: usize,
+    expected: (Bound<usize>, Bound<usize>),
+}


Errors in godot-rust typically don't have rich APIs, e.g. FromVariantError is simply an enum without methods.

FromVariantError::InvalidLength in particular is a good inspiration for this implementation:

... /// Length of the collection is different from the expected one. InvalidLength { len: usize, expected: usize }, ...

I think we could simply make the fields public here, and use a more similar naming:

#[derive(Debug)] pub struct InvalidArgumentLength { // or *Count? pub len: usize, pub expected_min: Bound<usize>, pub expected_max: Bound<usize>, }

Then, we would not need the 3 methods passed(), expected_min() and expected_max() -- it's anyway not clear if returning usize::MIN/usize::MAX is intuitive for the user -- and with Bounds, the user can work with a higher-level API.

In 99% of the cases, a user would not even need to access these values programmatically, but simply print the error message.

TLDR: let's keep rarely-used APIs as simple as possible, with only as much code as necessary.
We can always add more features in the future if there is actual demand.

I think we could simply make the fields public here,

In that case, the field becomes mutable.

Then, we would not need the 3 methods passed(), expected_min() and expected_max() -- it's anyway not clear if returning usize::MIN/usize::MAX is intuitive for the user -- and with Bounds, the user can work with a higher-level API.

Which is the better choice? 🤔

let's keep rarely-used APIs as simple as possible, with only as much code as necessary.

If it is an API that users can implement on their own, there is no problem without it, but since it is impossible to implement, this is major limitation for users.

Also, just because there are more lines of code does not mean that they are more complex.

We can always add more features in the future if there is actual demand.

Will you investigate demand later? I think that would be more labor intensive.

In that case, the field becomes mutable.

Yes, just like the FromVariantError enum.

If it is an API that users can implement on their own, there is no problem without it, but since it is impossible to implement, this is major limitation for users.

What do you mean here? We expose all the relevant info with len, expected_min and expected_max being public. We can also keep Display for convenience. The user can do everything, no?

We can always add more features in the future if there is actual demand.

Will you investigate demand later? I think that would be more labor intensive.

First, a user expects similar functionality to have similar APIs. FromVariantError variants provide mutable, publicly accessible fields (implied by being an enum).

/// Length of the collection is different from the expected one. InvalidLength { len: usize, expected: usize }, /// Invalid enum representation. InvalidEnumRepr { expected: VariantEnumRepr, error: Box<FromVariantError>, },

Now VarargsError is completely different: it uses tuple-like enumerators instead of struct-like ones, and its external structs are encapsulating their fields. This inconsistency exists despite both APIs serving an identical purpose.

Regarding the demand part of your question -- I'm a big fan of "keep it simple". No one ever demanded highly encapsulated error APIs. Most people don't even care about the error -- they unwrap() and if there's a panic, it's a bug to be fixed. At most, people may output the error message. I guess we could even use anyhow::Error to throw a string inside, and most users would still be happy. But we're already providing much more than that -- the user can differentiate precisely which error happened and react to it in a very specific way, if necessary. I don't think anything beyond this (such as From, or separate struct types, or encapsulation) adds real value to the library.

If I am wrong in my opinion, people have the option to use the issue tracker. I personally believe it's much less labor-intensive to start with little code and expand when needed, rather than put a lot of up-front effort for a corner-case API and make assumptions how this is going to benefit everyone. And that's without even mentioning maintenance of the code itself.

Serious question: why don't we go with this? It's like FromVariantError does it.

pub enum VarargsError { ArgumentTypeError { index: usize, error: FromVariantError, // or Box }, ArgumentLengthError { len: usize, expected_min: Bound<usize>, expected_max: Bound<usize>, }, }

Bromeon · 2022-06-05T19:08:58Z

@B-head any update on this? If you don't have the capacity right now to make changes, I can gladly implement my suggestions. Would be great to merge this and not let it go stale! 🙂

886: Add TryFrom to convert from Varargs to tuples r=Bromeon a=B-head This PR is stacked on `varargs_gets` branch. See #892 for details on some of the commits included in this PR. ## Feature Dirty syntax to convert to tuples. This makes manual binding easier to write than ever before. Implement via `varargs_into_tuple!()` macro. Can be converted to tuples up to 12 in length. This is the same length supported by standard library. ```rust fn call(&self, _this: TInstance<'_, Foo>, args: Varargs<'_>) -> Variant { // Convert from Varargs to tuples. let (a, b, c): (i64, i64, i64) = args.try_into().unwrap(); let ret = (a * b - c); ret.to_variant() } ``` ## Compatibility Since this PR only adds an API, there are no compatibility issues. Co-authored-by: B_head <b_head@outlook.jp> Co-authored-by: Jan Haller <bromeon@gmail.com>

Bromeon · 2022-07-16T12:59:14Z

This has been merged as part of #886.

B-head mentioned this pull request May 11, 2022

Add TryFrom to convert from Varargs to tuples #886

Merged

Bromeon reviewed May 13, 2022

View reviewed changes

gdnative-core/src/export/method.rs Outdated Show resolved Hide resolved

test/src/test_register.rs Outdated Show resolved Hide resolved

Bromeon added feature Adds functionality to the library c: export Component: export (mod export, derive) labels May 13, 2022

Change Varargs to hold slices

94c3738

This is instead of iterators. Better performance of the code I will write next.

B-head force-pushed the varargs_gets branch from e17c456 to 5326bb1 Compare May 13, 2022 23:38

B-head added 4 commits May 18, 2022 15:55

Add arguments check errors

f2e2823

Add get() and check_length() to Varargs

5ae565a

Add get_rest() to Varargs

b4fc5aa

Added `offset_index` so that actual position of arguments is displayed in error messages.

Add a test for Varargs::get()

1ebebb2

B-head force-pushed the varargs_gets branch from 5326bb1 to 1ebebb2 Compare May 18, 2022 07:06

Bromeon requested changes May 21, 2022

View reviewed changes

Bromeon added this to the v0.10.1 milestone Jul 16, 2022

Bromeon closed this Jul 16, 2022

B-head deleted the varargs_gets branch February 3, 2023 16:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add methods in `Varargs` to perform arguments validation and type conversion #892

Add methods in `Varargs` to perform arguments validation and type conversion #892

B-head commented May 11, 2022 •

edited

Loading

Bromeon commented May 13, 2022

B-head commented May 18, 2022

Bromeon left a comment

Bromeon May 21, 2022

B-head May 24, 2022

Bromeon May 24, 2022

Bromeon May 21, 2022

Bromeon May 21, 2022

B-head May 24, 2022

Bromeon May 24, 2022

Bromeon May 21, 2022

Bromeon May 21, 2022

B-head May 24, 2022 •

edited

Loading

Bromeon May 24, 2022 •

edited

Loading

Bromeon May 24, 2022

Bromeon commented Jun 5, 2022

Bromeon commented Jul 16, 2022

		/// Returns the type-converted value at the specified argument position.
		/// Returns `None` if the argument is not set.

Add methods in Varargs to perform arguments validation and type conversion #892

Add methods in Varargs to perform arguments validation and type conversion #892

Conversation

B-head commented May 11, 2022 • edited Loading

Feature

List of APIs to add

Varargs::check_length()

Varargs::get()

Varargs::get_opt()

Varargs::get_rest()

VarargsError

ArgumentTypeError

ArgumentLengthError

ArgumentBounds

Compatibility

Bromeon commented May 13, 2022

B-head commented May 18, 2022

Bromeon 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

B-head May 24, 2022 • edited Loading

Choose a reason for hiding this comment

Bromeon May 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon commented Jun 5, 2022

Bromeon commented Jul 16, 2022

Add methods in `Varargs` to perform arguments validation and type conversion #892

Add methods in `Varargs` to perform arguments validation and type conversion #892

B-head commented May 11, 2022 •

edited

Loading

`Varargs::check_length()`

`Varargs::get()`

`Varargs::get_opt()`

`Varargs::get_rest()`

`VarargsError`

`ArgumentTypeError`

`ArgumentLengthError`

`ArgumentBounds`

B-head May 24, 2022 •

edited

Loading

Bromeon May 24, 2022 •

edited

Loading