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 with a..b is not as good as with range(a, b) #21672

Closed
japaric opened this issue Jan 26, 2015 · 12 comments · Fixed by #21806
Closed

Type inference with a..b is not as good as with range(a, b) #21672

japaric opened this issue Jan 26, 2015 · 12 comments · Fixed by #21806
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-typesystem Area: The type system

Comments

@japaric
Copy link
Member

japaric commented Jan 26, 2015

STR

fn main() {
    (0..8).map(|i| i == 1u64).collect::<Vec<_>>();
    //~^ error: the type of this value must be known in this context
    (0..8).map(|i| 1u64 == i).collect::<Vec<_>>();
    //~^ error: type mismatch resolving `<core::ops::Range<i32> as core::iter::Iterator>::Item == u64`
    range(0, 8).map(|i| i == 1u64).collect::<Vec<_>>();  // OK
    range(0, 8).map(|i| 1u64 == i).collect::<Vec<_>>();  // OK
}

Version

rustc 1.0.0-nightly (458a6a2f6 2015-01-25 21:20:37 +0000)

cc @nick29581

@japaric
Copy link
Member Author

japaric commented Jan 26, 2015

@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?

@japaric
Copy link
Member Author

japaric commented Jan 26, 2015

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

@kmcallister kmcallister added A-diagnostics Area: Messages for errors, warnings, and lints A-typesystem Area: The type system labels Jan 28, 2015
@edwardw
Copy link
Contributor

edwardw commented Jan 28, 2015

This must be duo to #20189. It seems to enforce the numeric fallback rule a bit too eagerly.

@edwardw
Copy link
Contributor

edwardw commented Jan 28, 2015

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.

@bluss
Copy link
Member

bluss commented Jan 28, 2015

edwardw, not just that, type inference regressed slightly at the time of the range stabilization PR.

@ghost
Copy link

ghost commented Jan 28, 2015

also related #21595

bors added a commit that referenced this issue Jan 29, 2015
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
bors added a commit that referenced this issue Jan 29, 2015
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
@edwardw
Copy link
Contributor

edwardw commented Jan 30, 2015

Have a partial fix by deferring numeric fallback, but it seems to hit #20297 now.

@nikomatsakis
Copy link
Contributor

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:

  1. <Range<_#0i> as Iterator>::Item == u8

However, we are unable to make the connection between u8 and _#0i. The reason is that the new ::ops::Range has separate impls for each of the numeric types, whereas the old Range had only impl for a type A:Int. When we process the projections, we first try to resolve Range<_#0i> as Iterator (without considering the output type Item), and we get ambiguity, because there are many impls that could apply.

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 Range. Probably the best fix is to go back to that, actually, but this is an interesting quandry. I'll have to think about it a bit.

@nikomatsakis
Copy link
Contributor

To be clear, the example I was digging into was let xs: Vec<u8> = (0..10).collect()

@nikomatsakis
Copy link
Contributor

Also, to put it another way: having a single blanket impl allows us to draw the connection that when you iterate over Range<T>, you get Item=T. With the current setup of many distinct impls, that is not known to be true.

@nikomatsakis
Copy link
Contributor

After some discussion with @aturon on RFC, I just wanted to clarify a few things (I myself was a touch confused too).

  1. Changing the impl is probably the easiest way to fix this in the short term.
  2. However, to some extent, this limitation is an artifact of how trait matching is implemented. That is, we intentionally do not take output bindings into account when selecting traits. However, it doesn't have to be this way, but it'd take some work to massage the trait matching infrastructure, since impl selection doesn't presently have access to output binding information, that'd have to be propagated inward. It might be interesting to try and address this in a more wholistic way (i.e., something that accounts for the full set of facts that are registered).
  3. In particular, the important constraint that we MUST maintain is that, during trans, given the input types, we can derive the output type. But it's permissible for us to use the output type to derive the input type during type checking, since once inference is done, we then have the input types to supply during trans so that we can rederive the output.

@edwardw
Copy link
Contributor

edwardw commented Jan 31, 2015

The fix is very simple then. Thanks @nikomatsakis for helping diagnose the problem and pointing to the right direction.

edwardw added a commit to edwardw/rust that referenced this issue Feb 1, 2015
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
bors added a commit that referenced this issue Feb 1, 2015
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-typesystem Area: The type system
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants