Use TryInto for IntoUrl

[djc]

It looks like you intended to allow this change to be done without breaking the API? So far I'm unsuccessful at fixing it.

diff --git a/src/into_url.rs b/src/into_url.rs
index 4e56f02..ec28288 100644
--- a/src/into_url.rs
+++ b/src/into_url.rs
@@ -1,3 +1,4 @@
+use std::convert::TryInto;
 use url::Url;
 
 /// A trait to try to convert some type into a `Url`.
@@ -5,35 +6,9 @@ use url::Url;
 /// This trait is "sealed", such that only types within reqwest can
 /// implement it. The reason is that it will eventually be deprecated
 /// and removed, when `std::convert::TryFrom` is stabilized.
-pub trait IntoUrl: PolyfillTryInto {}
-
-impl<T: PolyfillTryInto> IntoUrl for T {}
-
-pub trait PolyfillTryInto {
-    // Besides parsing as a valid `Url`, the `Url` must be a valid
-    // `http::Uri`, in that it makes sense to use in a network request.
-    fn into_url(self) -> crate::Result<Url>;
-}
-
-impl PolyfillTryInto for Url {
-    fn into_url(self) -> crate::Result<Url> {
-        if self.has_host() {
-            Ok(self)
-        } else {
-            Err(crate::error::url_bad_scheme(self))
-        }
-    }
-}
-
-impl<'a> PolyfillTryInto for &'a str {
-    fn into_url(self) -> crate::Result<Url> {
-        Url::parse(self).map_err(crate::error::builder)?.into_url()
-    }
-}
-
-impl<'a> PolyfillTryInto for &'a String {
+pub trait IntoUrl: TryInto<Url> where Self::Error: Into<crate::Error> {
     fn into_url(self) -> crate::Result<Url> {
-        (&**self).into_url()
+        Ok(self.try_into()?)
     }
 }

Would be nice to have this fixed now that TryInto is available.

[jsha]

I spent some time working on this today and made a bit of progress.

There are a few blockers:

Url's TryFrom<&str> landed in March, but has yet to be incorporated into a release: servo/rust-url#569.

Url doesn't implement TryFrom<String>.

Url does implement From<Url> because of the reflexive blanket implementation, but that maps to a Result that can return Infallible, while in my candidate code here I've specified the associated type for IntoUrl's supertrait as returning a url::ParseError. This can probably be fixed by doing something more like the type bounds approach you used, but we need to implement a From to convert between ParseError and reqwest::error::Error, which is beyond me at the moment.

At any rate, here is the patch I have going so far:

diff --git a/Cargo.toml b/Cargo.toml
index 1cd1edc..c14f236 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -62,7 +62,7 @@ __internal_proxy_sys_no_cache = []
 
 [dependencies]
 http = "0.2"
-url = "2.1"
+url = { path = "../rust-url" }
 bytes = "0.5"
 serde = "1.0"
 ## json
diff --git a/src/into_url.rs b/src/into_url.rs
index 4e56f02..63ad00a 100644
--- a/src/into_url.rs
+++ b/src/into_url.rs
@@ -1,3 +1,7 @@
+use crate::error;
+use std::convert::TryInto;
+use std::result::Result;
+use url;
 use url::Url;
 
 /// A trait to try to convert some type into a `Url`.
@@ -5,37 +9,18 @@ use url::Url;
 /// This trait is "sealed", such that only types within reqwest can
 /// implement it. The reason is that it will eventually be deprecated
 /// and removed, when `std::convert::TryFrom` is stabilized.
-pub trait IntoUrl: PolyfillTryInto {}
-
-impl<T: PolyfillTryInto> IntoUrl for T {}
-
-pub trait PolyfillTryInto {
-    // Besides parsing as a valid `Url`, the `Url` must be a valid
-    // `http::Uri`, in that it makes sense to use in a network request.
-    fn into_url(self) -> crate::Result<Url>;
-}
-
-impl PolyfillTryInto for Url {
-    fn into_url(self) -> crate::Result<Url> {
-        if self.has_host() {
-            Ok(self)
-        } else {
-            Err(crate::error::url_bad_scheme(self))
+pub trait IntoUrl: TryInto<Url, Error = url::ParseError> {
+    fn into_url(self) -> Result<Url, error::Error> {
+        match self.try_into() {
+            Ok(u) => Ok(u),
+            Err(e) => Err(error::request(e)),
         }
     }
 }
 
-impl<'a> PolyfillTryInto for &'a str {
-    fn into_url(self) -> crate::Result<Url> {
-        Url::parse(self).map_err(crate::error::builder)?.into_url()
-    }
-}
-
-impl<'a> PolyfillTryInto for &'a String {
-    fn into_url(self) -> crate::Result<Url> {
-        (&**self).into_url()
-    }
-}
+impl<'a> IntoUrl for &'a str {}
+// impl<'a> IntoUrl for &'a String {}
+// impl<'a> IntoUrl for &'a Url {}
 
 if_hyper! {
     pub(crate) fn expect_uri(url: &Url) -> http::Uri {

Also worth noting: This patch produces a seemingly-unrelated build error in request.rs:

error[E0308]: mismatched types
   --> src/async_impl/request.rs:302:64
    |
302 |             let serializer = serde_urlencoded::Serializer::new(&mut pairs);
    |                                                                ^^^^^^^^^^ expected struct `url::form_urlencoded::Serializer`, found a different struct `url::form_urlencoded::Serializer`
    |
    = note: expected mutable reference `&mut url::form_urlencoded::Serializer<'_, _>`
               found mutable reference `&mut url::form_urlencoded::Serializer<'_, url::UrlQuery<'_>>`
    = note: perhaps two different versions of crate `url` are being used?

But the answer is presumably that last note - two different versions of crate url in use.

[seanmonstar]

It might not be possible to do this and provide all the implementations we want.