Skip to content

Implement nested composition of FromLuaMulti #287

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

Closed
wants to merge 18 commits into from
Closed
Show file tree
Hide file tree
Changes from 14 commits
Commits
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
42 changes: 34 additions & 8 deletions src/conversion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use crate::thread::Thread;
use crate::types::{LightUserData, Number};
use crate::userdata::{AnyUserData, UserData};
use crate::value::{FromLua, Nil, ToLua, Value};
use crate::{FromLuaMulti, MultiValue};

impl<'lua> ToLua<'lua> for Value<'lua> {
fn to_lua(self, _: Context<'lua>) -> Result<Value<'lua>> {
Expand Down Expand Up @@ -440,10 +441,27 @@ impl<'lua, T: ToLua<'lua>> ToLua<'lua> for Vec<T> {
}
}

impl<'lua, T: FromLua<'lua>> FromLua<'lua> for Vec<T> {
fn from_lua(value: Value<'lua>, _: Context<'lua>) -> Result<Self> {
impl<'lua, T: FromLuaMulti<'lua>> FromLua<'lua> for Vec<T> {
fn from_lua(value: Value<'lua>, lua: Context<'lua>) -> Result<Self> {
if let Value::Table(table) = value {
table.sequence_values().collect()
let mut values: MultiValue = MultiValue::from_vec(
table
.sequence_values::<Value>()
.filter_map(Result::ok) // Every value in Table is always a Value
.collect(),
);
let mut result = Vec::new();
while values.len() > 0 {
let mut consumed = 0;
match T::from_counted_multi(values.clone(), lua, &mut consumed) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cloning the values each time seems like a problem - it means means converting the list is now O(n^2).

I'm thinking aloud (you've looked at this a lot more than I have recently), but it feels like passing &mut values instead of by-value would solve that and also make the consumed count unnecessary as from_counted_multi could just drop_front(...) (or maybe consume(n)) instead.
The downside is that the function could do other weird things like replace it with a different list.
Perhaps a narrower wrapper MultiValueView or something which can only consume items. What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

The main drawback with that approach is that implementations have to manually juggle values (revert them when testing fails). Fallible could do a clone and revert it on error but it complicates the implementation for most other structures.

I tried adding a wrapper as well at some point but it required adding lifetimes to all the traits/functions that deal with FromLuaMulti conversion which felt like it complicates code too much. Though I do think an iterator with "revert last argument" function and "list skipped types" function would be ideal.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that adding extra lifetimes to traits would be awkward, and make updates from previous versions difficult.
But I think the quadratic runtime for long (even non-multi-value) argument lists is also a problem.
That quadratic issue is only because multiple arguments can have a crack a (nearly) the same list, and wouldn't be a problem with the old "take all" version. Maybe the new counted method can take a different type which does behave as an iterator. Perhaps with a "peek next" rather than "revert pop"?

Ok(it) => {
result.push(it);
values.drop_front(consumed);
}
Err(err) => return Err(err),
}
}
Ok(result)
} else {
Err(Error::FromLuaConversionError {
from: value.type_name(),
Expand Down Expand Up @@ -507,11 +525,19 @@ impl<'lua, T: ToLua<'lua>> ToLua<'lua> for Option<T> {
}
}

impl<'lua, T: FromLua<'lua>> FromLua<'lua> for Option<T> {
fn from_lua(value: Value<'lua>, lua: Context<'lua>) -> Result<Self> {
match value {
Nil => Ok(None),
value => Ok(Some(T::from_lua(value, lua)?)),
impl<'lua, T: FromLuaMulti<'lua>> FromLuaMulti<'lua> for Option<T> {
fn from_counted_multi(
values: crate::MultiValue<'lua>,
lua: Context<'lua>,
consumed: &mut usize,
) -> Result<Self> {
let front = values.peek_front();
if front.is_none() || front.map(|it| matches!(it, Nil)).unwrap_or_default() {
return Ok(None);
}
match T::from_counted_multi(values, lua, consumed) {
Ok(it) => Ok(Some(it)),
Err(err) => Err(err),
}
}
}
2 changes: 1 addition & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ pub use crate::error::{Error, ExternalError, ExternalResult, Result};
pub use crate::function::Function;
pub use crate::hook::{Debug, DebugNames, DebugSource, DebugStack, HookTriggers};
pub use crate::lua::{InitFlags, Lua, StdLib};
pub use crate::multi::Variadic;
pub use crate::multi::{Variadic, Fallible};
pub use crate::scope::Scope;
pub use crate::string::String;
pub use crate::table::{Table, TablePairs, TableSequence};
Expand Down
191 changes: 154 additions & 37 deletions src/multi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,18 @@ impl<'lua, T: ToLua<'lua>> ToLuaMulti<'lua> for T {
}

impl<'lua, T: FromLua<'lua>> FromLuaMulti<'lua> for T {
fn from_lua_multi(mut values: MultiValue<'lua>, lua: Context<'lua>) -> Result<Self> {
Ok(T::from_lua(values.pop_front().unwrap_or(Nil), lua)?)
fn from_counted_multi(
mut values: MultiValue<'lua>,
lua: Context<'lua>,
consumed: &mut usize,
) -> Result<Self> {
match values.pop_front() {
Some(it) => {
*consumed += 1;
Ok(T::from_lua(it, lua)?)
}
None => Ok(T::from_lua(Nil, lua)?),
}
}
}

Expand All @@ -45,7 +55,12 @@ impl<'lua> ToLuaMulti<'lua> for MultiValue<'lua> {
}

impl<'lua> FromLuaMulti<'lua> for MultiValue<'lua> {
fn from_lua_multi(values: MultiValue<'lua>, _: Context<'lua>) -> Result<Self> {
fn from_counted_multi(
values: MultiValue<'lua>,
_: Context<'lua>,
consumed: &mut usize,
) -> Result<Self> {
*consumed += values.len();
Ok(values)
}
}
Expand Down Expand Up @@ -85,6 +100,11 @@ impl<T> Variadic<T> {
pub fn new() -> Variadic<T> {
Variadic(Vec::new())
}

#[inline]
pub fn len(&self) -> usize {
self.0.len()
}
}

impl<T> Default for Variadic<T> {
Expand Down Expand Up @@ -128,57 +148,154 @@ impl<'lua, T: ToLua<'lua>> ToLuaMulti<'lua> for Variadic<T> {
}
}

impl<'lua, T: FromLua<'lua>> FromLuaMulti<'lua> for Variadic<T> {
fn from_lua_multi(values: MultiValue<'lua>, lua: Context<'lua>) -> Result<Self> {
values
.into_iter()
.map(|e| T::from_lua(e, lua))
.collect::<Result<Vec<T>>>()
.map(Variadic)
impl<'lua, T: FromLuaMulti<'lua>> FromLuaMulti<'lua> for Variadic<T> {
fn from_counted_multi(
mut values: MultiValue<'lua>,
lua: Context<'lua>,
total: &mut usize,
) -> Result<Self> {
let mut result = Vec::new();
while values.len() > 0 {
let mut consumed = 0;
if let Ok(it) = T::from_counted_multi(values.clone(), lua, &mut consumed) {
result.push(it);
values.drop_front(consumed);
*total += consumed;
} else {
break;
}
}
Ok(Variadic(result))
}
}

macro_rules! impl_tuple {
() => (
impl<'lua> ToLuaMulti<'lua> for () {
fn to_lua_multi(self, _: Context<'lua>) -> Result<MultiValue<'lua>> {
Ok(MultiValue::new())
}
}
/// Wrapper for arguments that allowed to fail during conversion.
///
/// Note that failing includes recieving a `nil` value if the type isn't
/// convertible from `nil`. That is, this wrapper allows _skipping_ arguments in
/// called functions. Capturing nil for skippable arguments can be done through
/// `Fallible<Option<T>>` as it will have a value of `Some(None)` when
/// converting from `nil`.
///
/// In case where `nil` argument is expected (must be specified from the
/// script), `Option` should be used instead of `Fallible`.
///
/// If conversion is successful, the value will be `Some(T)`.
///
/// Conversely, if conversion fails, the value will be `None`, and `consumed`
/// argument counter will stay unchanged.
pub struct Fallible<T>(Option<T>);

impl<T> Fallible<T> {
/// Returns inner `Option<T>`.
pub fn into_option(self) -> Option<T> {
self.0
}

/// Maps fallible type using provided `mapping` function into another
/// `Option`.
pub fn map<R, F: Fn(T) -> R>(self, mapping: F) -> Option<R> {
self.0.map(mapping)
}

/// Unwraps fallible type or panics if conversion failed.
pub fn unwrap(self) -> T {
self.0.unwrap()
}
/// Unwraps fallible type or returns `value` if conversion failed.
pub fn unwrap_or(self, value: T) -> T {
self.0.unwrap_or(value)
}
/// Unwraps fallible type or returns a return value of `init` if conversion
/// failed.
pub fn unwrap_or_else<F: Fn() -> T>(self, f: F) -> T {
self.0.unwrap_or_else(f)
}
/// Unwraps fallible type or returns the default value if conversion failed.
pub fn unwrap_or_default(self) -> T
where
T: Default,
{
self.0.unwrap_or_else(T::default)
}
/// Retuns `other` `Option` if this argument conversion failed.
pub fn or(self, other: Option<T>) -> Option<T> {
self.0.or(other)
}
/// Retuns `Option` value returned by `f` if this argument conversion
/// failed.
pub fn or_else<F: Fn() -> Option<T>>(self, f: F) -> Option<T> {
self.0.or_else(f)
}
}

impl<'lua> FromLuaMulti<'lua> for () {
fn from_lua_multi(_: MultiValue, _: Context<'lua>) -> Result<Self> {
Ok(())
impl<T> Deref for Fallible<T> {
type Target = Option<T>;
fn deref(&self) -> &Self::Target {
&self.0
}
}
impl<T> DerefMut for Fallible<T> {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.0
}
}

impl<'lua, T: FromLuaMulti<'lua>> FromLuaMulti<'lua> for Fallible<T> {
fn from_counted_multi(
values: MultiValue<'lua>,
lua: Context<'lua>,
consumed: &mut usize,
) -> Result<Self> {
match T::from_counted_multi(values, lua, consumed) {
Ok(it) => {
*consumed += 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this increment for? I would expect the T::from_counted_multi() would already have done this.

Copy link
Author

Choose a reason for hiding this comment

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

Right, that's an error. Didn't test this latest version as much.

Ok(Fallible(Some(it)))
}
Err(_) => Ok(Fallible(None)),
}
);
}
}

($last:ident $($name:ident)*) => (
impl<'lua, $($name,)* $last> ToLuaMulti<'lua> for ($($name,)* $last,)
where $($name: ToLua<'lua>,)*
$last: ToLuaMulti<'lua>
macro_rules! impl_tuple {
($($name:ident)*) => (
impl<'lua, $($name),*> ToLuaMulti<'lua> for ($($name,)*)
where $($name: ToLuaMulti<'lua>),*
{
#[allow(unused_variables)]
#[allow(unused_mut)]
#[allow(non_snake_case)]
fn to_lua_multi(self, lua: Context<'lua>) -> Result<MultiValue<'lua>> {
let ($($name,)* $last,) = self;
let ($($name,)*) = self;

let mut results = $last.to_lua_multi(lua)?;
push_reverse!(results, $($name.to_lua(lua)?,)*);
let mut results = MultiValue::new();
push_reverse!(results, $($name.to_lua_multi(lua)?,)*);
Ok(results)
}
}

impl<'lua, $($name,)* $last> FromLuaMulti<'lua> for ($($name,)* $last,)
where $($name: FromLua<'lua>,)*
$last: FromLuaMulti<'lua>
impl<'lua, $($name),*> FromLuaMulti<'lua> for ($($name,)*)
where $($name: FromLuaMulti<'lua>),*
{
#[allow(unused_variables)]
#[allow(unused_mut)]
#[allow(non_snake_case)]
fn from_lua_multi(mut values: MultiValue<'lua>, lua: Context<'lua>) -> Result<Self> {
$(let $name = values.pop_front().unwrap_or(Nil);)*
let $last = FromLuaMulti::from_lua_multi(values, lua)?;
Ok(($(FromLua::from_lua($name, lua)?,)* $last,))
fn from_counted_multi(mut values: MultiValue<'lua>, lua: Context<'lua>, total: &mut usize) -> Result<Self> {
$(
let $name = {
let mut consumed = 0;
let it = match FromLuaMulti::from_counted_multi(values.clone(), lua, &mut consumed) {
Ok(it) => it,
Err(err) => {
return Err(err);
}
};
*total += consumed;
values.drop_front(consumed);
it
};
)*
Ok(($($name,)*))
}
}
);
Expand All @@ -187,11 +304,11 @@ macro_rules! impl_tuple {
macro_rules! push_reverse {
($multi_value:expr, $first:expr, $($rest:expr,)*) => (
push_reverse!($multi_value, $($rest,)*);
$multi_value.push_front($first);
$multi_value.append($first);
);

($multi_value:expr, $first:expr) => (
$multi_value.push_front($first);
$multi_value.append($first);
);

($multi_value:expr,) => ();
Expand Down
14 changes: 7 additions & 7 deletions src/prelude.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@ pub use crate::{
AnyUserData as LuaAnyUserData, Chunk as LuaChunk, Context as LuaContext, Debug as LuaDebug,
DebugNames as LuaDebugNames, DebugSource as LuaDebugSource, DebugStack as LuaDebugStack,
Error as LuaError, ExternalError as LuaExternalError, ExternalResult as LuaExternalResult,
FromLua, FromLuaMulti, Function as LuaFunction, HookTriggers as LuaHookTriggers,
Integer as LuaInteger, LightUserData as LuaLightUserData, Lua, MetaMethod as LuaMetaMethod,
MultiValue as LuaMultiValue, Nil as LuaNil, Number as LuaNumber, RegistryKey as LuaRegistryKey,
Result as LuaResult, Scope as LuaScope, String as LuaString, Table as LuaTable,
TablePairs as LuaTablePairs, TableSequence as LuaTableSequence, Thread as LuaThread,
ThreadStatus as LuaThreadStatus, ToLua, ToLuaMulti, UserData as LuaUserData,
UserDataMethods as LuaUserDataMethods, Value as LuaValue,
Fallible as LuaFallible, FromLua, FromLuaMulti, Function as LuaFunction,
HookTriggers as LuaHookTriggers, Integer as LuaInteger, LightUserData as LuaLightUserData, Lua,
MetaMethod as LuaMetaMethod, MultiValue as LuaMultiValue, Nil as LuaNil, Number as LuaNumber,
RegistryKey as LuaRegistryKey, Result as LuaResult, Scope as LuaScope, String as LuaString,
Table as LuaTable, TablePairs as LuaTablePairs, TableSequence as LuaTableSequence,
Thread as LuaThread, ThreadStatus as LuaThreadStatus, ToLua, ToLuaMulti,
UserData as LuaUserData, UserDataMethods as LuaUserDataMethods, Value as LuaValue,
};
Loading