Skip to content

Add if let, while let and match arm inlay hints #1606

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

Merged
merged 1 commit into from
Jul 29, 2019

Conversation

SomeoneToIgnore
Copy link
Contributor

@SomeoneToIgnore SomeoneToIgnore commented Jul 28, 2019

image

Add more inline hints support.
Looks like while let type inference support is missing currently, so the corresponding hint tests lack the actual results.

I've also could not find a good way to distinguish between a and b pats in the following expressions:
if let Some(Test { a: None, b: y }) = &test {};

In this case we don't need to add a hint for first pat (a: None), since it's matched against the particular enum variant and need a hint for y, since it's a new variable.
But both a and b are BIND_PAT with similar contents, so looks like there's nothing I can check for to find any differences.

I don't display any hints for such cases now, to avoid confusion, but would be nice to know if there's a way to fix this behavior.

@matklad
Copy link
Member

matklad commented Jul 29, 2019

I've also could not find a good way to distinguish between a and b pats in the following expressions:

see #1618

})
.visit(|closure_parameter: ast::LambdaExpr| {
.visit(|closure_parameter: LambdaExpr| {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps, instead of visiting every construction where a pattern might appear, we should just visit BindPat directly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure which approach will be better though!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to get a PatKind of a parent node from the BindPat?
I know how to get the SyntaxKind, but I need more to determine whether to add a hint or not based on the data about the Pat: for instance, for let bindings that have an ascribed type we don't display a type hint.

Otherwise it seems like a nice idea, seems to be of more or less the same complexity to the current implementation.
But I think I've covered all the cases we need the type hints for, haven't I?
If so, it might be ok to leave the code as is and refactor it after the #1618 is done, since it will help a lot with detecting the leaf BindPats.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to get a PatKind of a parent node from the BindPat

pat.syntax().parent().and_then(ast::Pat::cast).map(|it| it.kind())

If so, it might be ok to leave the code as is and refactor it after the #1618 is done, since it will help a lot with detecting the leaf BindPats.

Makes sense!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense!

@matklad , the time had come to continue the discussion :)
I'm trying to apply the

Perhaps, instead of visiting every construction where a pattern might appear, we should just visit BindPat directly?

suggestion as

ast::BindPat(it) => {
    let pat = dbg!(dbg!(it).pat())?;
    let ty = dbg!(analyzer.type_of_pat(db, &pat))?;
    if ty.is_unknown() {
        return None;
    }

    acc.push(
        InlayHint {
            range: pat.syntax().text_range(),
            kind: InlayKind::TypeHint,
            label: ty.display_truncated(db, max_inlay_hint_length).to_string().into(),
        }
    );
},

yet in every test the pat is None:

[crates/ra_ide/src/inlay_hints.rs:94] it = BindPat {
    syntax: BIND_PAT@[69; 71)
      NAME@[69; 71)
        IDENT@[69; 71) "zz"
    ,
}
[crates/ra_ide/src/inlay_hints.rs:94] dbg!(it).pat() = None
[crates/ra_ide/src/inlay_hints.rs:94] it = BindPat {
    syntax: BIND_PAT@[105; 111)
      NAME@[105; 111)
        IDENT@[105; 111) "zz_ref"
    ,
}
[crates/ra_ide/src/inlay_hints.rs:94] dbg!(it).pat() = None

Is there something that I'm missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, looks like let pat = ast::Pat::from(it); should be used instead of let pat =it.pat()?;
What does the latter method return then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It returns the pattern in name @ pattern syntax.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, interesting usecase for the type hints actually, thank you.

@SomeoneToIgnore
Copy link
Contributor Author

By the way, I start to feel like specifying all sorts of different patters in InlayKind is a mistake.
Should we instead merge all current kinds into the TypeHint or something?

@matklad
Copy link
Member

matklad commented Jul 29, 2019

I am fine with merging them, yes. It wouldn't be hard to add it back, if we want to, for example, filter them on the client

@matklad
Copy link
Member

matklad commented Jul 29, 2019

bors r+

bors bot added a commit that referenced this pull request Jul 29, 2019
1606: Add `if let`, `while let` and match arm inlay hints r=matklad a=SomeoneToIgnore

<img width="693" alt="image" src="https://user-images.githubusercontent.com/2690773/62013363-152f1d80-b19a-11e9-90ea-07568757baa2.png">

Add more inline hints support.
Looks like `while let` type inference support is missing currently, so the corresponding hint tests lack the actual results.

I've also could not find a good way to distinguish between `a` and `b` pats in the following expressions:
`if let Some(Test { a: None, b: y }) = &test {};`

In this case we don't need to add a hint for first pat (`a: None`), since it's matched against the particular enum variant and need a hint for `y`, since it's a new variable.
But both `a` and `b` are `BIND_PAT` with similar contents, so looks like there's nothing I can check for to find any differences.

I don't display any hints for such cases now, to avoid confusion, but would be nice to know if there's a way to fix this behavior.

Co-authored-by: Kirill Bulatov <[email protected]>
@bors
Copy link
Contributor

bors bot commented Jul 29, 2019

Build succeeded

@bors bors bot merged commit b8f95f4 into rust-lang:master Jul 29, 2019
@SomeoneToIgnore SomeoneToIgnore deleted the more-inlay-hints branch July 29, 2019 12:42
bors bot added a commit that referenced this pull request Aug 6, 2019
1652: Improve type hints behavior r=matklad a=SomeoneToIgnore

This PR fixed the following type hints issues:

* Restructures the `InlayKind` enum contents based on the discussion here: #1606 (comment)
* Races described in #1639 
* Caches the latest decorations received for each file to show them the next time the file is opened (instead of a new server request)

Co-authored-by: Kirill Bulatov <[email protected]>
bors bot added a commit that referenced this pull request Feb 23, 2020
3278: Show more inlay hints r=matklad a=SomeoneToIgnore

Closes #3273

As suggested in #1606 (comment) , there is a simpler way to handle inlay hints after #1618 is closed.

This PR uses the approach suggested (which results in more type hints for various bindings) and also adds more name hints for function parameters.

Examples can be found in the issue:
* #3273 (comment)
* #3273 (comment)

Co-authored-by: Kirill Bulatov <[email protected]>
matklad pushed a commit to matklad/vscode-rust that referenced this pull request Jul 13, 2020
1652: Improve type hints behavior r=matklad a=SomeoneToIgnore

This PR fixed the following type hints issues:

* Restructures the `InlayKind` enum contents based on the discussion here: rust-lang/rust-analyzer#1606 (comment)
* Races described in #1639 
* Caches the latest decorations received for each file to show them the next time the file is opened (instead of a new server request)

Co-authored-by: Kirill Bulatov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants