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

cannot resolve std::borrow::Cow<'_, str>: std::borrow::Borrow<_> #50954

Closed
jebrosen opened this issue May 21, 2018 · 11 comments
Closed

cannot resolve std::borrow::Cow<'_, str>: std::borrow::Borrow<_> #50954

jebrosen opened this issue May 21, 2018 · 11 comments
Milestone

Comments

@jebrosen
Copy link
Contributor

This code fails to compile on nightly 2018-05-20 (playground):

use std::borrow::{Borrow, Cow};

fn takes_into_cow<'a, T: Into<Cow<'a, str>>>(x: T) {
    let s = x.into();
    println!("{}", s);
}

fn forward_cow<'a>(x: Cow<'a, str>) {
    takes_into_cow(x.borrow())
}

fn main() {
    forward_cow("test".into());
}

With this error:

error[E0283]: type annotations required: cannot resolve `std::borrow::Cow<'_, str>: std::borrow::Borrow<_>`
 --> src/main.rs:9:22
  |
9 |     takes_into_cow(x.borrow())
  |                      ^^^^^^

This compiles fine on stable and beta, and probably on nightly as well until some time in the last week or so.

This is a simplified version of the code involved in rwf2/Rocket#643.

@leoyvens
Copy link
Contributor

leoyvens commented May 22, 2018

probably caused by #50170, we didn't do a crater run for that one. In that PR we added:

impl<'a> From<&'a String> for Cow<'a, str>

But we already have:

impl<'a> From<&'a str> for Cow<'a, str>

This makes the following Borrow impl ambiguous in this case:

impl<'a, B> Borrow<B> for Cow<'a, B> where B: ToOwned

It seems all impls (actually not all, see comment below) in that PR can cause the same problem, is it possible to see how many crates were broken by it? Hopefully crater can do that.

@leoyvens leoyvens added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels May 22, 2018
@pietroalbini pietroalbini added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label May 22, 2018
@pietroalbini
Copy link
Member

Scheduled a check-only crater run. Should start in a few days.

@jebrosen
Copy link
Contributor Author

I think the only problematic impls added were impl From<&'a T::Owned> for Cow<'a, T>. This is the last one in each "chunk" of #50170 (comment).

The same error occurs with this CString case:

use std::borrow::{Borrow, Cow};
use std::ffi::{CStr, CString};

fn main() {
    let x = CString::new("x").unwrap();
    let y: Cow<CStr> = x.borrow().into();
}

No code should be doing this one already, but it appears that the existence of impl From<&'a CString> for Cow<'a, CStr> makes impl From<&'a CStr> for Cow<'a, CStr> less useful, so I would argue against those ones.

@pietroalbini
Copy link
Member

Check-only crater run started. Should be finished in ~3 days.

@pietroalbini
Copy link
Member

Hi @leodasvacas (crater requester)! Crater results are at: http://cargobomb-reports.s3.amazonaws.com/issue-50954/index.html. 'Blacklisted' crates (spurious failures etc) can be found here. If you see any spurious failures not on the list, please make a PR against that file.

(interested observers: Crater is a tool for testing the impact of changes on the crates.io ecosystem. You can find out more at the repo if you're curious)

@kennytm
Copy link
Member

kennytm commented May 28, 2018

ogrep-0.2.1:

May 25 11:49:29.066 INFO kablam! error[E0283]: type annotations required: cannot resolve `std::borrow::Cow<'_, str>: std::convert::From<&_>`
May 25 11:49:29.066 INFO kablam!    --> src/main.rs:541:17
May 25 11:49:29.066 INFO kablam!     |
May 25 11:49:29.066 INFO kablam! 541 |                 Cow::from(options.pattern.as_ref())
May 25 11:49:29.066 INFO kablam!     |                 ^^^^^^^^^
May 25 11:49:29.066 INFO kablam!     |
May 25 11:49:29.066 INFO kablam!     = note: required by `std::convert::From::from`

screen-api-0.4.1:

May 25 17:33:44.418 INFO kablam! error[E0283]: type annotations required: cannot resolve `std::string::String: std::convert::AsRef<_>`
May 25 17:33:44.418 INFO kablam!    --> src/websocket/types/mod.rs:163:76
May 25 17:33:44.419 INFO kablam!     |
May 25 17:33:44.419 INFO kablam! 163 |             } => Channel::room_map_view(room_name, shard_name.as_ref().map(AsRef::as_ref)),
May 25 17:33:44.419 INFO kablam!     |                                                                            ^^^^^^^^^^^^^
May 25 17:33:44.419 INFO kablam!     |
May 25 17:33:44.419 INFO kablam!     = note: required by `std::convert::AsRef::as_ref`

travis-0.1.1:

May 25 22:28:02.573 INFO kablam! error[E0283]: type annotations required: cannot resolve `std::string::String: std::convert::AsRef<_>`
May 25 22:28:02.573 INFO kablam!   --> examples/demo.rs:60:50
May 25 22:28:02.573 INFO kablam!    |
May 25 22:28:02.573 INFO kablam! 60 |             let builds = travis.builds(repo.slug.as_ref());
May 25 22:28:02.573 INFO kablam!    |                                                  ^^^^^^

@leoyvens
Copy link
Contributor

This also happens with AsRef, I think it's because of the following impl:

impl<'a, T: ?Sized + ToOwned> AsRef<T> for Cow<'a, T>

I'm not sure what the motivation for the problematic impls was, but if it was to improve functions that take T: Into<Cow<_>> then it might have backfired. Though the set of regressions is small enough that we could consider it acceptable, if we want the impls.

@jebrosen
Copy link
Contributor Author

All three of those look like a case of String -> AsRef::as_ref() -> Cow::from() failing to resolve, so I don't see how impl<'a, T: ?Sized + ToOwned> AsRef<T> for Cow<'a, T> could come into it. I thought there was some blanket impl <T> AsRef<T> for T but that's not the case, so I can't tell what the ambiguity is here.

@alexander-irbis
Copy link

http://play.rust-lang.org/?gist=d540658d847fed9d6bcd5ca5c74828b8&version=nightly&mode=debug

// #![feature(type_ascription)]

use std::borrow::Cow;

fn get_cow(string: &String) -> Cow<str> {
    // Fails on nightly with "type annotations required: 
    // cannot resolve `std::borrow::Cow<'_, str>: std::convert::From<&_>`"
    // https://github.com/rust-lang/rust/issues/50954
    // This works fine in rust 1.27 and earlier
    Cow::from(string.as_ref())
    
    // In rust nightly-1.28 It is easy to fix as in examples below
    // Cow::from(string.as_ref(): &str)
    // Cow::from(string)
    
    // But I found only these are acceptable in both cases
    // Cow::from(string.as_str())
    // Cow::Borrowed(string.as_ref())
    // Cow::Borrowed(string)
}

fn main() {
    let owned = String::from("Hello, world!");
    let _cow = get_cow(&owned);
}

This is a kind of breaking change, that form of from cannot co-exist in the old and the new stable rust.
However in my case (rsmorphy) I can painlessly replace it with Cow::from(string.as_str()).

@leoyvens leoyvens removed the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Jun 9, 2018
@pietroalbini pietroalbini added regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Jun 20, 2018
@pietroalbini pietroalbini added this to the 1.28 milestone Jun 20, 2018
azasypkin added a commit to azasypkin/rust-caster that referenced this issue Jun 24, 2018
Change `as_ref` to `as_str` (to fix `rustc` regression rust-lang/rust#50954).
@Centril Centril added the P-high High priority label Jun 25, 2018
@aturon
Copy link
Member

aturon commented Jun 26, 2018

cc @rust-lang/libs

@Mark-Simulacrum
Copy link
Member

closing in favor of #51844

@pietroalbini pietroalbini removed P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jun 27, 2018
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

No branches or pull requests

8 participants