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

Nightly regression. #32301

Closed
dpc opened this issue Mar 17, 2016 · 5 comments
Closed

Nightly regression. #32301

dpc opened this issue Mar 17, 2016 · 5 comments
Labels
regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@dpc
Copy link
Contributor

dpc commented Mar 17, 2016

Mioco works on nightly, but recent nightly (around two day ago) does not compile mioco anymore.

I have minimized code to:

https://gist.github.com/dpc/a6bd306b8187651ec700

This code compiles on stable, but breaks on nightly, so I believe this is a regression.

dpc@futex ~/t/mioco-break (master) [I]> multirust override nightly ; make
multirust: using existing install for 'nightly'
multirust: override toolchain for '/home/dpc/tmp/mioco-break' set to 'nightly'
cargo build
   Compiling mioco-break v0.1.0 (file:///home/dpc/tmp/mioco-break)
udp.rs:11:5: 13:6 error: duplicate definitions with name `try_read`: [E0201]
udp.rs:11     pub fn try_read(&mut self, buf: &mut [u8]) -> io::Result<Option<(usize, SocketAddr)>> {
udp.rs:12         Ok(None)
udp.rs:13     }
udp.rs:11:5: 13:6 help: run `rustc --explain E0201` to see a detailed explanation
lib.rs:14:5: 16:6 note: conflicting definition is here:
lib.rs:14     pub fn try_read(&mut self, buf: &mut [u8]) -> io::Result<Option<usize>> {
lib.rs:15         Ok(None)
lib.rs:16     }
error: aborting due to previous error
       error Could not compile `mioco-break`.

To learn more, run the command again with --verbose.
Makefile:2: recipe for target 'all' failed
make: *** [all] Error 101
dpc@futex ~/t/mioco-break (master) [2] [I]> multirust override stable ; make
multirust: using existing install for 'stable'
multirust: override toolchain for '/home/dpc/tmp/mioco-break' set to 'stable'
cargo build
   Compiling mioco-break v0.1.0 (file:///home/dpc/tmp/mioco-break)
udp.rs:11:5: 13:6 warning: method is never used: `try_read`, #[warn(dead_code)] on by default
udp.rs:11     pub fn try_read(&mut self, buf: &mut [u8]) -> io::Result<Option<(usize, SocketAddr)>> {
udp.rs:12         Ok(None)
udp.rs:13     }
udp.rs:11:32: 11:35 warning: unused variable: `buf`, #[warn(unused_variables)] on by default
udp.rs:11     pub fn try_read(&mut self, buf: &mut [u8]) -> io::Result<Option<(usize, SocketAddr)>> {
                                         ^~~
lib.rs:14:32: 14:35 warning: unused variable: `buf`, #[warn(unused_variables)] on by default
lib.rs:14     pub fn try_read(&mut self, buf: &mut [u8]) -> io::Result<Option<usize>> {
                                         ^~~
@dpc
Copy link
Contributor Author

dpc commented Mar 17, 2016

Just to comment on the code: mio::udp::UdpSocket does not implement mio::Evented so impl<MT> MioAdapter<MT> where MT: mio::Evented + mio::TryRead + 'static should not be applied.

@sfackler sfackler added the regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. label Mar 17, 2016
dpc added a commit to dpc/mioco.pre-0.9 that referenced this issue Mar 17, 2016
@nikomatsakis
Copy link
Contributor

The problem here is that we started applying the coherence rules to inherent impls -- the fact that we did not do so before was a bug. The coherence rules are somewhat more restrictive about negative reasoning. For example here, you are not allowed to rely on the fact that UdpSocket does not implement Evented because --- in principle, at least --- mio might add an impl of Evented for UdpSocket and that is not supposed to be a breaking change. (That is, in general, adding an impl of a trait for a type is not a breaking change -- nothing specific to mio). But it would in fact break your code given those two impls.

We probably ought to have had a warning period for this change, however.

@nikomatsakis nikomatsakis added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 17, 2016
@nikomatsakis
Copy link
Contributor

cc @aturon

@nagisa
Copy link
Member

nagisa commented Mar 17, 2016

Duplicate of #32247 I think?

@aturon
Copy link
Member

aturon commented Mar 17, 2016

Yes, this is effectively a dupe. I'm planning to land a change ASAP to produce a warning instead of an error for now, though we (at the moment) intend to go forward with this change.

@aturon aturon closed this as completed Mar 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants