Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Either::as_ref method #31

Open
mexus opened this issue Sep 15, 2018 · 5 comments
Open

Either::as_ref method #31

mexus opened this issue Sep 15, 2018 · 5 comments
Labels
potential-2.0 potential changes for `either` 2.0

Comments

@mexus
Copy link
Contributor

mexus commented Sep 15, 2018

Hi everyone,

I've just hit an issue while trying to replace an impl AsRef with a concrete Either type in a return position, here's an example: https://play.rust-lang.org/?gist=1f4771605ae47ea7e4e10881a994a2dc&version=stable&mode=debug&edition=2015

I believe it happens because Either::as_ref method effectively hides the AsRef::as_ref method :(

Shouldn't it be renamed to something like to_ref so it doesn't overlap with the AsRef trait, or is there any reason behind this specific name? The same goes for as_mut and probably for into_iter as well.

@cuviper
Copy link
Member

cuviper commented Sep 18, 2018

I believe it happens because Either::as_ref method effectively hides the AsRef::as_ref method :(

Yes, inherent methods take priority over trait methods.

Shouldn't it be renamed to something like to_ref so it doesn't overlap with the AsRef trait, or is there any reason behind this specific name? The same goes for as_mut

Renaming would be a breaking change, at least, requiring us to bump to either 2.0.

Either::as_ref and as_mut mimic the methods on Option and Result, creating references on the inner types while still remaining an Either. This is unfortunately different than what AsRef and AsMut do, but note that Option and Result don't implement those traits at all! Whereas Either is a weird chimera where both semantics can make sense in different contexts...

and probably for into_iter as well.

You can see in #12 when into_iter was added. This one is also weird as we couldn't implement IntoIterator directly, because we already conditionally implement Iterator, and there's a blanket impl<I> IntoIterator for I where I: Iterator which then applies.

But into_iter doesn't cause the same problem you have here, because the inherent method is a strict superset of the trait method. If the inner types aren't already Iterators, only the inherent method can be called. If they are already Iterators, then the inherent and trait methods actually return the exact same thing!

@mexus
Copy link
Contributor Author

mexus commented Sep 18, 2018

Thanks for such a detailed answer! :)

As you've mentioned Either is more of a chimera than a direct analogue of either Iterator or Option or Result or whatever, so I believe an interface should not be simply "taken" from an existing type.. especially an inherent method that hides a trait method :)

To make my long story short, do you agree that it's not a good thing that AsRef trait can't be utilized to its full extent while there is an Either::as_ref method? :) If yes, then I think as a first step a method like by_ref (or whatever) could be introduced to provide the same functionality (Either<A, B> → Either<&A, &B>) and mark the as_ref as deprecated, and when everything's ready for 2.0 the original one might be safely removed.
If you don't agree then I'll just go with a workaround, it's not a big deal at all.. well, it's definitely not big enough to start a dispute or anything.

Btw, nice trick with the into_iter! Didn't think of it that far.. tbh I've just briefly looked at all the method names that might clash with any common traits from the stdlib :)

Anyhow, thanks again for the explanation!

@cuviper
Copy link
Member

cuviper commented Sep 28, 2018

As you've mentioned Either is more of a chimera than a direct analogue of either Iterator or Option or Result or whatever, so I believe an interface should not be simply "taken" from an existing type..

Option and Result are not direct analogues either, but it's convenient for them to have similar interfaces for functionality that makes sense in both. Either also follows suit, and ideally this is supposed to help user's intuition.

do you agree that it's not a good thing that AsRef trait can't be utilized to its full extent while there is an Either::as_ref method?

I think it's an unfortunate clash, but not that big of a deal. It still works for trait bounds, and you can call it directly in the UFCS style, however ugly that may be. 🙂

I think it's not helpful to make deprecations and replacements until we're actually planning a 2.0, then we can do it to ease the transition. Until then, it wouldn't help the current conflict, so the deprecation would just add more noise.

@mexus
Copy link
Contributor Author

mexus commented Oct 6, 2018

Well, sounds reasonable, but then another question appears: do you think it's worth adding a milestone or a tag or something for a probable 2.0? I have a bad feeling that all the improvement ideas might easily get lost in time, and if I close this issue (since it's more-or-less resolved) at least I will definitely forget about it after some time..

@cuviper cuviper added the potential-2.0 potential changes for `either` 2.0 label Oct 15, 2018
@cuviper
Copy link
Member

cuviper commented Oct 15, 2018

I don't know that 2.0 is "probable" any time soon, but I just added a "potential-2.0" label, at least. :)

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
potential-2.0 potential changes for `either` 2.0
Projects
None yet
Development

No branches or pull requests

2 participants