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.

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

Generic abstract type support #93

Open
Caellian opened this issue Oct 14, 2024 · 3 comments · May be fixed by #96
Open

Generic abstract type support #93

Caellian opened this issue Oct 14, 2024 · 3 comments · May be fixed by #96

Comments

@Caellian
Copy link
Contributor

There's no way of passing partially visible types through cel expressions.

This means that any functions that use This require that value to be From<Value> for any type they operate on. This is doable in a lot of cases, but negatively impacts performance. In some cases it's impossible because internal API guarantees can't be (safely) upheld.

Cel spec provides abstract types for this kind of use.

Use Case

I have some User type on Rust size, which has a bunch of Option, MaybeUninit, etc. values that get populated through use (cache, temporary state, etc.).

I want to provide a get_role function which performs expensive computation. The Rust implementation can make use of private cached values stored in User. Cel functions are forced to reconstruct User partially from data that's visible to end-user, and then recompute all potential intermediate and final information to provide the result once.

Solution

TryIntoValue provides a type projection from internal types to Cel types. This issue arises when the library discards the source type information in favor of operating with Cel types (i.e. Value).

The solution is to retain source type in form of

ExternalValue {
  r#type: TypeId,
  value: Arc<dyn TryIntoValue>,
}

enum Value {
  // ... other ...
  External(ExternalValue),
}

, so that those values can be passed around and accessed from Handler<T>. This is assuming TryIntoValue: Any + Send + Sync for simplicity, but another trait with stricter requirements should be introduced and auto-implemented for all T: TryIntoValue + Any + Send + Sync.

FromContext::from_context needs to compare value TypeId for those Values and error with UnsupportedTargetType or NotSupportedAsMethod appropriately.

By providing:

impl ExternalValue {
    fn as_ref<T>(&self) -> Result<&T, ExecutionError>
    where
        T: TryIntoValue
    {
        if self.r#type == std::any::TypeId::of::<T>() {
            self.value.downcast_ref().ok_or(/* error below */)
        } else {
            Err(ExecutionError::IncorrectExternal { /* ... expected, actual ... */ })
        }
    }
    // ... and similarly for as_mut ...
}

users will be able to do:

fn cel_role(
    ftx: &FunctionContext,
    This(this): This<Value>,
) -> Result<Role, ExecutionError> {
    if let Value::External(v) = this {
        let user = v.as_ref();
        Ok(user.get_cached_or_compute_rule())
    } else {
        // error ...
    }
}

Issues

  • User sees that some value is Value::Map and assumes they can construct an identical value in place of internally-typed one, which fails. This suggests there needs to be a new error type returned when Value::* is provided in place of Value::External.

Examples in the wild

  • mlua - UserData
    • lua has a special opaque type for this kind of thing that's internally a void*, in cel this would be an abstract type
@clarkmcc
Copy link
Owner

Generally, I like the approach, thank you for the thorough write-up with examples and a detailed explanation of the use-case.

The solution is to retain source type in form of ExternalValue

Is the idea that someone would construct an external value explicitly and pass it to the context in order to use this (see example below)? Automatically serializable types would continue to operate as they do today?

context.add_variable_from_value("user", Value::ExternalValue(&user)) // or something

It seems like this problem could also be solved by the trait-based design in #76, do you agree

@Caellian
Copy link
Contributor Author

After some experimenting with vendored version of the library. ExternalType looks like:

#[derive(Debug, Clone)]
pub struct ExternalValue(Arc<dyn ExternalType>);

impl ExternalValue {
    pub fn downcast<T>(&self) -> Option<Arc<T>>
    where
        T: 'static + Send + Sync,
    {
        Arc::downcast::<T>(self.0.clone()).ok()
    }
}

That's blocked by rust-lang/rust#119335 for now. A more complicated version can be created that partially copies Arc functionality which I'll do locally, but I suggest waiting on the issue to close before implementing this as the resulting code will be much simpler.

@Caellian
Copy link
Contributor Author

Is the idea that someone would construct an external value explicitly and pass it to the context in order to use this (see example below)? Automatically serializable types would continue to operate as they do today?

Ideally, if the type implements ExternalType, cel should use details provided by ExternalType trait instead of serializing it as that's a destructive operation, but that would require specialization. For now, a distinction can be made with different method names like you've suggested (context.add_variable_from_value).

It seems like this problem could also be solved by the trait-based design in #76, do you agree

Yes and no. This issue basically attacks the same problem from a different angle. I personally prefer Value being an enum because it's cheaper to use for discrete values (numbers/strings), it's also what I'm used to coming from mlua .

If Value is a trait, then whatever mechanism this suggestion would use needs to move up a level so that Value can be stored and used - and this issue can be closed because ExternalTypeValue.

@Caellian Caellian linked a pull request Oct 15, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants