self_cell rework

[Voultapher]

This is a ground up rewrite that addresses all outstanding issues:

• Switch to eager only version, rename to self_cell (lazy is opt in via custom dependent type)
• Add fallible constructor support
• New API that avoids the reported soundness problems
• Extensive documentation
• Advanced examples
• More tests
• Go from 1 dependency to 0

I will merge this in the absence of feedback, one week from now, Tue 20.04.2021.

[zbraniecki]

Thanks! I tried using the new branch for Fluent work in projectfluent/fluent-rs#221

I really like the changes and the API looks very nice!

Here are several items of feedback:

• The lack of ability to set pub status means I have to add pub struct FluentResource(InnerFluentResource) just to expose FluentResource.
• Need to import TryInto into the scope, could be in the macro.
• It would be nice to be able to pass ast::Resource rather than having to define type Resource = ast::Resource to be able to pass it to the macro

And one blocker:

• The casting of errors through TryFrom is unfortunate for my use case. I have TryFrom<&String> for ast::Resource which returns Error = (Self, Vec<ParserError>); - but when I then call it on InnerFluentResource::try_from it returns Result<InnerFluentResource, (ast::Resource, Vec<ParserError>)> which I cannot manually wrap in InnerFluentResource anymore so I can't have FluentResource::try_new(input: &str) -> Result<Self, (Self, Vec<ParserError>)>

Even if the pub issue was fixed and I was able to skip the InnerFluentResource abstraction, I'd still need my try_new be able to return Result<Self, (Self, Vec<Error>)> which means I can't just pass-through the error from TryFrom.

I'm not sure how to resolve the blocker, and I recognize that my use case scenario is esoteric, but it definitely is a blocker :(

Let me know if the feedback is helpful and if there's something else I can do to help brainstorm that!

[Voultapher]

Thanks for the great feedback. I'm working on the various points, a questions I have:

Need to import TryInto into the scope, could be in the macro.

How could I do that? A simple use core::convert::TryInto would not work, because I want to support multiple self_cell invocations in the same module, and I can only import it once.

[Voultapher]

Regarding the blocker, one I idea:

Owner: String
Dependent: (Resource, Option<Vec<crate::parser::ParserError>>)

Then use the regular from variant and check if the option is some and return that in the try_new. Could that work?

[Voultapher]

It would be nice to be able to pass ast::Resource rather than having to define type Resource = ast::Resource to be able to pass it to the macro

I tried a couple approaches, and while I found functioning ones, I found none working on stable. Again the no new identifier problem. https://doc.rust-lang.org/std/macro.concat_idents.html would open up a lot of doors.

[Voultapher]

The lack of ability to set pub status means I have to add pub struct FluentResource(InnerFluentResource) just to expose FluentResource.

This has been addressed with 1880cbe

[Voultapher]

@zbraniecki friendly ping. I'd like to resolve your blocker with you before merging this.

[zbraniecki]

Hi, sorry! Will look into this tomorrow!

[zbraniecki]

How could I do that? A simple use core::convert::TryInto would not work, because I want to support multiple self_cell invocations in the same module, and I can only import it once.

Could you use the whole path when invoking and not import?

I tried a couple approaches, and while I found functioning ones, I found none working on stable. Again the no new identifier problem. https://doc.rust-lang.org/std/macro.concat_idents.html would open up a lot of doors.

Wouldn't expr or type work here?

Owner: String
Dependent: (Resource, Option<Veccrate::parser::ParserError>)

Unfortunately, I can't implement From or Into for (Resource, Option<Vec<crate::parser::ParserError>>) because of blanket impls:

error[E0119]: conflicting implementations of trait `std::convert::Into<(ast::Resource<&str>, std::vec::Vec<parser::errors::ParserError>)>` for type `&std::string::String`:
   --> fluent-syntax/src/ast/mod.rs:120:1
    |
120 | impl<'s> Into<(Resource<&'s str>, Vec<crate::parser::ParserError>)> for &'s String {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: conflicting implementation in crate `core`:
            - impl<T, U> Into<U> for T
              where U: From<T>;
    = note: upstream crates may add a new impl of trait `std::convert::From<&std::string::String>` for type `(ast::Resource<&str>, std::vec::Vec<parser::errors::ParserError>)` in future versions

error[E0117]: only traits defined in the current crate can be implemented for arbitrary types
   --> fluent-syntax/src/ast/mod.rs:120:1
    |
120 | impl<'s> Into<(Resource<&'s str>, Vec<crate::parser::ParserError>)> for &'s String {
    | ^^^^^^^^^----------------------------------------------------------^^^^^----------
    | |        |                                                              |
    | |        |                                                              `String` is not defined in the current crate
    | |        this is not defined in the current crate because tuples are always foreign
    | impl doesn't use only types from inside the current crate
    |
    = note: define and implement a trait or new type instead

error: aborting due to 2 previous errors

Similar problem for From.

So, I'm still stuck with how to wrangle errors through fallible lazy constructor.

I also realized that getting rid of InnerFluentResource (which I can now do!) exposes the methods from self_cell into the public API so I may want to preserve the wrapper to keep the public API clean.

[Voultapher]

Could you use the whole path when invoking and not import?

Unfortunately that's not enough, https://github.com/Voultapher/once_self_cell/pull/7/files#diff-b1a35a68f14e696205874893c07fd24fdb88882b47c23cc0e0c80a30c7d53759R186 I'm already using the whole path for the type definition. But TIL there is a way to call the trait function without importing it https://stackoverflow.com/questions/25273816/why-do-i-need-to-import-a-trait-to-use-the-methods-it-defines-for-a-type

Just pushed a fix, that also includes some visibility fixes.

[Voultapher]

Let me see if I can figure out a solution to your blocker. I'll try to build a small POC.

[Voultapher]

@zbraniecki here is a POC, it's not ideal because you have to clone the errors, but maybe that isn't a big problem for you.

use self_cell::self_cell;

#[derive(Debug, Eq, PartialEq)]
struct Resource<'a>(pub Vec<&'a str>);

fn parse_runtime<'a>(source: &'a String) -> Result<Resource<'a>, (Resource<'a>, Vec<ParseError>)> {
    if source.contains("bad") {
        Err((Resource(vec![]), vec![ParseError::Dummy]))
    } else {
        Ok(Resource(
            source.split(' ').filter(|word| word.len() > 1).collect(),
        ))
    }
}

#[derive(Clone, Debug)]
enum ParseError {
    Dummy,
}

#[derive(Debug)]
struct ResourceWithErr<'a>((Resource<'a>, Option<Vec<ParseError>>));

impl<'a> From<&'a String> for ResourceWithErr<'a> {
    fn from(source: &'a String) -> Self {
        match parse_runtime(source) {
            Ok(resource) => Self((resource, None)),
            Err((resource, errors)) => Self((resource, Some(errors))),
        }
    }
}

self_cell!(
    struct FluentResource {
        #[from]
        owner: String,

        #[covariant]
        dependent: ResourceWithErr,
    }

    impl {Debug}
);

impl FluentResource {
    fn try_new(source: String) -> Result<Self, (Self, Vec<ParseError>)> {
        let cell = Self::new(source);

        if let Some(errors) = cell.borrow_dependent().0 .1.as_ref() {
            let cloned_errors = errors.clone();
            Err((cell, cloned_errors))
        } else {
            Ok(cell)
        }
    }
}

fn main() {
    dbg!(FluentResource::try_new("fox = cat + dog".into()).unwrap());
    dbg!(FluentResource::try_new("fox = bad_dog".into()).unwrap_err());
}

In general the try_new interface you build is quite awkward, something like ResourceWithErr seems like a more idiomatic way to say, you will always get a FluentResource but there is optionally some errors that accompany it. Together with helper functions for ResourceWithErr you might be able to change the try_new interface. But of course that's up to you. The solution I provided should be functional, the layout inefficiency needs to benchmarked to see if it significantly affects your use cases, as does the error clone.

Let me know if this works for you.

[zbraniecki]

In general the try_new interface you build is quite awkward, something like ResourceWithErr seems like a more idiomatic way to say

We spent quite a bit collecting feedback on various pros and cons - projectfluent/fluent-rs#219 - it's not trivial what the right approach should be and I expect to continue iterating on it as the whole ecosystem matures and patterns emerge.

What I'm concerned about is that effectively your library locks that by making it opaque.

Let me know if this works for you.

I don't think it does, and I don't think its necessarily a bad thing. The value your crate provides is not limited to a single use case (even one that inspired it), and I think at this point trying to find a balance between what Fluent needs and what you offer becomes a limitation to your ability to release and get external testing and validation.

I'd suggest release what you have and keep a bug open on future possibilities of aligning with Fluent needs. Some of the feedback you'll get, and some of the feedback Fluent will get, may get us more aligned and we should keep an eye on that, but I would suggest unblocking the next release of once_self_cell atm.

[Voultapher]

I don't think it does,

I see where you are coming from. Could you at least explain why.

[zbraniecki]

I see where you are coming from. Could you at least explain why.

I have a system that works both for my testers and in production at scale.
The core value for me of switching to once_self_cell is a chance to limit the dependency chain, which is important for me, but has to be balanced against other important values of the system.

One API decision (way to return success with failures) Fluent made seems to be causing strong design challenges for against the design decisions you made and "just don't fit".

Since both Fluent API and your API are still in flux, I'm leaning toward recognizing that outcome of our exploration here and moving on to avoid preventing you from releasing the next revision of your crate.

Releasing it is important for once_self_cell to get further feedback from other potential testers, while remaining in PR state while trying to create suboptimal ways to mitigate the incompatibility feels like the wrong tradeoff.

I'd prefer to continue monitoring for potential future alignment as the API of once_self_cell evolves and the API of Fluent evolves.

[Voultapher]

Thanks for the explanation. The situation might not be as stuck as it appears to you. Some further thinking made the situation more clear to me. You have a constructor for your dependent that doesn't only produce the lifetime dependent value, but also some other unrelated value. It shouldn't be too hard to create a third constructor type that covers this use case, it shouldn't bloat the API too much either. Why I'm keen on finding a solution here is not entirely selfless. Having a semi-popular crate successfully transition, would both signal to me that this crate actually is a valid competitor to the ones it tries to be an alternative for, and it could help others have more confidence.

[zbraniecki]

Why I'm keen on finding a solution here is not entirely selfless. Having a semi-popular crate successfully transition, would both signal to me that this crate actually is a valid competitor to the ones it tries to be an alternative for, and it could help others have more confidence.

I agree with that goal! I merely suggest that we don't need to achieve it in this pre-release :)

[Voultapher]

How about this, let's timebox this PR to max Sunday 25.04.2021. In that meantime we'll try to find a solution, if not, all good and we'll continue this as an issue.

[Voultapher]

@zbraniecki this latest commit introduced a new constructor type that should cover your use case.

[Voultapher]

In line with our agreed timebox I'm closing this PR. Please let me know if this works for you, also in terms of performance. If not we'll continue your topic in a separate issue.

[zbraniecki]

Please let me know if this works for you, also in terms of performance. If not we'll continue your topic in a separate issue.

I tried to apply the from_fn constructor, but I'm not sure how it is meant to cover my case since it's infallible. Can you nudge me in the right direction?

[Voultapher]

Unless I'm missing something shouldn't the same approach used before https://github.com/projectfluent/fluent-rs/pull/221/files#diff-73bb1d907649cc571c753052726efc674e5b561ed29e48d8d7e46d55ba3f8030L77 work here too. errors is not dependent on owner and can be mutated inside the closure.

[Voultapher]

@zbraniecki friendly ping, it be great to get this resolved. If this latest suggestion of mine doesn't work out, please open a new issue. Discussion in a closed PR isn't ideal.