-
Notifications
You must be signed in to change notification settings - Fork 13k
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 with a..b
is not as good as with range(a, b)
#21672
Comments
@nikomatsakis I think you have a PR that improves type inference in some cases where closures are involved (?), do you know if by chance that fixes this as well? |
This is not closure specific: let _: Vec<u8> = (0..10).collect();
//~^ error: type mismatch resolving `<core::ops::Range<i32> as core::iter::Iterator>::Item == u8`
let _: Vec<u8> = range(0, 10).collect(); // OK |
This must be duo to #20189. It seems to enforce the numeric fallback rule a bit too eagerly. |
Another example from IRC is that for x in 0..10 { println!("{}", x); } type checked while for x in 0..10 { println!("{}", x % 2); } didn't. Rather interesting. |
edwardw, not just that, type inference regressed slightly at the time of the range stabilization PR. |
also related #21595 |
Note: Do not merge until we get a newer snapshot that includes #21374 There was some type inference fallout (see 4th commit) because type inference with `a..b` is not as good as with `range(a, b)` (see #21672). r? @alexcrichton
Note: Do not merge until we get a newer snapshot that includes #21374 There was some type inference fallout (see 4th commit) because type inference with `a..b` is not as good as with `range(a, b)` (see #21672). r? @alexcrichton
Have a partial fix by deferring numeric fallback, but it seems to hit #20297 now. |
OK, so I spent some time digging into this. I don't think that deferring numeric fallback is necessarily the solution. @edwardw sorry if I kept disappearing on IRC, I had to put my head down and study the log to see what was happening. So what seems to happen is that we correctly come up with the constraint:
However, we are unable to make the connection between Unfortunately this all seems a bit right to me. Basically we're trying to infer the type of the range from the output type, which is the reverse direction inference is supposed to go. When we used a blanket impl, we were able to resolve this because there was only one impl for |
To be clear, the example I was digging into was |
Also, to put it another way: having a single blanket impl allows us to draw the connection that when you iterate over |
After some discussion with @aturon on RFC, I just wanted to clarify a few things (I myself was a touch confused too).
|
The fix is very simple then. Thanks @nikomatsakis for helping diagnose the problem and pointing to the right direction. |
The new `::ops::Range` has separated implementations for each of the numeric types, while the old `::iter::Range` has one for type `Int`. However, we do not take output bindings into account when selecting traits. So it confuses `typeck` and makes the new range does not work as good as the old one when it comes to type inference. This patch implements `Iterator` for the new range for one type `Int`. This limitation could be lifted, however, if we ever reconsider the output types' role in type inference. Closes rust-lang#21595 Closes rust-lang#21649 Closes rust-lang#21672
The new `::ops::Range` has separated implementations for each of the numeric types, while the old `::iter::Range` has one for type `Int`. However, we do not take output bindings into account when selecting traits. So it confuses `typeck` and makes the new range does not work as good as the old one when it comes to type inference. This patch implements `Iterator` for the new range for one type `Int`. This limitation could be lifted, however, if we ever reconsider the output types' role in type inference. Closes #21595 Closes #21649 Closes #21672
STR
Version
cc @nick29581
The text was updated successfully, but these errors were encountered: