Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 5 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
94c3738
Change `Varargs` to hold slices
B-head May 4, 2022
f2e2823
Add arguments check errors
B-head May 11, 2022
5ae565a
Add `get()` and `check_length()` to `Varargs`
B-head May 11, 2022
b4fc5aa
Add `get_rest()` to `Varargs`
B-head May 11, 2022
1ebebb2
Add a test for `Varargs::get()`
B-head May 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
310 changes: 300 additions & 10 deletions gdnative-core/src/export/method.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
//! Method registration

use std::borrow::Cow;
use std::convert::TryInto;
use std::fmt;
use std::marker::PhantomData;
use std::ops::{Bound, RangeBounds};

use crate::core_types::{FromVariant, FromVariantError, Variant};
use crate::export::class::NativeClass;
Expand Down Expand Up @@ -212,14 +214,15 @@ impl<C: NativeClass, F: StaticArgsMethod<C>> Method<C> for StaticArgs<F> {
/// for common operations with them. Can also be used as an iterator.
pub struct Varargs<'a> {
idx: usize,
iter: std::slice::Iter<'a, &'a Variant>,
args: &'a [&'a Variant],
offset_index: usize,
}

impl<'a> Varargs<'a> {
/// Returns the amount of arguments left.
#[inline]
pub fn len(&self) -> usize {
self.iter.len()
self.args.len() - self.idx
}

#[inline]
Expand Down Expand Up @@ -250,7 +253,7 @@ impl<'a> Varargs<'a> {
/// Returns the remaining arguments as a slice of `Variant`s.
#[inline]
pub fn as_slice(&self) -> &'a [&'a Variant] {
self.iter.as_slice()
self.args
}

/// Discard the rest of the arguments, and return an error if there is any.
Expand Down Expand Up @@ -284,16 +287,303 @@ impl<'a> Varargs<'a> {
let args = std::mem::transmute::<&[*mut sys::godot_variant], &[&Variant]>(args);
Self {
idx: 0,
iter: args.iter(),
args,
offset_index: 0,
}
}

/// Check the length of arguments.
/// See `get()`, `get_opt()` or `get_rest()` for examples.
///
/// # Errors
/// Returns an error if the length of arguments is outside the specified range.
#[inline]
pub fn check_length(&self, bounds: impl RangeBounds<usize>) -> Result<(), ArgumentLengthError> {
let passed = self.args.len();
if bounds.contains(&passed) {
Ok(())
} else {
Err(ArgumentLengthError::new(passed, bounds))
}
}

/// Returns the type-converted value at the specified argument position.
///
/// # Errors
/// Returns an error if the conversion fails or the argument is not set.
///
/// # Examples
/// ```
/// # fn foo(args: gdnative::export::Varargs) -> Result<(), Box<dyn std::error::Error>> {
/// args.check_length(2..=2)?;
/// let a: usize = args.get(0)?;
/// let b: i64 = args.get(1)?;
/// # Ok(())
/// # }
/// ```
#[inline]
pub fn get<T: FromVariant>(&self, index: usize) -> Result<T, ArgumentTypeError> {
let relative_index = index;
let actual_index = index + self.offset_index;

match self.args.get(relative_index) {
Some(v) => match T::from_variant(v) {
Ok(ok) => Ok(ok),
Err(err) => Err(ArgumentTypeError::new(actual_index, err)),
},
None => {
let err = FromVariantError::Custom("Argument is not set".to_owned());
Err(ArgumentTypeError::new(actual_index, err))
}
}
}

/// Returns the type-converted value at the specified argument position.
/// Returns `None` if the argument is not set.
Comment on lines +341 to +342
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

///
/// # Errors
/// Returns an error if the conversion fails.
///
/// # Examples
/// ```
/// # fn foo(args: gdnative::export::Varargs) -> Result<(), Box<dyn std::error::Error>> {
/// args.check_length(1..=2)?;
/// let a: usize = args.get(0)?;
/// let b: i64 = args.get_opt(1)?.unwrap_or(72);
/// # Ok(())
/// # }
/// ```
#[inline]
pub fn get_opt<T: FromVariant>(&self, index: usize) -> Result<Option<T>, ArgumentTypeError> {
let relative_index = index;
let actual_index = index + self.offset_index;

match self.args.get(relative_index) {
Some(v) => match T::from_variant(v) {
Ok(ok) => Ok(Some(ok)),
Err(err) => Err(ArgumentTypeError::new(actual_index, err)),
},
None => Ok(None),
}
}

/// Returns the type-converted value from the specified argument position.
/// Can be converted to any type that implements TryFrom<Varargs>.
///
/// # Errors
/// Returns an error if the conversion fails.
///
/// # Examples
/// ```ignore
/// # fn foo(args: gdnative::export::Varargs) -> Result<(), Box<dyn std::error::Error>> {
/// args.check_length(1..)?;
/// let a: usize = args.get(0)?;
/// let rest: Vec<i64> = args.get_rest(1)?;
/// # Ok(())
/// # }
/// ```
#[inline]
pub fn get_rest<T>(&self, rest_index: usize) -> Result<T, <Varargs<'a> as TryInto<T>>::Error>
where
Varargs<'a>: TryInto<T>,
{
let relative_rest_index = rest_index;
let actual_rest_index = rest_index + self.offset_index;

let rest = self.args.get(relative_rest_index..).unwrap_or_default();
let varargs = Varargs::<'a> {
idx: 0,
args: rest,
offset_index: actual_rest_index,
};
varargs.try_into()
}

/// Get the varargs's offset index.
#[inline]
#[must_use]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. there is a high chance that the user accidentally forgets a return value
  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🙃

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

pub fn offset_index(&self) -> usize {
self.offset_index
}
}

impl<'a> Iterator for Varargs<'a> {
type Item = &'a Variant;
#[inline]
fn next(&mut self) -> Option<Self::Item> {
self.iter.next().copied()
let ret = self.args.get(self.idx).copied();
if ret.is_some() {
self.idx += 1;
}
ret
}
}

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

impl std::error::Error for VarargsError {}
impl std::fmt::Display for VarargsError {
#[inline]
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match &self {
VarargsError::ArgumentTypeError(e) => e.fmt(f),
VarargsError::ArgumentLengthError(e) => e.fmt(f),
}
}
}

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)
}
}
Comment on lines +440 to +452
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this.

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

Is this okay?

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/// Error to incorrect type of argument.
/// Displays a message containing the position of the argument and cause of the failure to convert.
#[derive(Debug)]
pub struct ArgumentTypeError {
index: usize,
nested_error: FromVariantError,
}

impl ArgumentTypeError {
/// Create a new error with the argument position and `FromVariantError`.
#[inline]
#[must_use]
pub fn new(index: usize, nested_error: FromVariantError) -> Self {
Self {
index,
nested_error,
}
}

/// Returns an ordinal number representation.
#[inline]
#[must_use]
fn ordinal(&self) -> String {
match self.index + 1 {
1 => "1st".to_owned(),
2 => "2nd".to_owned(),
3 => "3rd".to_owned(),
i @ 4.. => format!("{i}th"),
_ => "unknown".to_owned(),
}
}

/// Get the argument type error's index.
#[inline]
#[must_use]
pub fn index(&self) -> usize {
self.index
}

/// Get a reference to the argument type error's nested error.
#[inline]
#[must_use]
pub fn nested_error(&self) -> &FromVariantError {
&self.nested_error
}
}

impl std::error::Error for ArgumentTypeError {}
impl std::fmt::Display for ArgumentTypeError {
#[inline]
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(
f,
"Incorrect type of {} argument, cause: {}",
self.ordinal(),
self.nested_error,
)
}
}

/// 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>),
}
Comment on lines +514 to +520
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@B-head B-head May 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@Bromeon Bromeon May 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>,
    },
}


impl ArgumentLengthError {
/// Creates a new error with the length of the arguments passed and the expected arguments range.
#[inline]
#[must_use]
pub fn new(passed: usize, expected: impl RangeBounds<usize>) -> Self {
Self {
passed,
expected: (
expected.start_bound().cloned(),
expected.end_bound().cloned(),
),
}
}

/// Get the argument length error's passed.
#[inline]
#[must_use]
pub fn passed(&self) -> usize {
self.passed
}

/// Get the argument length error's expected min.
#[inline]
#[must_use]
pub fn expected_min(&self) -> usize {
match self.expected.0 {
Bound::Included(s) => s,
Bound::Excluded(s) => s + 1,
Bound::Unbounded => usize::MIN,
}
}

/// Get the argument length error's expected max.
#[inline]
#[must_use]
pub fn expected_max(&self) -> usize {
match self.expected.1 {
Bound::Included(e) => e,
Bound::Excluded(e) => e - 1,
Bound::Unbounded => usize::MAX,
}
}
}

impl std::error::Error for ArgumentLengthError {}
impl std::fmt::Display for ArgumentLengthError {
#[inline]
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let expected_msg = match (self.expected_min(), self.expected_max()) {
(usize::MIN, usize::MAX) => "any".to_owned(),
(usize::MIN, e) => format!("max {e}"),
(s, usize::MAX) => format!("min {s}"),
(s, e) => {
if s == e {
s.to_string()
} else {
format!("min {s} and max {e}")
}
}
};
write!(
f,
"Argument lengths do not match, passed {}, expected {}",
self.passed, expected_msg
)
}
}

Expand Down Expand Up @@ -361,10 +651,11 @@ impl<'r, 'a, T: FromVariant> ArgBuilder<'r, 'a, T> {
#[inline]
pub fn get(mut self) -> Result<T, ArgumentError<'a>> {
self.get_optional_internal().and_then(|arg| {
let actual_index = self.args.idx + self.args.offset_index;
arg.ok_or(ArgumentError {
site: self.site,
kind: ArgumentErrorKind::Missing {
idx: self.args.idx,
idx: actual_index,
name: self.name,
},
})
Expand All @@ -389,14 +680,13 @@ impl<'r, 'a, T: FromVariant> ArgBuilder<'r, 'a, T> {
ty,
..
} = self;
let idx = args.idx;
let actual_index = args.idx + args.offset_index;

if let Some(arg) = args.iter.next() {
args.idx += 1;
if let Some(arg) = args.next() {
T::from_variant(arg).map(Some).map_err(|err| ArgumentError {
site: *site,
kind: ArgumentErrorKind::CannotConvert {
idx,
idx: actual_index,
name: name.take(),
value: arg,
ty: ty
Expand Down
Loading