Skip to content

Get rid of where Self: std::marker::Sized #45

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
joepio opened this issue Nov 23, 2020 · 8 comments
Closed

Get rid of where Self: std::marker::Sized #45

joepio opened this issue Nov 23, 2020 · 8 comments
Labels
help wanted Extra attention is needed

Comments

@joepio
Copy link
Member

joepio commented Nov 23, 2020

I'm still learning Rust, and that means that I'm still not fully understanding traits and object safety.

We have a Store struct based on the Storelike trait, which contains all the data. We also have Resource, which have a reference to a Storelike.

pub struct Resource<'a> {
    propvals: PropVals,
    subject: String,
    classes: Option<Vec<Class>>,
    store: &'a dyn Storelike,
    commit: CommitBuilder,
}

This enables a more convenient API, so we can have:

resource.set_prop("prop", "val")

instead of requiring the store in every single call:

resource.set_prop(store, "prop", "val")

Since our Store is a trait, it's size is not known at compile time. From what I understand, this means that in methods on the Storelike trait where we create Resources, we need to explicitly state that the Store (Self) is Sized:

fn create_agent(&self, name: &str) -> AtomicResult<(String, String)>
    where
        Self: std::marker::Sized,
    {
        let mut agent = Resource::new_instance(urls::AGENT, self)?;
        Ok(())
    }

This where clause is now needed in all functions that create resources, and I feel like that is wrong, although I can't say for sure that it is.

I thought it might make sense to require Sized in the Storelike trait, like this: pub trait Storelike: Sized, and although that removes the need for the where clauses, it introduces another problem. In every signature where a reference to the store is made:

impl Collection {
    /// Constructs a Collection, which is a paginated list of items with some sorting applied.
    pub fn new(
        store: &dyn Storelike,
        collection_builder: crate::collections::CollectionBuilder,
    )

...this error appears:

the trait `storelike::Storelike` cannot be made into an object
the trait `storelike::Storelike` cannot be made into an objectrustc(E0038)
storelike.rs(47, 22): ...because it requires `Self: Sized`
joepio added a commit that referenced this issue Nov 23, 2020
@joepio
Copy link
Member Author

joepio commented Nov 23, 2020

Also, with the current approach, I can't call self.store.commit in a Resource method (e.g. resource.save()):

the `commit` method cannot be invoked on a trait objectrustc
storelike.rs(76, 15): this has a `Sized` requirement

@joepio
Copy link
Member Author

joepio commented Nov 23, 2020

I've posted this question to StackOverlfow. https://stackoverflow.com/questions/64975080/invoking-a-method-on-a-trait-object

@joepio joepio added the help wanted Extra attention is needed label Nov 29, 2020
joepio added a commit that referenced this issue Dec 20, 2020
joepio added a commit that referenced this issue Dec 25, 2020
@joepio
Copy link
Member Author

joepio commented Dec 27, 2020

This issue is just the worst... It keeps coming back in all kinds of situations - the sized constraints on the many Storelike methods severely limit where I can call them.

I see no other option than to drastically change the Resource API, and remove the reference to Storelike from Resource. This means that most of the Resource methods will now explicitly require.

@joepio
Copy link
Member Author

joepio commented Dec 31, 2020

I've tried changing the Resource API, remove the Store reference, but that didn't fix all instances of this issue. The issue happens whenever I call a method that uses Storelike as an argument.

For example, when creating a new Collection::new, Storelike.tpf() is called. This means that Collection::new needs to be compiled for different sizes of Storelike structs. And for this to work, Storelike needs to be sized.

With the refactor I suggested above, I explicitly pass Store to many Resource methods. The result, unfortunately, is that now all the Storelike methods that call Resource methods that use Storelike (self) need to be sized.... E.g. resource.get_shortname(&item, self) is called in Storelike.get_path, which means that now get_path needs a where self is Sized thing.

joepio added a commit that referenced this issue Dec 31, 2020
@joepio
Copy link
Member Author

joepio commented Dec 31, 2020

Just tried adding :Sized to the trait, and replacing all dyn Storelike with impl Storelike... And so far, it seems to work!

@joepio
Copy link
Member Author

joepio commented Dec 31, 2020

However, when I try to do this while keeping a reference to Storelike in Resource, things fall apart again:

pub struct Resource<'a> {
    propvals: PropVals,
    subject: String,
    classes: Option<Vec<Class>>,
    // `impl Trait` not allowed outside of function and inherent method return types rustcE0562
    store: &'a impl Storelike,
    commit: CommitBuilder,
}

One possible solution to this, was to use Generics. Just tried that, but I also fail to get that working.

@joepio
Copy link
Member Author

joepio commented Jan 1, 2021

I kind of fixed it, but it still doesn't feel right. I set a :Sized bound for the entire Storelike trait, which also means that I can't keep a reference to it in other structs, such as Resource. This caused me to refactor quite a bit, and the API got a little worse because of it. Adjusting a resource (e.g. setting a property) now requires explicitly passing the store. Some might prefer it, though, and it does enable multi-store workflows / passing resources from store to store.

Anyway - I can now do all the things i needed to do. Use impl Storelike instead of dyn Storelike if you want to stay out of trouble. Closing.

@joepio joepio closed this as completed Jan 1, 2021
@joepio
Copy link
Member Author

joepio commented Mar 7, 2021

Well, it turns out setting :Sized also comes with some serious limitations. I'm working on a plugin #73 system, which includes structs that contain functions that refer to Storelike. And that's a problem, because Storelike is sized, and that means I can't use &impl Storelike in that function.

I think there are two solutions to this:

  • Get rid of one of the implementations (which will be the in-memory one)
  • Move all the error-causing logic from the trait. Might involve some code duplication, but I get to keep both implementations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

1 participant