-
Notifications
You must be signed in to change notification settings - Fork 211
Add methods in Varargs
to perform arguments validation and type conversion
#892
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.
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
Changes from all commits
94c3738
f2e2823
5ae565a
b4fc5aa
1ebebb2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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; | ||
|
@@ -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] | ||
|
@@ -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. | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 /// 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] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would reserve
I don't think any of the uses in this file qualifies for these criteria. Simple getters or even Some examples where we used it:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
The standard library recently did that. So even Actually, when I auto-generated the getter, it just generated the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's actually a good example! However, I think we agreed that storing the iterator state in
Interesting, wasn't aware of the standard library doing that, as 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
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need these See also my comment in the other PR #886 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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))?,
)* There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Errors in godot-rust typically don't have rich APIs, e.g.
...
/// 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 In 99% of the cases, a user would not even need to access these values programmatically, but simply print the error message. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In that case, the field becomes mutable.
Which is the better choice? 🤔
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.
Will you investigate demand later? I think that would be more labor intensive. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, just like the
What do you mean here? We expose all the relevant info with
First, a user expects similar functionality to have similar APIs. /// 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 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Serious question: why don't we go with this? It's like 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 | ||
) | ||
} | ||
} | ||
|
||
|
@@ -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, | ||
}, | ||
}) | ||
|
@@ -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 | ||
|
Uh oh!
There was an error while loading. Please reload this page.