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

Regression in autoderef on ascii traits #32074

Closed
Manishearth opened this issue Mar 6, 2016 · 8 comments
Closed

Regression in autoderef on ascii traits #32074

Manishearth opened this issue Mar 6, 2016 · 8 comments
Labels
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.

Comments

@Manishearth
Copy link
Member

The following code compiles on stable but gives an error on beta/nightly:

use std::ascii::AsciiExt; 

fn main() {
        let x = "a".to_string();
        x.eq_ignore_ascii_case("A");
}

playpen

<anon>:5:32: 5:35 error: mismatched types:
 expected `&collections::string::String`,
    found `&'static str`
(expected struct `collections::string::String`,
    found str) [E0308]
<anon>:5         x.eq_ignore_ascii_case("A");

This was caused by the new impl of AsciiExt on String: 700ac0e

cc @SimonSapin @alexcrichton

@Manishearth Manishearth added I-nominated T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Mar 6, 2016
@Manishearth
Copy link
Member Author

The impl is unstable, but IIRC unstable impls are still accessible?

@Manishearth
Copy link
Member Author

This might be an acceptable kind of regression. It's of the "added new impl" type, which we have already said is okay if the only change necessary to fix it is explicit UFCS. In this case, explicit deref is required, which is a similar annotation burden (however, the fact that we have to do this isn't very clear from the error message).

Might need a crater run if we plan to keep it, cc @brson

@SimonSapin
Copy link
Contributor

I’ll move the into_* methods to a separate trait (like they used to be at some point), this should fix this regression.

@Manishearth
Copy link
Member Author

That should work!

@SimonSapin
Copy link
Contributor

I’ve submitted PR #32076. The discussion for what this API should be like is at #27809.

SimonSapin added a commit to SimonSapin/rust that referenced this issue Mar 6, 2016
Implementing `AsciiExt` for `String` and `Vec<u8>` caused a regression:
rust-lang#32074
and the `where Self: Sized` hack to have the `into_` methods in that trait
(which is also implemented for DST `str` and `[u8]`) was rather clunky.

CC rust-lang#27809
@alexcrichton
Copy link
Member

The libs team discussed this during triage today and the decision was to revert the into_ascii_* methods and go back to the state on 1.7 stable today.

@alexcrichton alexcrichton added P-high High priority and removed I-nominated labels Mar 17, 2016
@SimonSapin
Copy link
Contributor

@alexcrichton 1.7 doesn’t have any into_ascii_* methods if I’m reading https://doc.rust-lang.org/1.7.0/std/ascii/trait.AsciiExt.html correctly. So is the decision to remove them?

alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 17, 2016
The addition of these methods in rust-lang#31335 required adding impls of the trait for
the `String` and `Vec<T>` types. This unfortunately caused a regression (rust-lang#32074)
in type inference for using these methods which the libs team has decided to not
push forward with. These methods were stabilized in rust-lang#32020 which was intended to
get backported to beta, but the backport hasn't happened just yet. This commit
reverts both the addition and stabilization of these methods.

One proposed method of handling this, in rust-lang#32076, was to move the methods to an
extra trait to avoid conflicts with type inference. After some discussion,
however, the libs team concluded that we probably want to reevaluate what we're
doing here, so discussion will continue on the tracking issue, rust-lang#27809.
@alexcrichton
Copy link
Member

Ah yeah sorry I don't think I explained enough. The plan of action is detailed a little more clearly in the description of #32314, however.

bors added a commit that referenced this issue Mar 19, 2016
std: Revert addition of `into_ascii_*` methods

The addition of these methods in #31335 required adding impls of the trait for
the `String` and `Vec<T>` types. This unfortunately caused a regression (#32074)
in type inference for using these methods which the libs team has decided to not
push forward with. These methods were stabilized in #32020 which was intended to
get backported to beta, but the backport hasn't happened just yet. This commit
reverts both the addition and stabilization of these methods.

One proposed method of handling this, in #32076, was to move the methods to an
extra trait to avoid conflicts with type inference. After some discussion,
however, the libs team concluded that we probably want to reevaluate what we're
doing here, so discussion will continue on the tracking issue, #27809.

Closes #32074
alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 21, 2016
Just to make sure we don't accidentally break this in the future.
alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 24, 2016
The addition of these methods in rust-lang#31335 required adding impls of the trait for
the `String` and `Vec<T>` types. This unfortunately caused a regression (rust-lang#32074)
in type inference for using these methods which the libs team has decided to not
push forward with. These methods were stabilized in rust-lang#32020 which was intended to
get backported to beta, but the backport hasn't happened just yet. This commit
reverts both the addition and stabilization of these methods.

One proposed method of handling this, in rust-lang#32076, was to move the methods to an
extra trait to avoid conflicts with type inference. After some discussion,
however, the libs team concluded that we probably want to reevaluate what we're
doing here, so discussion will continue on the tracking issue, rust-lang#27809.

Conflicts:
	src/libstd/ascii.rs
Manishearth added a commit to Manishearth/rust that referenced this issue Mar 26, 2016
std: Add regression test for rust-lang#32074

Just to make sure we don't accidentally break this in the future.
Manishearth added a commit to Manishearth/rust that referenced this issue Mar 26, 2016
std: Add regression test for rust-lang#32074

Just to make sure we don't accidentally break this in the future.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

No branches or pull requests

3 participants