Simplify Varargs API

[chitoyuu]

While #886 was an interesting addition to the library, some of its decisions are dubious in necessity:

• The addition of gdnative::export::IndexBounds, whose functionalities largely duplicate the standard std::ops::RangeBounds trait.
• The use of TryFrom for tuple impls, which as opposed to the existing FromVarargs:
  • Can only report a single error, instead of all the errors.
  • Does not have the same composability (FromVarargs types can be chained for common argument sets).
  • Is unused anywhere else.
• Introduction of a new VarargsError type, that:
  • Does not work with the existing gdnative::export::ArgumentError type, but duplicates its functionality instead.
  • Lacks the ability to be reported with a Site.
  • Is unused anywhere else, anyway.
• Addition of APIs that needlessly expose internals, and can complicate future changes.
  • Method arguments are meant to be accessed sequentially, required ones followed by optional ones. FromVariant conversions are meant to be done only once, because otherwise it's just unnecessary inefficiency. Accessing Varargs in any other way is most certainly a user error, and should not be enabled by the API design.
  • That Varargs is internally a slice should have remained an implementation detail, because there exist reasons why we might want to change that later, like in case we uncovered a source of UB during the conversion.

We probably want to re-consider how much of these are actually useful to the end user, and deprecate/remove things that are mostly cruft.

[Bromeon]

I was never fully happy with the Varargs API and would definitely welcome a more elegant approach to the problem. I agree that some parts are unnecessarily specific and can be unified with other parts of the gdnative API surface. Breaking changes are appropriate here, similar to how 0.10 simplified a lot of things in module structure and naming.

Note that 461b72e was already a first attempt at cleaning some parts up, but we could have gone further. Some notable points:

1. std::ops::RangeBounds was considered, but rejected because it allows exclusive bounds. Since this is only used in length checks, having two syntaxes makes it easy to allow off-by-one errors.

   • That said, it's questionable if an extra type like IndexBounds is necessary at all, same about the length check. If the latter is still desired, even something like check_length(usize, usize) function + half-open overloads could do the job.

2. We should probably have IntoIterator rather than Iterator.

3. ArgumentError<'a> type

   • It type-erases the concrete error, which is probably OK given almost everyone just wants the message, but it's still in contrast with other error types like FromVariantError.
   • Do we truly need the lifetime here? I see that Site<'a> has it, but do you think it would be too expensive to have it owned? Given that we're explicitly in an error path of the program.

4. One thing that always bothered me a bit is that we have both Varargs and VariantArray, two conceptually very similar types, but completely disconnected and with different APIs.

   • One main difference is that Varargs<'a> borrows the arguments and thus holds a lifetime. This is probably something we want to keep.
   • The other thing is that Varargs distinguishes required from optional parameters.
   • Even if the two types stay distinct, there could be some interop, e.g. treating VariantArray as a borrowed Varargs<'a> or copying into an owned VariantArray -- a bit similar to Cow<'a, T>. Just brainstorming, would need some discussion, might be that such functionality is not needed after all.

Thanks a lot for the initiative! 👍

[chitoyuu]

That said, it's questionable if an extra type like IndexBounds is necessary at all, same about the length check. If the latter is still desired, even something like check_length(usize, usize) function + half-open overloads could do the job.

Frankly, I'm not sure if a separate length check is really necessary. Sure, doing the check before any FromVariant conversions can save a few cycles when the check fails, but we're talking about a failed method call here. In all but the weirdest cases, this would mean that either the GDScript call or the Rust definition is bugged, the game isn't working as intended, and so performance is irrelevant.

If one really wants to do the check first before reading anything (for some JavaScript-tier method overloading, maybe), they can still just do (1..=3).contains(varargs.len()), without all this complexity.

2. We should probably have IntoIterator rather than Iterator.

It would be nice to separate inherent access methods from the Iterator stuff, yes.

ArgumentError<'a> type

* It type-erases the concrete error, which is probably OK given almost everyone just wants the message, but it's still in contrast with other error types like [`FromVariantError`](https://godot-rust.github.io/docs/gdnative/core_types/enum.FromVariantError.html).

* Do we truly need the lifetime here? I see that `Site<'a>` has it, but do you think it would be too expensive to have it owned? Given that we're explicitly in an error path of the program.

I think the reason here is that ArgumentErrorKind as it is currently implemented borrows a lot of stuff from the Varargs and ArgBuilder from which it was spawned, and the latter types do need all the lifetimes. Borrowing the Site is just kind of a "why-not" given this background. For the same reason it isn't quite suitable to be made public as is.

I suppose it can be modified to drop the lifetime and become public, but I struggle to think of a way to actually make use of that information in user code. Whatever weird thing a user is trying to accomplish, there's probably a better way to do it than try-catching argument errors, even though I don't know what that thing is.

In any case, performance is likely irrelevant here, so there isn't any reason to oppose the idea either.

• Even if the two types stay distinct, there could be some interop, e.g. treating VariantArray as a borrowed Varargs<'a> or copying into an owned VariantArray -- a bit similar to Cow<'a, T>. Just brainstorming, would need some discussion, might be that such functionality is not needed after all.

Remember that VariantArrays aren't Vecs. Unfortunately, the former conversion wouldn't be safe in a lot of cases, because VariantArrays can point to shared memory. Even for unique VariantArrays, we'll need to either add a generic parameter to Varargs, or use a trait object for the underlying storage. Both options aren't very ideal.

The latter should be possible through args.into_iter().collect().

[Bromeon]

Frankly, I'm not sure if a separate length check is really necessary. Sure, doing the check before any FromVariant conversions can save a few cycles when the check fails, but we're talking about a failed method call here. In all but the weirdest cases, this would mean that either the GDScript call or the Rust definition is bugged, the game isn't working as intended, and so performance is irrelevant.

Good reasoning, let's remove it then 🙂

The latter should be possible through args.into_iter().collect().

Ah, right. These iterator combinations are sometimes not very obvious, maybe this could be mentioned in the Varargs struct docs.