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

string: add impl From<&IString> for IString #37

Merged
merged 2 commits into from
Oct 28, 2023
Merged

Conversation

cecton
Copy link
Member

@cecton cecton commented Oct 28, 2023

I just notice that this is possible with String but not with IString:

let x = String::from("foo");
let y = String::from(&x);

let x = IString::Static("foo");
let y = IString::from(&x);

I get this issue while using Yew that could be fixed that way:

error[E0277]: the trait bound `implicit_clone::unsync::IString: From<&implicit_clone::unsync::IString>` is not satisfied
   --> packages/yew/src/html/conversion/into_prop_value.rs:298:28
    |
298 |                 VText::new(self).into()
    |                 ---------- ^^^^ the trait `From<&implicit_clone::unsync::IString>` is not implemented for `implicit_clone::unsync::IString`
    |                 |
    |                 required by a bound introduced by this call

@cecton cecton mentioned this pull request Oct 28, 2023
2 tasks
@kirillsemyonkin
Copy link
Collaborator

kirillsemyonkin commented Oct 28, 2023

Do other ImplicitClone types suffer from this? Would impl<T: ImplicitClone> From<&T> for T be possible?

@cecton
Copy link
Member Author

cecton commented Oct 28, 2023

@kirillsemyonkin it doesn't seem so:

error[E0210]: type parameter `T` must be used as the type parameter for some local type (e.g., `MyStruct<T>`)
  --> src/unsync.rs:20:6
   |
20 | impl<T: ImplicitClone> From<&T> for T {}
   |      ^ type parameter `T` must be used as the type parameter for some local type
   |
   = note: implementing a foreign trait is only possible if at least one of the types for which it is implemented is local
   = note: only traits defined in the current crate can be implemented for a type parameter

@kirillsemyonkin
Copy link
Collaborator

@cecton What about other types (IMap, IArray)?

@cecton
Copy link
Member Author

cecton commented Oct 28, 2023

Vec doesn't have this feature. I checked already. (I didn't bother checking HashMap but I assume it's the same).

@kirillsemyonkin
Copy link
Collaborator

@cecton Weird that they do not. If they are worried about allocations, they would probably ban impl From<&String> for String too. If they believe cloning String is cheap in that case (probably since u8 is Copy), then surely impl<T> From<&Rc<T>> for Rc<T> should be cheap, but I do not see such impl (and for some reason there is impl<T: Clone> From<&[T]> for Rc<[T]> (not T: Copy)... Weren't they worried about allocations and cloning in my previous sentence?). I believe standard library is just incomplete and it shouldn't be a good reason to block the idea because of that, especially since our allocations are supposed to be much less painful than their more broad cases.

@cecton
Copy link
Member Author

cecton commented Oct 28, 2023

I believe standard library is just incomplete and it shouldn't be a good reason to block the idea because of that

I love the spirit ♥️

In my use-case I did have a need for From<&IString> for IString but not for the IArray. Usually I prefer to go with a "per use case basis" as if: if someone needed this, then it's justified (that doesn't mean it's rightfully justified ofc but it has a reason to be).

But I really don't mind adding it for IArray and IMap. Worst case scenario we revert that before 1.0 because we found out it was bad in some way. Not everything has to be perfect and if it's safe to try, we should probably try.

@cecton cecton merged commit ac29809 into yewstack:main Oct 28, 2023
12 checks passed
@cecton cecton deleted the from-ref branch October 28, 2023 12:02
cecton added a commit to cecton/implicit-clone that referenced this pull request Nov 28, 2023
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 this pull request may close these issues.

2 participants