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

Add generic conversion traits #23538

Closed
wants to merge 1 commit into from
Closed

Conversation

aturon
Copy link
Member

@aturon aturon commented Mar 20, 2015

This commit:

  • Introduces std::convert, providing an implementation of
    RFC 529.

  • Deprecates the AsPath, AsOsStr, and IntoBytes traits, all
    in favor of the corresponding generic conversion traits.

    Consequently, various IO APIs now take As<Path> rather than
    AsPath, and so on. Since the types provided by std implement both
    traits, this should cause relatively little breakage.

  • Deprecates many from_foo constructors in favor of from.

  • Changes PathBuf::new to take no argument (creating an empty buffer,
    as per convention). The previous behavior is now available as
    PathBuf::from.

  • De-stabilizes IntoCow. It's not clear whether we need this separate trait.

Closes #14433
Closes #22751

[breaking-change]

r? @alexcrichton
cc @gankro @erickt @cmr

@aturon
Copy link
Member Author

aturon commented Mar 20, 2015

PRELIMINARY PR: Do not merge until the RFC is approved.

@aturon
Copy link
Member Author

aturon commented Mar 20, 2015

cc @sfackler @wycats

@aturon
Copy link
Member Author

aturon commented Mar 20, 2015

Per IRC: @alexcrichton is very uncomfortable with these living in libcollections rather than libcore. This placement was to facilitate impls like impl<T: Clone> To<Vec<T>> for [T] which would otherwise be disallowed due to coherence.

@aturon
Copy link
Member Author

aturon commented Mar 20, 2015

cc @reem

@aturon
Copy link
Member Author

aturon commented Mar 20, 2015

Per IRC: @reem points out that the same coherence problems make it much more difficult to define "implicit conversions" via generics targetting user-land types: you cannot implement To<MyType<T>> for StdType<T> due to coherence.

Introducing an ad hoc conversion trait, on the other hand, makes it possible to do this in userland.

@aturon
Copy link
Member Author

aturon commented Mar 20, 2015

Updated: I've now moved the traits into libcore, and have a (rather sad) impl of To<Vec<u8>> for [u8] which coherence permits.

@bors
Copy link
Contributor

bors commented Mar 20, 2015

☔ The latest upstream changes (presumably #23548) made this pull request unmergeable. Please resolve the merge conflicts.

@aturon
Copy link
Member Author

aturon commented Mar 20, 2015

I believe this is now ready to go, once the RFC itself has been merged.

@@ -14,6 +14,7 @@

use core::clone::Clone;
use core::cmp::{Eq, Ord, Ordering, PartialEq, PartialOrd};
use core::convert::As;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could these traits also go into the libcore prelude? I think files like this can have use core::prelude::* at the top as well (slowly been migrating over time).

@alexcrichton
Copy link
Member

cc #23567

@aturon aturon changed the title [WIP] Add generic conversion traits Add generic conversion traits Mar 23, 2015
@aturon
Copy link
Member Author

aturon commented Mar 23, 2015

Updated:

  • Removed To for the time being.

  • Renamed As to AsRef.

  • Renamed all methods to match their trait names.

  • All conversion functionality now in libcore; added to core::prelude

  • Added From => Into implementation, which makes it possible to add conversions in both directions without running afoul of coherence. For example, we now have From<[T]> for Vec<T> where T: Clone, which yields the corresponding Into going in the other direction -- despite the fact that the two types live in different crates.

    I also believe this addresses a few concerns about things implementing From instead of Into

  • Removed AsRef impls for slices from Option/Result, which was causing a bad interaction with their as_ref methods. This functionality is available by calling as_slice directly.

All in all, I feel much better with the form this PR is taking now. It feels cleaner, leaner, more consistent and more extensible. Thanks for the feedback making this possible!

@alexcrichton: re-r?

@@ -67,8 +67,8 @@ pub fn current_dir() -> io::Result<PathBuf> {
/// println!("Successfully changed working directory to {}!", root.display());
/// ```
#[stable(feature = "env", since = "1.0.0")]
pub fn set_current_dir<P: AsPath + ?Sized>(p: &P) -> io::Result<()> {
os_imp::chdir(p.as_path())
pub fn set_current_dir<P: AsRef<path::Path> + ?Sized>(p: &P) -> io::Result<()> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this module import Path directly and use AsRef<Path>?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahha! It can now. I think when I first wrote it Path was still in the prelude :)

@alexcrichton
Copy link
Member

I, too, am quite happy with how this is turning out! r=me with the as_slice methods on Option and Result remaining #[unstable] (as they were before)

@aturon
Copy link
Member Author

aturon commented Mar 23, 2015

pings addressed :)

@alexcrichton
Copy link
Member

@bors: r+ 8389253

@aturon
Copy link
Member Author

aturon commented Mar 23, 2015

@bors: r=alexcrichton 97c8e43

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 23, 2015
Conflicts:
	src/librustc_back/rpath.rs
This commit:

* Introduces `std::convert`, providing an implementation of
RFC 529.

* Deprecates the `AsPath`, `AsOsStr`, and `IntoBytes` traits, all
in favor of the corresponding generic conversion traits.

  Consequently, various IO APIs now take `AsRef<Path>` rather than
`AsPath`, and so on. Since the types provided by `std` implement both
traits, this should cause relatively little breakage.

* Deprecates many `from_foo` constructors in favor of `from`.

* Changes `PathBuf::new` to take no argument (creating an empty buffer,
  as per convention). The previous behavior is now available as
  `PathBuf::from`.

* De-stabilizes `IntoCow`. It's not clear whether we need this separate trait.

Closes rust-lang#22751
Closes rust-lang#14433

[breaking-change]
@alexcrichton
Copy link
Member

Rolled into #23654

@bors
Copy link
Contributor

bors commented Mar 24, 2015

☔ The latest upstream changes (presumably #23654) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants