Swap From<Cow<'static, str>> with From<Cow<'_, str>> for IString

[kirillsemyonkin]

Same as #67.

Reason being OsStr::to_string_lossy returns Cow<'_, str> for whatever reason, preventing .into() the very same way having only From<&'static str> did - with the confusing errors that point to wrong places and do not describe anything much useful.

[cecton]

I'm not sure about it but I think Cow static is the most common case and the other one only an exception. Maybe we could treat it as such and create from_osstr_lossy instead that would take an osstr in argument? I don't know, I need to double check if that makes any logical sense but I'm not at home at the moment

If we go the other way around we should probably also add Into-Cow because I read lot of APIs allow using that type as parameter.

[kirillsemyonkin]

we should probably also add Into-Cow

IString::as_cow exists. Its bound is not 'static, it is '_ (same lifetime as &self because it is bound by the Rc variant). Not sure when Cow would be static often (its purpose is to have Borrowed variant, and if its not used I would just use the underlying ToOwned, and if there are places where 'static can be made, it is either const or thread_local).

create from_osstr_lossy instead

I can also just do osstr.to_string_lossy().to_string().into() instead for a quick solve just like in #67. Yes, Rc::from(osstr.to_string_lossy()).into() is more optimal, but I am not incentivized to do it (wrapping something is not as easy; maybe .to_rc() trait is desired, also could be osstr.to_string_lossy().to_istring() with #57). The actual problem is with Rust's errors. I once again sat for 5 minutes wondering what is wrong with my code, at the wrong spot Rust pointed me to, only to then remember that there might be a 'static in the From.

I'm not sure about it but I think Cow static is the most common case and the other one only an exception.

No clue, never used it. Having owned variant being bound by some reference lifetime sounds really annoying, it is like the worst of both refs and owneds worlds. In std I only see it being used for to_string_lossy, for whatever reason when every other method gives String (for example, str::replace when no changes to string have occurred could totally return Cow::Borrowed instead, but it does not).

[cecton]

I'm not sure about it but I think Cow static is the most common case and the other one only an exception.

No clue, never used it. Having owned variant being bound by some reference lifetime sounds really annoying, it is like the worst of both refs and owneds worlds. In std I only see it being used for to_string_lossy, for whatever reason when every other method gives String (for example, str::replace when no changes to string have occurred could totally return Cow::Borrowed instead, but it does not).

They use the Cow ref in regex apparently: https://docs.rs/regex/latest/regex/struct.Regex.html#method.replace

The actual problem is with Rust's errors. I once again sat for 5 minutes wondering what is wrong with my code, at the wrong spot Rust pointed me to, only to then remember that there might be a 'static in the From.

Yeah I know... I'm just less keen with Cow because it seemed like the right case where we should prefer the static one and not the ref one. After all, the whole point of IString is to be a fancier version of the original AttrValue made by @ranile which in turn was a fancy version of Cow static str as it was originally used in Yew.

we should probably also add Into-Cow

IString::as_cow exists. Its bound is not 'static, it is '_ (same lifetime as &self because it is bound by the Rc variant). Not sure when Cow would be static often (its purpose is to have Borrowed variant, and if its not used I would just use the underlying ToOwned, and if there are places where 'static can be made, it is either const or thread_local).

I'm surprised by this to be honest. It was done as part of #10. I think @ranile will have more feedback on this.

[cecton]

In doubt I'm not blocking it if you think it's the right way

[ranile]

After all, the whole point of IString is to be a fancier version of the original AttrValue made by @ranile which in turn was a fancy version of Cow static str as it was originally used in Yew.

It's been too long for me to remember the details. With Yew, 'static made sense because it's either a literal or owned (Rc<str>). It makes sense to use '_ instead of 'static as long as rustc is able infer the lifetime in Yew uses correctly, imo.

[kirillsemyonkin]

It makes sense to use '_ instead of 'static as long as rustc is able infer the lifetime in Yew uses correctly, imo.

To put you into the context, we have a dilemma:

• Either make From<Cow<'static, str>>, where it is able to make an IString::Static, but it will be horrible when you try to use cow.into() because it will be pointing to cow's declaration, for whatever reason, and you will have a terrible time trying to figure out why that is;
• Or make From<Cow<'_, str>>, where it is unable to optimize and will always clone the string data to make a IString::Rc, but cow.into() will not only have no confusing and annoying errors, but also work for any Cow<str>.

Your comment about "being able to infer the lifetime in Yew uses correctly", honestly I did not understand what exactly you mean lol

[ranile]

Thanks for the context! I would prefer the 2nd option but with a helper function to add back the previous implementation with the optimization. My thought process is: API that is not optimized in one case is better than one that fails with a horrible error message. Achieving that without losing functionality is best.

Worth mentioning, I don't really have a strong opinion either way. I'll leave the final decision up to you and @cecton

it will be horrible when you try to use cow.into() because it will be pointing to cow's declaration, for whatever reason, and you will have a terrible time trying to figure out why that is;

That sounds like something to raise with rustc folks. It's good to have this addressed (or at least documented, even if it's in the GH issues) upstream

Your comment about "being able to infer the lifetime in Yew uses correctly", honestly I did not understand what exactly you mean lol

That was more of "if this change doesn't break build in Yew (because of the heavy dependency and assumption of 'static), it's all good". There's probably some into there (or downstream) that might get affected by it, unless I'm grossly misremembering (which is highly possible)

[kirillsemyonkin]

That was more of "if this change doesn't break build in Yew (because of the heavy dependency and assumption of 'static), it's all good". There's probably some into there (or downstream) that might get affected by it, unless I'm grossly misremembering (which is highly possible)

No, I believe this PR is an API expansion - not a breaking change, only allowing more Cows to be .into()ed (unless there are edge-cases I am not aware of).

a helper function to add back the previous implementation with the optimization

Not sure if there is already some way to have old behavior, which looked like this:

match cow {
    Cow::Borrowed(static_str) => IString::Static(static_str),
    Cow::Owned(owned_string) => owned_string.into(),
}

I also discussed other way around with #57, where you do not add a method for optimized case, but rather add method for unoptimized "always clone" case, leaving .into() as broken (erroring incorrectly).

[cecton]

Or make From<Cow<'_, str>>, where it is unable to optimize and will always clone the string data to make a IString::Rc, but cow.into() will not only have no confusing and annoying errors, but also work for any Cow<str>.

I read lot of APIs use impl Into<Cow<'static, str>>, that's why I think it's more useful. Maybe we can implement the Into for it and keep the From on Cow '_ str at the same time? It's only the From static and From '_ that can't be implemented together after all, right?

[kirillsemyonkin]

Maybe we can implement the Into for it and keep the From on Cow '_ str at the same time?

There is automatic impl<T, U> Into<U> for T where U: From<T> in std that would conflict with it

[cecton]

I just tested to be sure but it seems it compiles:

impl From<Cow<'_, str>> for IString {
    fn from(cow: Cow<'_, str>) -> Self {
        match cow {
            Cow::Borrowed(s) => s.into(),
            Cow::Owned(s) => s.into(),
        }
    }
}

impl Into<Cow<'static, str>> for IString {
    fn into(self) -> Cow<'static, str> { todo!() }
}

[kirillsemyonkin]

impl Into<Cow<'static, str>> for IString

I thought it was other way around, i.e. impl Into<IString> for Cow<'static, str>. Well, either of these work for this purpose (regardless of PR's impl From<Cow<'_, str>> for IString):

impl Into<Cow<'static, str>> for IString {
    fn into(self) -> Cow<'static, str> {
        match self {
            IString::Static(s) => Cow::Borrowed(s),
            IString::Rc(s) => Cow::Owned(s.to_string()),
        }
    }
}

impl From<IString> for Cow<'static, str> {
    fn from(s: IString) -> Cow<'static, str> {
        match s {
            IString::Static(s) => Cow::Borrowed(s),
            IString::Rc(s) => Cow::Owned(s.to_string()),
        }
    }
}

This would be about having a conversion path into Cow<'static, str>, as opposed to only having fn as_cow(&self) -> Cow<'_, str>. However, this PR is not about this.