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 nightly-2020-09-23: "missing as_flags in implementation" of OpenOptionsExt #77089

Closed
dtolnay opened this issue Sep 23, 2020 · 11 comments · Fixed by #77090
Closed
Labels
P-critical Critical priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@dtolnay
Copy link
Member

dtolnay commented Sep 23, 2020

The following code compiles fine with nightly-2020-09-22 (and stable rustc since 1.10.0) but not nightly-2020-09-23.

use std::os::unix::fs::OpenOptionsExt;

struct MyOpenOptions;

impl OpenOptionsExt for MyOpenOptions {
    fn mode(&mut self, _mode: u32) -> &mut Self {
        unimplemented!()
    }

    fn custom_flags(&mut self, _flags: i32) -> &mut Self {
        unimplemented!()
    }
}
error[E0046]: not all trait items implemented, missing: `as_flags`
 --> src/lib.rs:5:1
  |
5 | impl OpenOptionsExt for MyOpenOptions {
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `as_flags` in implementation
  |
  = help: implement the missing item: `fn as_flags(&self) -> std::result::Result<i32, std::io::Error> { todo!() }`

This is minimized from https://github.com/async-rs/async-std/blob/e8126633a89aafea23259eb9faddb70b89f94423/src/fs/open_options.rs#L303-L313.

Mentioning #76110, #76801, @FedericoPonzi, @joshtriplett.

@dtolnay dtolnay added the regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. label Sep 23, 2020
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Sep 23, 2020
@jyn514
Copy link
Member

jyn514 commented Sep 23, 2020

Whoops, I didn't realize the trait itself was stable. Probably the PR should either be reverted or add a default implementation for as_flags().

@jyn514 jyn514 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 23, 2020
@Stupremee Stupremee added P-critical Critical priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Sep 23, 2020
@Stupremee
Copy link
Member

Assigning P-critical as discussed as part of the Prioritization Working Group procedure and removing I-prioritize.

@yoshuawuyts
Copy link
Member

async-std is now broken on nightly as well: async-rs/async-std#883

@ollie27
Copy link
Member

ollie27 commented Sep 23, 2020

I was under the impression that the *Ext traits were not intended to be implemented outside of std which means this is really an issue with async-std which shouldn't have implemented OpenOptionsExt at all.

std::os::unix::process::CommandExt for example has had several methods added to it since 1.0. Is there something different with OpenOptionsExt or is this just that it was implemented by a popular crate?

@dtolnay
Copy link
Member Author

dtolnay commented Sep 23, 2020

The trait likely should have been sealed, but I think sealed traits were not invented until after 1.0.0.

@jonas-schievink
Copy link
Contributor

Can we retroactively seal CommandExt? It seems like we should, if we keep adding methods like in #72160.

@jyn514
Copy link
Member

jyn514 commented Oct 11, 2020

Wouldn't that be a breaking change?

@jonas-schievink
Copy link
Contributor

Yes, just like adding more methods to it (which has been done several times)

@joshtriplett
Copy link
Member

We could do a crater run to see if sealing it would break anything.

@jyn514
Copy link
Member

jyn514 commented Oct 11, 2020

Well clearly it would - it would break async-std the same way as_flags() broke async std. We need to get them not to use OpenOptionsExt before we can change anything here.

@joshtriplett
Copy link
Member

@jyn514 My comment was in reference to sealing CommandExt, not OpenOptionsExt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-critical Critical priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. 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.

8 participants