Add TryFrom to convert from Varargs to tuples #886

B-head · 2022-05-04T23:43:07Z

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.

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.

godot-rust#886

Bromeon

Thanks for this! 🙂

Maybe a general question, what was your use case for this feature?

It seems like type safety is the main reason, i.e. not dealing with Variant but direct Rust types. On the other hand, this is quite a low-level API (implementing Method trait), and for type safety we also have #[derive(FromVarargs)] struct Args which is only slightly more verbose. I'm a bit hesitant to heavily invest in convenience of the low-level builder APIs, as most people use the proc-macros, and a lot of this might become entirely obsolete with GDExtension 🤔

This doesn't support match patterns (for different number of arguments), right?
I guess for that we could still use slice patterns, even though that means Variant elements:

match varargs.as_slice() {
    [a, b]            => ...,
    [first, .., last] => ...,
    _                 => ...,
}

gdnative-core/src/export/method.rs

Bromeon · 2022-05-08T09:02:38Z

gdnative-core/src/export/method.rs

+/// All possible error types for convert from Varargs.
+#[derive(Debug)]
+pub enum VarargsError {
+    ArgumentLengthError(ArgumentLengthError),
+    FromVariantError(FromVariantError),
+}


Do we really need nested types here?

We have struct-like enumerators for FromVariantError and it seems to work well.

We also don't really need all the From<T> implementations -- as I see it, they provide minor convenience for this one file, at the cost of much more code.

That's what I missed! 🤣

However, FromVariantError does not appear to be specialized to arguments error messages. In Rust, Change of type enum is breaking change, so it cannot be changed immediately either. So it looks like we will create an ArgumentTypeError that is specialized for argument errors and VarargsError will remain nested.

It could also be done like this, no?

#[derive(Debug)] pub enum VarargsError { ArgumentLengthError { passed: usize, expected: usize, }, FromVariantError(FromVariantError), }

Or if you find ArgumentTypeError more meaningful than FromVariantError, you could also do a struct-like enumerator for that.

Unless of course we find a good reason to reuse those types, but they seem used isolated.

I did not do that, because if we write the structure directly to enum, we would not be able to write methods for each errors.

test/src/test_register.rs

Bromeon · 2022-05-08T09:21:09Z

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.

B-head · 2022-05-09T18:41:45Z

Maybe a general question, what was your use case for this feature?

My use case is refactoring of the #[methods] macro. Since macros will generate manual binding code, improve manual binding will improve macros.

However, this is a failure because it lacks a bit of flexibility. 😅 Nevertheless, the PR was created because it is useful.

a lot of this might become entirely obsolete with GDExtension 🤔

Don't worry, the types passed as arguments are the same. See header (GDNativeVariantPtr is void*).

This doesn't support match patterns (for different number of arguments), right? I guess for that we could still use slice patterns, even though that means Variant elements:
match varargs.as_slice() {
    [a, b]            => ...,
    [first, .., last] => ...,
    _                 => ...,
}

Interesting, but a, b, first and last are still &Variant types, so more type conversion code is needed.

Bromeon · 2022-05-09T19:10:41Z

My use case is refactoring of the #[methods] macro. Since macros will generate manual binding code, improve manual binding will improve macros.

I see, but that on its own is not a reason to make that a public and documented API. If it's only used by the proc-macro, it can still be #[doc(hidden)], no?

In other words: why does a user need this, when there is already the safer ClassBuilder::method() API?

Don't worry, the types passed as arguments are the same. See header (GDNativeVariantPtr is void*).

Thanks for checking! However, both being void* doesn't mean they refer to the same types -- it literally means "pointer to anything" 🙂

So, we would need to check that. What I meant is that with the current bottom-up approach toward GDExtension, I'm not sure yet which parts of the code can be kept (and at what cost). A lot of stuff has accumulated over the years, and it's an opportunity to strip down rarely used legacy code, also in light of bigger safety refactorings. So... time will tell, don't be discouraged if not everything will be taken over to GDExtension 😉

B-head · 2022-05-09T19:25:28Z

In other words: why does a user need this, when there is already the safer ClassBuilder::method() API?

This PR is replaces the StaticArgs struct, but the Method trait and ClassBuilder::method() can still be used. No unsafe code is needed.

Bromeon · 2022-05-09T21:02:41Z

Sorry, I totally wrote to the wrong PR. I meant to comment on your other changes #887 🙈 hope that makes more sense.

Bogay · 2022-05-10T07:14:17Z

gdnative-core/src/export/method.rs

+        if self.idx < self.args.len() {
+            let idx = self.idx;
+            self.idx += 1;
+            Some(self.args[idx])
+        } else {
+            None
+        }


This part may be simplified by using slice.get?

Suggested change

if self.idx < self.args.len() {

let idx = self.idx;

self.idx += 1;

Some(self.args[idx])

} else {

None

}

let result = self.args.get(self.idx);

self.idx += 1;

result

The test did not pass, so I made some fixes.

Suggested change

if self.idx < self.args.len() {

let idx = self.idx;

self.idx += 1;

Some(self.args[idx])

} else {

None

}

let ret = self.args.get(self.idx);

ret.map(|&v| {

self.idx += 1;

v

})

This is slightly confusing, as map() typically transforms the contents of the Option and has no side effects.

I'd rather do:

let ret = self.args.get(self.idx); if ret.is_some() { self.idx += 1; } ret

In the future there might be Option::inspect() for this purpose, but it's not yet stable.

Fixed. Reflected in next push.

godot-rust#886

B-head · 2022-05-11T22:38:34Z

Note that this PR now depends on #892!

Bromeon · 2022-05-13T21:02:03Z

Then I'd say we sort out #892 first, which would make this PR significantly smaller to review.

Change Varargs struct to store a slice instead of an iterator

Since this is a rarely used feature, a rich encapsulated API for error handling is likely not needed. Most users do not explicitly differentiate error variants, having a descriptive Display impl is usually more important. Supports only 4 expressions for length checks: a, a.., ..=b, a..=b. These are almost always literals and exclusive bounds are likely error prone in this context. Additionally, check_length() with the full range '..' makes no sense, so it's not supported.

Bromeon · 2022-07-13T21:18:05Z

Rebased on master, completed the PR and integrated my change suggestions.
Thanks again for your work! 👍

bors r+

bors · 2022-07-13T21:32:08Z

Build succeeded:

full-ci

B-head added a commit to B-head/godot-rust that referenced this pull request May 4, 2022

Add TryFrom to convert from Varargs to tuples

fd52458

godot-rust#886

B-head force-pushed the varargs_to_tuple branch from ef00e44 to 90947bd Compare May 4, 2022 23:44

B-head marked this pull request as draft May 5, 2022 07:09

Bromeon reviewed May 8, 2022

View reviewed changes

Bromeon added feature Adds functionality to the library c: core Component: core (mod core_types, object, log, init, ...) labels May 8, 2022

Bogay reviewed May 10, 2022

View reviewed changes

B-head added a commit to B-head/godot-rust that referenced this pull request May 11, 2022

Add TryFrom to convert from Varargs to tuples

63dd353

godot-rust#886

B-head force-pushed the varargs_to_tuple branch from 90947bd to f6fe765 Compare May 11, 2022 21:41

B-head marked this pull request as ready for review May 11, 2022 22:20

Bromeon mentioned this pull request May 13, 2022

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

Closed

Bromeon added c: export Component: export (mod export, derive) and removed c: core Component: core (mod core_types, object, log, init, ...) labels May 13, 2022

B-head and others added 2 commits July 13, 2022 19:34

Allow to convert Varargs to tuples

619e7d4

Change Varargs struct to store a slice instead of an iterator

Bromeon force-pushed the varargs_to_tuple branch from f6fe765 to 461b72e Compare July 13, 2022 21:15

bors bot merged commit ebf4427 into godot-rust:master Jul 13, 2022

chitoyuu mentioned this pull request Dec 6, 2022

Simplify Varargs API #988

Open

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TryFrom to convert from Varargs to tuples #886

Add TryFrom to convert from Varargs to tuples #886

B-head commented May 4, 2022 •

edited

Loading

Bromeon left a comment •

edited

Loading

Bromeon May 8, 2022

B-head May 9, 2022

Bromeon May 9, 2022

B-head May 11, 2022

Bromeon commented May 8, 2022

B-head commented May 9, 2022

Bromeon commented May 9, 2022

B-head commented May 9, 2022 •

edited

Loading

Bromeon commented May 9, 2022

Bogay May 10, 2022 •

edited

Loading

B-head May 11, 2022

Bromeon May 13, 2022

B-head May 13, 2022

B-head commented May 11, 2022

Bromeon commented May 13, 2022

Bromeon commented Jul 13, 2022

bors bot commented Jul 13, 2022

Add TryFrom to convert from Varargs to tuples #886

Add TryFrom to convert from Varargs to tuples #886

Conversation

B-head commented May 4, 2022 • edited Loading

Feature

Compatibility

Bromeon left a comment • edited Loading

Choose a reason for hiding this comment

Bromeon May 8, 2022

Choose a reason for hiding this comment

B-head May 9, 2022

Choose a reason for hiding this comment

Bromeon May 9, 2022

Choose a reason for hiding this comment

B-head May 11, 2022

Choose a reason for hiding this comment

Bromeon commented May 8, 2022

B-head commented May 9, 2022

Bromeon commented May 9, 2022

B-head commented May 9, 2022 • edited Loading

Bromeon commented May 9, 2022

Bogay May 10, 2022 • edited Loading

Choose a reason for hiding this comment

B-head May 11, 2022

Choose a reason for hiding this comment

Bromeon May 13, 2022

Choose a reason for hiding this comment

B-head May 13, 2022

Choose a reason for hiding this comment

B-head commented May 11, 2022

Bromeon commented May 13, 2022

Bromeon commented Jul 13, 2022

bors bot commented Jul 13, 2022

B-head commented May 4, 2022 •

edited

Loading

Bromeon left a comment •

edited

Loading

B-head commented May 9, 2022 •

edited

Loading

Bogay May 10, 2022 •

edited

Loading