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 astar-0.1.1, Rust 1.17 #40193

Closed
brson opened this issue Mar 1, 2017 · 11 comments
Closed

Regression in astar-0.1.1, Rust 1.17 #40193

brson opened this issue Mar 1, 2017 · 11 comments
Labels
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

@brson
Copy link
Contributor

brson commented Mar 1, 2017

brian@ip-10-145-43-250:~/dev/astar⟫ git remote -v
origin  https://github.com/TyOverby/astar (fetch)
origin  https://github.com/TyOverby/astar (push)
brian@ip-10-145-43-250:~/dev/astar⟫ git log -1
commit 1a2edf90779a317d61bd747d24ba948ded522590
Merge: 1dcc942 fa4d420
Author: Ty Overby <ty@pre-alpha.com>
Date:   Sun Dec 18 22:50:01 2016 -0800

    Merge pull request #7 from andrewcsmith/master

    A few simplifications
127 brian@ip-10-145-43-250:/mnt2/dev⟫ rustc +nightly -Vv
rustc 1.17.0-nightly (be760566c 2017-02-28)
binary: rustc
commit-hash: be760566cf938d11d34c2f6bd90d8fd0f67c2344
commit-date: 2017-02-28
host: x86_64-unknown-linux-gnu
release: 1.17.0-nightly
LLVM version: 3.9

brian@ip-10-145-43-250:~/dev/astar⟫ cargo +nightly test
    Updating registry `https://github.com/rust-lang/crates.io-index`
 Downloading typed-arena v1.2.0
 Downloading num v0.1.37
 Downloading num-bigint v0.1.36
 Downloading num-iter v0.1.33
 Downloading num-complex v0.1.36
 Downloading num-traits v0.1.37
 Downloading num-integer v0.1.33
 Downloading num-rational v0.1.36
   Compiling num-traits v0.1.37
   Compiling libc v0.2.20
   Compiling rustc-serialize v0.3.22
   Compiling typed-arena v1.2.0
   Compiling rand v0.3.15
   Compiling num-integer v0.1.33
   Compiling num-iter v0.1.33
   Compiling num-complex v0.1.36
   Compiling num-bigint v0.1.36
   Compiling num-rational v0.1.36
   Compiling num v0.1.37
   Compiling astar v0.1.1 (file:///mnt2/dev/astar)
error[E0282]: type annotations needed
  --> src/test.rs:67:44
   |
67 |     assert_eq!(p, vec![(0, 0)].into_iter().collect());
   |                                            ^^^^^^^ cannot infer type for `B`

error: aborting due to previous error

error: Could not compile `astar`.
Build failed, waiting for other jobs to finish...
error: build failed

Not on 1.16.

cc @TyOverby

@brson brson added the regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. label Mar 1, 2017
@TyOverby
Copy link
Contributor

TyOverby commented Mar 2, 2017

This is a pretty weird one. The test in question:

#[test]
fn test_start_end() {
    // path() statically returns Option<VecDeque<(i32, i32)>> 
    let p = path((0,0), (0,0)).unwrap();
    assert_eq!(p, vec![(0, 0)].into_iter().collect());
}

Just looking at this, I would expect it to infer that it needs to collect into VecDeque<(i32, i32>, but seeing as assert_eq uses PartialEq, which could be implemented for any T and R, picking a collection type seems undecidable.

Has the implementation of assert_eq changed at all?

@ollie27
Copy link
Member

ollie27 commented Mar 2, 2017

Probably #38661 then.

@alexcrichton
Copy link
Member

@TyOverby yeah I believe the PR pointed out by @ollie27 likely caused the regression here as there's more than one impl available now (or sorta through generics) which means that the compiler can't deduce that the rhs is a vecdeque as well.

@alexcrichton alexcrichton added I-nominated T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Mar 2, 2017
@TyOverby
Copy link
Contributor

TyOverby commented Mar 2, 2017

Yep. That would do it.

@TyOverby
Copy link
Contributor

TyOverby commented Mar 2, 2017

So what's the process here? I could certainly fix and publish Astar, but is this considered a "libs breaking change"?

@alexcrichton
Copy link
Member

@TyOverby we'll discuss this at next libs triage to decide whether we'd like to revert, but my guess is that unless updating is difficult for you we're likely to keep this as-is.

If that ends up being the case, then yeah if you'd like to publish a fix to Astar that'd be great. Please let us know though if that's an inconvenience though as it will affect the discussion of what to do about this change!

@alexcrichton
Copy link
Member

Ok the libs team met today and we discussed this issue. Our conclusion was that we'd like to classify this under "acceptable breakage". @TyOverby would it be possible to push an update to the crate in question? If it's difficult to do please just let us know!

@arielb1
Copy link
Contributor

arielb1 commented Mar 16, 2017

1.17 is now beta

@arielb1 arielb1 added regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Mar 16, 2017
@TyOverby
Copy link
Contributor

@alexcrichton The crate is building now.

@alexcrichton
Copy link
Member

Thanks @TyOverby!

@TyOverby
Copy link
Contributor

🎉

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

5 participants