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 requires type of LHS to be known for binary operators #8280

Closed
mihneadb opened this issue Aug 4, 2013 · 12 comments
Closed

Type inference requires type of LHS to be known for binary operators #8280

mihneadb opened this issue Aug 4, 2013 · 12 comments
Labels
A-inference Area: Type inference C-enhancement Category: An issue proposing an enhancement or a PR with one.

Comments

@mihneadb
Copy link
Contributor

mihneadb commented Aug 4, 2013

This is the code:

use std::hashmap;

fn main() {
    let mut map = hashmap::HashMap::new();

    let str = "Hello, world!";

    for &char in str.to_ascii().iter() {
        let count = map.find(&char);
        match count {
            Some(&x) => map.insert(char, x + 1),
            None => map.insert(char, 1)
        }
    }

    println!("{:?}", map);
}

Error message:

count_chars.rs:11:41: 11:42 error: the type of this value must be known in this context
count_chars.rs:11             Some(&x) => map.insert(char, x + 1),
                                                       ^
error: aborting due to previous error
@huonw
Copy link
Member

huonw commented Aug 4, 2013

Either

-            Some(&x) => map.insert(char, x + 1),
+            Some(&x) => map.insert(char, 1 + x),

or

-            Some(&x) => map.insert(char, x + 1),
-            None => map.insert(char, 1)
+            None => map.insert(char, 1),
+            Some(&x) => map.insert(char, x + 1),

makes the inference work:

  • in the first case, the 1 in 1 + x defaults to int and so the second argument (i.e. x) is inferred to also be int
  • in the second, the plain insert comes first and so that 1 there defaults to int and the inference succeeds.

(There are various ("correct") type errors and mutability issues that stop it compiling from after this.)

It seems like the original code should be legal, given the trivial adjustments (especially reordering the arms).

(cc @nikomatsakis)

@huonw
Copy link
Member

huonw commented Nov 10, 2013

Triage: I've updated the code for language changes; the inference is still suboptimal (i.e. it fails), the fixes in the comment above still address the inference issue, but also still hit the borrowing errors.

The following code works fine:

use std::hashmap;

fn main() {
    let mut map = hashmap::HashMap::new();

    let str = "Hello, world!";

    for &char in str.to_ascii().iter() {
        map.insert_or_update_with(char, 1, |_, v| *v += 1);
    }

    println!("{:?}", map);
}

@nikomatsakis
Copy link
Contributor

Reading this example more closely, I actually don't think inference is expected to succeed here. You haven't told it the type of the values in the map, but you attempting to do x + 1. Due to the possibility of overloaded operators, we (at least currently) require that the type of x be known at that point. Reordering the arms makes type inference succeed because it forces the type of x to be int. Similarly, using insert_or_update_with also supplies the initial value (1), giving us the information we needed. I'm going to close.

@nikomatsakis
Copy link
Contributor

That said -- we currently treat operator overloading as an operator, but it's possible that with the changes in #10337 we could avoid the requirement that we know the type of x. I'm going to tentatively re-open as a "sub-bug" of #10337.

@nikomatsakis
Copy link
Contributor

Update title.

@Aatch
Copy link
Contributor

Aatch commented Jan 20, 2015

Multidispatch should have fixed this, correct?

@nikomatsakis
Copy link
Contributor

@Aatch It did not. The distinction has to do with needing to decide between "builtin" operators like + on integers and trait-driven operations. Currently we do this at the moment of type-checking the + (or other operator). It'll take some rearchitecting but we could defer this decision.

@arielb1
Copy link
Contributor

arielb1 commented Jun 21, 2015

This does compile (rather, type-check - of course it won't borrow-check) at 1.0 and above.

@arielb1 arielb1 added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Jun 21, 2015
@wthrowe
Copy link
Contributor

wthrowe commented Jun 21, 2015

The following does not work, however:

fn main() {
    0u8.into() == 0u8;
}

It gives: "error: unable to infer enough type information about _; type annotations or generic parameter binding required" (on the into())

I'd been assuming that was this issue since it works if you switch the order of the operands, but if this one is solved perhaps it is a new one?

@steveklabnik
Copy link
Member

Triage: @arielb1 said that this compiles, but it at least needs some massaging due to changes. I tried to do some of them, but it wasn't totally clear to me what should be done, exactly.

It's not clear to me if @wthrowe 's example is the same thing or different.

@Mark-Simulacrum Mark-Simulacrum added A-inference Area: Type inference C-enhancement Category: An issue proposing an enhancement or a PR with one. and removed A-typesystem Area: The type system E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. labels Jul 19, 2017
@bstrie
Copy link
Contributor

bstrie commented Dec 2, 2017

My own attempt to concoct a modern reproduction no longer exhibits this bug:

    let mut v = Vec::new();
    
    match v.get(0) {
        Some(&x) => v.push(x + 1),  // A-OK
        None => v.push(1),
    }

In fact, our inference is so good these days that even this works:

    let mut v = Vec::new();
    
    match v.get(0) {
        Some(&x) => v.push(x + x),
        None => v.push(1),
    }

Given that this bug was filed so long before 1.0 that the test case no longer works and that nobody seems to have come up with a reproduction of the original behavior, I'm closing this.

@bstrie bstrie closed this as completed Dec 2, 2017
@arielb1
Copy link
Contributor

arielb1 commented Dec 3, 2017

So autoref for operators will make this stop working for the normal reasons. I suppose that would be a regression. I hope it won't be too terribly annoying.

flip1995 pushed a commit to flip1995/rust that referenced this issue Jan 27, 2022
Add `msrv` config for `map_clone`

Just a small PR to have some fun with Clippy and to clear my head a bit 😅

---

changelog: [`map_clone`]: The suggestion takes `msrv` into account
changelog: Track `msrv` attribute for `manual_bits` and `borrow_as_prt`

fixes: rust-lang#8276
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-inference Area: Type inference C-enhancement Category: An issue proposing an enhancement or a PR with one.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants