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 aligner-0.1.3, Rust 1.17 #40952

Closed
brson opened this issue Mar 31, 2017 · 13 comments
Closed

Regression in aligner-0.1.3, Rust 1.17 #40952

brson opened this issue Mar 31, 2017 · 13 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.
Milestone

Comments

@brson
Copy link
Contributor

brson commented Mar 31, 2017

https://github.com/kaegi/aligner

ec2-user@ip-10-145-56-122:~/triage/aligner$ git log -1
commit 89a5e6e62f54fcfe7a9a26d1a969f11c7de11399
Author: kaegi <[email protected]>
Date:   Sun Mar 12 22:43:07 2017 +0100

    Bump version to 0.1.4
ec2-user@ip-10-145-56-122:~/triage/aligner$ rustc +beta -Vv
rustc 1.17.0-beta.2 (b7c276653 2017-03-20)
binary: rustc
commit-hash: b7c27665307704a9b158fe242e88e83914029415
commit-date: 2017-03-20
host: x86_64-unknown-linux-gnu
release: 1.17.0-beta.2
LLVM version: 3.9
101 ec2-user@ip-10-145-56-122:~/triage/aligner$ cargo +beta test
   Compiling ascii v0.7.1
   Compiling cfg-if v0.1.0
   Compiling odds v0.2.25
   Compiling rustc-demangle v0.1.4
   Compiling unicode-segmentation v1.1.0
   Compiling bitflags v0.8.0
   Compiling vec_map v0.7.0
   Compiling ansi_term v0.9.0
   Compiling winapi-build v0.1.1
   Compiling nodrop v0.1.9
   Compiling combine v2.3.1
   Compiling strsim v0.6.0
   Compiling arrayvec v0.3.20
   Compiling unicode-width v0.1.4
   Compiling gcc v0.3.43
   Compiling libc v0.2.21
   Compiling dbghelp-sys v0.2.0
   Compiling backtrace v0.3.0
   Compiling winapi v0.2.8
   Compiling atty v0.2.2
   Compiling term_size v0.2.3
   Compiling rand v0.3.15
   Compiling time v0.1.36
   Compiling clap v2.21.1
   Compiling backtrace-sys v0.1.10
   Compiling kernel32-sys v0.2.2
   Compiling pbr v1.0.0-alpha.2
error[E0283]: type annotations required: cannot resolve `std::string::String: std::convert::AsRef<_>`
   --> /home/ec2-user/.cargo/registry/src/github.com-1ecc6299db9ec823/pbr-1.0.0-alpha.2/src/pb.rs:378:64
    |
378 |                         base = base + repeat!(self.bar_current.as_ref(), curr_count - 1) +
    |                                       -------------------------^^^^^^------------------- in this macro invocation

error: aborting due to previous error

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

cc @kaegi

@brson brson added the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label Mar 31, 2017
@alexcrichton
Copy link
Member

This affects https://github.com/GGist/bip-rs/tree/master/bip_metainfo at 2e7b786ed48422b48b2e5d4679d411dd894d5c95 as well

cc @GGist

@alexcrichton
Copy link
Member

This affects checksums as well

cc @nabijaczleweli

@alexcrichton
Copy link
Member

This affects dmsort as well

cc @emilk

@alexcrichton
Copy link
Member

This affects pbr as well

cc @a8m

@alexcrichton
Copy link
Member

Er sorry everyone cc'd, looks like the regression is with pbr, not other crates.

@a8m
Copy link

a8m commented Mar 31, 2017

Thanks for letting me know that @alexcrichton. fixed in pbr-1.0.0-alpha.3.

@alexcrichton
Copy link
Member

Out of curiosity, do you know what change to rust caused this?

@a8m
Copy link

a8m commented Apr 1, 2017

I didn't dig into this, but here's the fix that solved the issue.

@alexcrichton
Copy link
Member

Thanks @a8m, sounds like a "standard change that causes inference regressions", but I'm not personally quite ready to close this out.

@rust-lang/libs any thoughts on which PR may have caused this?

@aturon
Copy link
Member

aturon commented Apr 4, 2017

@alexcrichton I'm not sure which particular PR, but we've definitely been r+-ing a bunch of AsRef expansions. I think this is in fact par for the course, and we should close this issue (but also work on mitigation, like the flag to turn off input type inference).

@alexcrichton
Copy link
Member

Actually taking a look at the code, my guess is that this is caused by #40028 which added the second FromIterator<&T> for String, whereas before there was only FromIterator<&str>.

Good lord that's a subtle inference issue.

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

@aturon I definitely want to disable input type inference, at least for some traits, if not by default. (I think in retrospect it ought to be something one can opt into on the impl -- i.e., it's kind of a declaration that you promise not to add further impls with the same self type, similar to a negative impl declaration.)

@alexcrichton alexcrichton added this to the 1.17 milestone Apr 10, 2017
@alexcrichton
Copy link
Member

Ok we discussed this during libs triage the other day, and the conclusion was to close this. The known regressions have been fixed and we'd otherwise categorize this under "known breakage".

If this comes up over time though please let us know as we're always interested in understanding when changes like this cause problems!

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