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

Unable to inference a result type of an indexing operation #2534

Closed
ztlpn opened this issue Dec 12, 2019 · 18 comments · Fixed by #2592 or #5319
Closed

Unable to inference a result type of an indexing operation #2534

ztlpn opened this issue Dec 12, 2019 · 18 comments · Fixed by #2592 or #5319
Labels
A-ty type system / type inference / traits / method resolution

Comments

@ztlpn
Copy link
Contributor

ztlpn commented Dec 12, 2019

Hi everyone, I was under impression that type inference involving arrays should work, but it is behaving weirdly:

fn foo(bar: [usize; 2]) -> usize {
    let baz = bar[1];
    baz
}

fn foo2(bar: [usize; 2]) -> usize {
    let baz = bar[1];
    return baz;
}

fn foo3(bar: [usize; 2]) {
    let baz = bar[1];
    println!("{}", baz);
}

fn main() {
    let foo4 = |bar: [usize; 2]| -> usize {
        let baz = bar[1];
        baz
    };

    let foo5 = |bar: [usize; 2]| -> usize {
        let baz = bar[1];
        return baz;
    };

    println!(
        "{} {} {} {}",
        foo([1, 2]),
        foo2([1, 2]),
        foo4([1, 2]),
        foo5([1, 2])
    );
    foo3([1, 2]);
}

The type inferred for baz is usize for foo1 and foo2 (good), {unknown} for foo3 and foo4 (I would expect that type inference is not too hard there) and () for foo5 (plain wrong!).

Tested with the latest master (4444192)

Edit: there are two separate issues: type in foo3 and foo4 is not inferred because it is an indexing operation and is inferred incorrectly in foo5 because of the return statement in the closure (#2547 )

@flodiebold
Copy link
Member

These are not related to arrays -- it's indexing expressions which aren't implemented yet. I think there's nothing stopping us from doing that, just no-one has done it yet.

So all these indexing expressions get inferred as unknown; in cases foo1, foo2, foo4 and foo5 we then infer the type of baz from the later usage, except in foo5 we get it wrong because we don't handle return in closures correctly (we use the enclosing function's return type), which is another bug.

@ztlpn
Copy link
Contributor Author

ztlpn commented Dec 12, 2019

@flodiebold Oh, you are right - looks like I didn't compress the test case enough, the simplest case would be

fn main() {
    let foo = || {
        return 1;
    };
}

(foo gets resolved to || -> ()). Also, looks like explicit return type (let foo = || -> usize {...) in a closure gets ignored. Is this a known issue?

@flodiebold
Copy link
Member

Yeah, that's the second thing I wrote -- when handling return, we don't handle closures and go straight to the surrounding function. It's a known issue to me, but we don't have an issue for it here yet 🙂

@ztlpn ztlpn changed the title Wrong type inference involving arrays Unable to inference a result type of an indexing operation Dec 12, 2019
@ztlpn
Copy link
Contributor Author

ztlpn commented Dec 12, 2019

Ok, I've split the issue into two so that there is some place to subscribe to and track progress.

@oxalica
Copy link
Contributor

oxalica commented Dec 20, 2019

It seems not fixed yet.

@matklad matklad reopened this Dec 20, 2019
@flodiebold flodiebold added the A-ty type system / type inference / traits / method resolution label Dec 26, 2019
@flodiebold
Copy link
Member

flodiebold commented Dec 26, 2019

I think the main remaining issue is performing autoderef for indexing operations. We might also need to apply unsized coercions in the case of arrays, I haven't checked.

@flodiebold
Copy link
Member

flodiebold commented Feb 14, 2020

I now checked and we do need to perform unsized coercions.

Also, there's an issue with type variable fallback in the common case of some_array[1], because Chalk doesn't know that the only impl that can apply is the one for usize (but rustc is able to make this inference). Also also, this won't work in many cases until rust-lang/chalk#234 is fixed.

So to summarize, to get inference for array[0] to work, we need to:

How complicated such a simple expression can be 😄

@flodiebold
Copy link
Member

Autoref and unsizing are done now.

@flodiebold
Copy link
Member

The only remaining problem here is handling integer variables so that arr[0] works -- arr[0usize] should work right now. For that, there are no immediate plans yet though.

@PandarinDev
Copy link

The only remaining problem here is handling integer variables so that arr[0] works -- arr[0usize] should work right now. For that, there are no immediate plans yet though.

Just tested this and arr[0usize] works perfectly, thanks! Can you please give a short explanation on what's blocking implementation of type inference when using integer indices?

@flodiebold
Copy link
Member

See the linked Chalk issue rust-lang/chalk#327.

@PandarinDev
Copy link

I see, thank you for linking the related Chalk issue.

@AzureMarker
Copy link
Member

Even after that Chalk issue was solved, foo3 still is {unknown}. Is this expected? All of the other cases work.

@flodiebold
Copy link
Member

We're not making use of the Chalk support yet.

@stanciuadrian
Copy link
Contributor

While browsing Blandy & Orendorff I found more test cases:

    let a1 = String::new();
    let b1 = &a1[1..];
    let c1 = &a1[..];
    let d1 = &a1[..2];
    let e1 = &a1[1..2];

    let a2 = "";
    let b2 = &a2[1..];
    let c2 = &a2[..];
    let d2 = &a2[..2];
    let e2 = &a2[1..2];

    let a3 = [1, 2, 3];
    let b3 = &a3[1..];
    let c3 = &a3[..];
    let d3 = &a3[..2];
    let e3 = &a3[1..2];

@lnicola
Copy link
Member

lnicola commented Jun 26, 2020

I suppose we should use the new VariableKind::IntegerTy, possibly by adding a kind field to Canonical<T>?

@flodiebold
Copy link
Member

flodiebold commented Jun 26, 2020

@lnicola that's basically it, yeah. I actually started this already but didn't finish yet, IIRC it caused some more necessary changes. (You need a kind per variable in the Canonical, so you need to copy around and specify these variable kinds in lots of places, and so on...)

flodiebold added a commit to flodiebold/rust-analyzer that referenced this issue Jun 30, 2020
This means we need to keep track of the kinds (general/int/float) of variables
in `Canonical`, which requires some more ceremony. (It also exposes some places
where we're not really dealing with canonicalization correctly -- another thing
to be cleaned up when we switch to using Chalk's types directly.)

Should fix the last remaining issue of rust-lang#2534.
flodiebold added a commit to flodiebold/rust-analyzer that referenced this issue Jul 1, 2020
This means we need to keep track of the kinds (general/int/float) of variables
in `Canonical`, which requires some more ceremony. (It also exposes some places
where we're not really dealing with canonicalization correctly -- another thing
to be cleaned up when we switch to using Chalk's types directly.)

Should fix the last remaining issue of rust-lang#2534.
bors bot added a commit that referenced this issue Jul 1, 2020
5149: Implement Chalk variable kinds r=flodiebold a=flodiebold

This means we need to keep track of the kinds (general/int/float) of variables in `Canonical`, which requires some more ceremony. (It also exposes some places where we're not really dealing with canonicalization correctly -- another thing to be cleaned up when we switch to using Chalk's types directly.)

Should fix the last remaining issue of #2534.

Co-authored-by: Florian Diebold <[email protected]>
flodiebold added a commit to flodiebold/rust-analyzer that referenced this issue Jul 1, 2020
This means we need to keep track of the kinds (general/int/float) of variables
in `Canonical`, which requires some more ceremony. (It also exposes some places
where we're not really dealing with canonicalization correctly -- another thing
to be cleaned up when we switch to using Chalk's types directly.)

Should fix the last remaining issue of rust-lang#2534.
bors bot added a commit that referenced this issue Jul 1, 2020
5149: Implement Chalk variable kinds r=flodiebold a=flodiebold

This means we need to keep track of the kinds (general/int/float) of variables in `Canonical`, which requires some more ceremony. (It also exposes some places where we're not really dealing with canonicalization correctly -- another thing to be cleaned up when we switch to using Chalk's types directly.)

Should fix the last remaining issue of #2534.

Co-authored-by: Florian Diebold <[email protected]>
@flodiebold
Copy link
Member

Now this is blocked on a working version of rust-lang/chalk#555, and then we'll still need to adapt the logic in RA to search for impls for integer types when the self type is an integer variable (and same for floats).

@bors bors bot closed this as completed in 39e049d Jul 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ty type system / type inference / traits / method resolution
Projects
None yet
8 participants