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

Type inference regression on beta using as _ #45480

Closed
bstrie opened this issue Oct 23, 2017 · 10 comments
Closed

Type inference regression on beta using as _ #45480

bstrie opened this issue Oct 23, 2017 · 10 comments
Labels
A-inference Area: Type inference regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Milestone

Comments

@bstrie
Copy link
Contributor

bstrie commented Oct 23, 2017

Spotted by @gankro :

fn main() {
    let mut x: u64 = 0;
    let y: u32 = 0;
    x += y as _;   // Compiles on stable, fails on beta/nightly
}
@bstrie bstrie added A-inference Area: Type inference regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Oct 23, 2017
@ollie27
Copy link
Member

ollie27 commented Oct 23, 2017

I believe this is expected breakage from #44287.

jrmuizel added a commit to jrmuizel/zydis-rs that referenced this issue Oct 23, 2017
The regression is filed as rust-lang/rust#45480. It seems this broke because of rust-lang/rust#44287.
@jrmuizel
Copy link
Contributor

It would be valuable to have a list breaking changes that's updated whenever something breaking lands.

athre0z pushed a commit to zyantific/zydis-rs that referenced this issue Oct 24, 2017
The regression is filed as rust-lang/rust#45480. It seems this broke because of rust-lang/rust#44287.
@arielb1 arielb1 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 26, 2017
@pietroalbini
Copy link
Member

pietroalbini commented Oct 27, 2017

This also happens when using methods like parse:

fn main() {
    let mut var = 0;
    let other = "10".parse().unwrap();  // Fails on beta/nightly
    var += other;
}

Edit: and also this, which is a bit annoying:

fn main() {
    let mut result = 0;
    result += "10".parse().unwrap();  // Fails on beta/nightly
    println!("{}", result);
}

@Mark-Simulacrum
Copy link
Member

Also occurs in average-0.6.2 cc @vks

@Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum
Copy link
Member

And also whiteread-0.4.3 cc @krdln

@vks
Copy link
Contributor

vks commented Nov 19, 2017 via email

@egilburg
Copy link

egilburg commented Nov 21, 2017

Should as be at all needed in the above example? if x is u64 and y is u32, it seems the only choice is to upcast y to x, it's not something that's ambiguous or could be hand-coded any better. Could the compiler detect that and make just x += y compile without manual upcast, in the same spirit as making x += &y work without manual deref?

Other cases may be more ambiguous (e.g. adding a u64 to a u32) since the u64 value may not fit in the u32, but adding a u32 to a u64 seems to be lossless (sans integer overflow which is already existing issue on any integer math even of the same type). And even in the u32 += u64 case, it already works with the as u32 example (or as _ on Stable) when the u64 had a value too large for u32, silently losing the non-fitting data.


Also, just because the stability guarantees technically allow these types of breaking changes, doesn't necessarily mean it's a great idea to do so. It seems a lot of valid code is broken, and fixing it requires extra verbosity compared to what the compiler was previously able to resolve, which goes against the spirit of the ergonomics initiative.

@lilianmoraru
Copy link

Yep, this looks really bad:

result += "10".parse().unwrap();  // Fails on beta/nightly
println!("{}", result);

@Mark-Simulacrum Mark-Simulacrum added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Dec 5, 2017
@Mark-Simulacrum Mark-Simulacrum added this to the 1.22 milestone Dec 5, 2017
@Mark-Simulacrum
Copy link
Member

@rust-lang/libs: Closing this as we can't revert now as this has leaked onto stable. Ideally we'd probably have type defaults or something to avoid this in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-inference Area: Type inference regression-from-stable-to-stable Performance or correctness regression from one stable version to another. 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

9 participants