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: get_unchecked resolves to unstable feature use #76479

Closed
Mark-Simulacrum opened this issue Sep 8, 2020 · 7 comments · Fixed by #77201
Closed

regression: get_unchecked resolves to unstable feature use #76479

Mark-Simulacrum opened this issue Sep 8, 2020 · 7 comments · Fixed by #77201
Assignees
Labels
A-stability Area: `#[stable]`, `#[unstable]` etc. P-critical Critical priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Milestone

Comments

@Mark-Simulacrum
Copy link
Member

Unclear if this is a genuine regression or something else is going on, but seems like at least plausibly some change to resolution is involved.

@Mark-Simulacrum Mark-Simulacrum added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example labels Sep 8, 2020
@Mark-Simulacrum Mark-Simulacrum added this to the 1.47.0 milestone Sep 8, 2020
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Sep 8, 2020
@tesuji
Copy link
Contributor

tesuji commented Sep 8, 2020

Bisect result: #73565

searched nightlies: from nightly-2020-04-01 to nightly-2020-09-07
regressed nightly: nightly-2020-08-22
searched commits: from e15510c to de521cb
regressed commit: d9d4d39

bisected with cargo-bisect-rustc v0.5.2

Host triple: x86_64-unknown-linux-musl
Reproduce with:

cargo bisect-rustc --start 2020-04-01 --target x86_64-unknown-linux-gnu --host x86_64-unknown-linux-gnu --verbose 

@Mark-Simulacrum Mark-Simulacrum added E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example and removed E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example labels Sep 8, 2020
@Mark-Simulacrum
Copy link
Member Author

Thanks @lzutao!

Looks like "get_unchecked is now an unstable, hidden method on Iterator" is the cause here, because it means downstream Iterator extensions cannot implement such a method (right?). This didn't cause too much breakage and it feels rare for such a method to be defined as an extension trait rather than inherently, so I'm personally fine with closing this as expected breakage.

It would be good to get an MCVE to get some idea of whether I'm correct here that inherent methods would still work on downstream iterators, and if so then I'm personally fine with closing this.

@tesuji
Copy link
Contributor

tesuji commented Sep 8, 2020

MCVE: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=d334cb2f4f0a985ddf674012a928abd3
Note: it is just an mvce and doesn't reflect what original code does.

// #![feature(trusted_random_access)]
pub struct Foo {
    x: usize,
    y: usize
}

impl Foo {
    fn get_unchecked(&self, x: usize, y: usize) -> (usize, usize) {
        unimplemented!()
    }
}
impl Iterator for Foo {
    type Item = (usize, usize);
    fn next(&mut self) -> Option<(usize, usize)> {
        Some(self.get_unchecked(self.x, self.y))
    }
}

@Dylan-DPC-zz Dylan-DPC-zz added P-critical Critical priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Sep 8, 2020
@Dylan-DPC-zz
Copy link

Marking this as P-critical based on the wg-prioritization discussion

@pnkfelix pnkfelix self-assigned this Sep 17, 2020
@Stupremee Stupremee removed the E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example label Sep 22, 2020
@spastorino
Copy link
Member

Implemented this but @matthewjasper already has a solution too, so leaving it up to them and adjusting the assignee properly.

@spastorino
Copy link
Member

Reopened to track backport of #77201

@camelid camelid added the A-stability Area: `#[stable]`, `#[unstable]` etc. label Sep 30, 2020
@spastorino
Copy link
Member

Closing this one as #77201 was already backported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-stability Area: `#[stable]`, `#[unstable]` etc. P-critical Critical priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants