Better inlay hints in 'for' loops

[popzxc]

For #5206 (one part of the fix).

This PR refines the logic of spawning an inlay hints in for loops. We only must provide a hint if the following criteria are met:

• User already typed in keyword.
• Type of expression is known and it's not unit.

However: I don't know why, but I was unable to make complete_for_hint test work. Either without or with my changes, I was always getting this test failed because no hint was spawned for the loop variable.

This change works locally, so I would really appreciate an explanation why this test isn't working now and how to fix it.

[SomeoneToIgnore]

I was unable to make complete_for_hint test work

This test uses the IntoIterator trait functionality that is a part of stdlib.
RA does not use stdlib in its tests due to test speed concerns: no real type inference can be done for such cases ergo no inlay hints.

There are existing ways to emulate the functionality you need: https://github.com/rust-analyzer/rust-analyzer/blob/251ef93ac3bbb138a2eedf6090f2f56f1a15d898/crates/hir_ty/src/tests/traits.rs#L164-L201

So I wonder, if you can reuse this somehow.

[popzxc]

Well, after a bit of shallow experimenting (like code below, + several different attempts to play with check function and the fixture contents itself) I still had no success. It seems that the linked test uses TestDb declared in hir_ty crate, and it somehow different from the database used in MockAnalysis.

Sad to admit, but I need more assistance on that matter. ¯\_(ツ)_/¯

    #[test]
    fn complete_for_hint() {
        check(
            r#"
//- /main.rs crate:main deps:core,alloc
fn main() {
    let mut data = Vec::new();
    data.push("foo");
    for i in data {
      //^ &str
      i;
    //^ &str
    }
}

//- /core.rs crate:core
#[prelude_import] use iter::*;
mod iter {
    trait IntoIterator {
        type Item;
    }
}
//- /alloc.rs crate:alloc deps:core
mod collections {
    struct Vec<T> {}
    impl<T> Vec<T> {
        fn new() -> Self { Vec {} }
        fn push(&mut self, t: T) { }
    }
    impl<T> IntoIterator for Vec<T> {
        type Item=T;
    }
}
"#,
        );

[SomeoneToIgnore]

In this case the best bet would be to ping Matklad next week after his vacation.

I might try to take a stab at this later, but cannot guarantee anything now alas.

[popzxc]

Sure, it's not an emergency :)

[SomeoneToIgnore]

I've checked your test a bit, looks like there was an issue with the compilation.
Now we're able to resolve the main type and have proper //^ marks on the places where inlay hints should be present:

    #[test]
    fn complete_for_hint() {
        check(
            r#"
//- /main.rs crate:main deps:core
pub struct Vec<T> {}

impl<T> Vec<T> {
    pub fn new() -> Self { Vec {} }
    pub fn push(&mut self, t: T) {}
}

impl<T> IntoIterator for Vec<T> {
    type Item=T;
}

fn main() {
    let mut data = Vec::new();
      //^^^^^^^^ Vec<&str>
    data.push("foo");
    for i in data {
      let z = i;
        //^ &str
    }
}

The test fails still, it appears to me that it happens due to the trait solving: the actual IntoIterator trait is different (has more associated types and methods that we need to implement) and inlay hints go throughout the whole system to determine the type: what works for type inference tests fails here due to other things that interfere, such as the trait solving mentioned.

I see the following options:

• Attempt to replicate the proper IntoIterator trait somehow, keeping track of the compilation issues (in order for the hints ot appear, there has to be none)
• Screw everything and add another slow test here: https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/rust-analyzer/tests/rust-analyzer/main.rs or nearby. AFAIK, matklad was not that happy for the new tests to appear there, but I think in this particular case it might be ok

[popzxc]

Thanks for that info! Unfortunately, after some experimenting I was still unable to make check function work.

I guess I'll just wait for Matklad to get back from vacation, and implement whatever he'll find appropriate.

[SomeoneToIgnore]

@matklad , any approach you can think of?

I think there should be a test, but the most feasible one seems to be the slow test.

[matklad]

Yeah, I plan to look into this today. The TL;DR is that #5997 (comment) should work, but we probably should fix our testing infra to at least not have two independent testing infras :-)

[SomeoneToIgnore]

Here's somewhat similar demand for the Iterator : https://rust-lang.zulipchat.com/#narrow/stream/185405-t-compiler.2Fwg-rls-2.2E0/topic/.233750

So we might need to come up with some good way to test the iterators' hints.

[matklad]

Didn't get to making core-aware tests easier per se, but I've at least removed duplication between IDE and hir tests: #6123.

After this, I think #5997 (comment) should work.

[popzxc]

Nice! Will check that tomorrow.

[matklad]

Yup, the following test now works:

    #[test]
    fn complete_for_hint() {
        check(
            r#"
//- /main.rs crate:main deps:core
pub struct Vec<T> {}

impl<T> Vec<T> {
    pub fn new() -> Self { Vec {} }
    pub fn push(&mut self, t: T) {}
}

impl<T> IntoIterator for Vec<T> {
    type Item=T;
}

fn main() {
    let mut data = Vec::new();
      //^^^^^^^^ Vec<&str>
    data.push("foo");
    for i in data {
      //^ &str
      let z = i;
        //^ &str
    }
}

//- /core.rs crate:core
#[prelude_import] use iter::*;
mod iter {
    trait IntoIterator {
        type Item;
    }
}
//- /alloc.rs crate:alloc deps:core
mod collections {
    struct Vec<T> {}
    impl<T> Vec<T> {
        fn new() -> Self { Vec {} }
        fn push(&mut self, t: T) { }
    }
    impl<T> IntoIterator for Vec<T> {
        type Item=T;
    }
}
"#,
        );
    }

[popzxc]

Yay, it works!

I also had to remove for_expression test from inlay_hints.rs, because it seems that it relied on the type deduction from the start += increment; statement rather than from the iterator type, which is kind of indirect.

Hopefully, new complete_for_hint can be a (somewhat more correct) substitution for it.

[SomeoneToIgnore]

Small nit: I think we can make it more readable by writing something like

// We *should* display hint only if user provided "in {expr}" and we know the type of expr (and it's not unit).
// Type of expr should be iterable.
return it.in_token().is_none() ||
    it.iterable()
        .and_then(|iterable_expr|sema.type_of_expr(&iterable_expr))
        .map(|iterable_ty| iterable_ty.is_unknown() || iterable_ty.is_unit())
        .unwrap_or(true)

[SomeoneToIgnore]

Thanks for a good motivation to fix our testing infra, I've managed to submit a long stalled PR because of this.
The change looks good to me, you might improve the code readability in the ast::ForExpr(it) => {, but that's minor and not mandatory.

bors d+

[bors]

✌️ popzxc can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

[SomeoneToIgnore]

Interesting, I get the () instantly I finish typing in in such case:

It looks like RA takes Ok(()) as an iterable() the for expr.
As one way to fix, we can check the children of the ForExpr and display nothing, if there's none (or no block expression).
I don't think this is the best way, so not insisting on anything in this PR (you've anyway mentioned that this is a part of the fix).

[popzxc]

Yeah, I thought about this case and was not sure if such a situation should be handled at all. Once in keyword is typed, it becomes hard to guess whether code below is part of expression or not yet. I considered the following situations:

1. User has typed some_iterable() and first thought that it would be some_iterable().map(...), but then realized that for would work better. They return to the beginning of the line and add for i in .... Now we should show the type.
2. User has the function which returns Result<SomeType> and first typed Ok(some_val) and then started writing the loop (as in your example). Logically, we should not show the type.

These examples are roughly equivalent, and the meaning (and expected behavior) is not expressed in the code, it's just in the head of code author. In my opinion, once in keyword is typed, we should not try to predict the future, and just interpret code as usual.

Otherwise we can stuck writing exceptions for heuristics.

[popzxc]

bors r+

[bors]

Build succeeded:

• Rust (ubuntu-latest)
• Rust (windows-latest)
• TypeScript (ubuntu-latest)
• TypeScript (windows-latest)