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

Moving AsciiExt methods to types is a footgun for maintaining backwards compatibility #46510

Closed
shepmaster opened this issue Dec 5, 2017 · 9 comments
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@shepmaster
Copy link
Member

In Rust 1.22.1, this code compiles:

use std::ascii::AsciiExt;

fn main() {
    "a".eq_ignore_ascii_case("a");
}

In Rust 1.23.0.beta.1, it issues a warning:

warning: unused import: `std::ascii::AsciiExt`
 --> src/main.rs:1:5
  |
1 | use std::ascii::AsciiExt;
  |     ^^^^^^^^^^^^^^^^^^^^
  |
  = note: #[warn(unused_imports)] on by default

My tidiness kicks in and I remove the trait — no one wants warnings, right?

However, if I remove this trait, my crates will no longer compile on older versions of Rust. I've actually been bitten by this twice now, and one of those times has been released to crates.io. 😞

@shepmaster
Copy link
Member Author

/cc @LukasKalbertodt @SimonSapin @sfackler (key people on #44042)

@shepmaster
Copy link
Member Author

At least one project pointed to the PR as an issue as well (rusoto/rusoto#877).

@shepmaster
Copy link
Member Author

shepmaster commented Dec 5, 2017

For now, I'm going to do:

#[allow(unused)]
use std::ascii::AsciiExt;

@shepmaster shepmaster added I-nominated T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Dec 5, 2017
@Mark-Simulacrum
Copy link
Member

I believe this has been discussed and the allow(unused) solution is our recommendation; I don't think that's awful, though I agree it's not great.

@SimonSapin
Copy link
Contributor

This was discussed in https://users.rust-lang.org/t/psa-dealing-with-warning-unused-import-std-ascii-asciiext-in-today-s-nightly/13726, but yeah it’s not great. Maybe a different error message would help?

@shepmaster
Copy link
Member Author

I believe this has been discussed
This was discussed

If this has already been discussed to death, then I'm OK with closing this issue as you see fit.

Maybe a different error message would help

Perhaps, although special-casing such a thing in the beta cycle seems rough.

We could potentially also post a cut-down version of the warning and the suggestion into the release notes?

@SimonSapin
Copy link
Contributor

I wouldn’t say "to death".

Release notes sounds good.

Also, if this is judged bad enough, there’s always the option to revert this change in 1.23 beta to give us time to have better diagnostics in 1.24. I don’t have a strong opinion on this.

@alexcrichton
Copy link
Member

We discussed this during the libs triage meeting today and we agreed this is definitely unfortunate! For warnings like deprecation we typically avoid actually deprecating until the alternative hits stable (for this precise reason) but in this scenario it's a little different because we can't implement the change here without having warnings!

We agreed that we'll close this for now though. In general we want to head off these sorts of situations and prevent them from happening (and we do) but this is a particularly special case which seems pretty unlikely to come up again and not necessarily worth the trouble of more compiler changes.

For now though yeah the recommended solution/workaround would be #[allow(unused)]

@sfackler
Copy link
Member

sfackler commented Dec 5, 2017

The only remaining contexts this could come up IIRC are the target-specific extension traits. We need to implement the platform compatibility lint before that can happen though, so those are quite a ways off. It might be worth revisiting a smarter lint then since we'll already be digging around in that infrastructure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

5 participants