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: Add inlay parameter hints #1326

Merged
merged 1 commit into from Jul 27, 2017

Conversation

Projects
None yet
7 participants
@farodin91
Copy link
Contributor

farodin91 commented Jun 9, 2017

Fixes #1141

@farodin91

This comment has been minimized.

Copy link
Contributor Author

farodin91 commented Jun 9, 2017

@farodin91 farodin91 force-pushed the farodin91:inlay-hints-provider branch from cc752d9 to 06192ae Jun 9, 2017

@vasily-kirichenko

This comment has been minimized.

Copy link
Contributor

vasily-kirichenko commented Jun 9, 2017

wow. let me try!

@vasily-kirichenko

This comment has been minimized.

Copy link
Contributor

vasily-kirichenko commented Jun 9, 2017

Pure beauty :)

1

@vasily-kirichenko

This comment has been minimized.

Copy link
Contributor

vasily-kirichenko commented Jun 9, 2017

@farodin91 I found quite a lot of bugs :)

image

@farodin91

This comment has been minimized.

Copy link
Contributor Author

farodin91 commented Jun 9, 2017

I don't show type if i can't infer a type.

@farodin91 farodin91 force-pushed the farodin91:inlay-hints-provider branch from 06192ae to e65284c Jun 9, 2017

@farodin91

This comment has been minimized.

Copy link
Contributor Author

farodin91 commented Jun 9, 2017

Adding tests and Options

@vasily-kirichenko

This comment has been minimized.

Copy link
Contributor

vasily-kirichenko commented Jun 9, 2017

But isn't 1 + 1 has i32 type?..

@Undin

This comment has been minimized.

Copy link
Member

Undin commented Jun 9, 2017

Not now:)
At this moment plugin can't infer result of arithmetic operations for primitive types. Their implementations are created with macros but, unfortunately, we can't expand macros yet.

@farodin91 farodin91 force-pushed the farodin91:inlay-hints-provider branch from e65284c to 4ae1a78 Jun 9, 2017

@vasily-kirichenko

This comment has been minimized.

Copy link
Contributor

vasily-kirichenko commented Jun 9, 2017

image

Cool :) But generic parameters are not inferred.

@farodin91

This comment has been minimized.

Copy link
Contributor Author

farodin91 commented Jun 9, 2017

If you set the parameter it should infer parameters. For example let x:HashMap<String, String> after this is should infer.

@farodin91

This comment has been minimized.

Copy link
Contributor Author

farodin91 commented Jun 11, 2017

@alygin Could you review?

@alygin
Copy link
Contributor

alygin left a comment

My main concern here is optionality of this feature. I think it must be possible to switch it off separately for the argument names and for the inferred types.

IDEA has a special setting for it: "Editor -> General -> Appearance -> Show parameter name hints" with the "Configure..." button for extended settings, including per-language options. Unfortunately, other products (I looked in CLion) still don't have such block in the Preferences, so we need to add our own settings, probably to the "Langs & Frameworks -> Rust" panel (RustProjectSettingsPanel).

PS: I'm on vacation now, so sorry for possible delays in my answers :)

val patBinding = ident.patBinding
val type = inferDeclarationType(patBinding)
if (type is TyUnknown || type is TyUnit) return emptyList()
return listOf(InlayInfo(": " + type.toString(), patBinding.textRange.endOffset))

This comment has been minimized.

@alygin

alygin Jun 12, 2017

Contributor

": " + type.toString() could be just ": $type"

get() = reference.resolve() as? RsFunction

enum class HintType(desc: String, enabled: Boolean) {
LET_BINDING_HIST("Show local variable type hints", true) {

This comment has been minimized.

@alygin

alygin Jun 12, 2017

Contributor

Looks like a typo. Did you mean LET_BINDING_HINT?

fun transform(index: Int, valueParameter: RsValueParameter, valueArgumentList: RsValueArgumentList): InlayInfo? {
if (index >= valueArgumentList.exprList.size)
return null
return InlayInfo(valueParameter.pat?.text + ":", valueArgumentList.exprList[index].textRange.startOffset)

This comment has been minimized.

@alygin

alygin Jun 12, 2017

Contributor

You also need to return null here if valueParameter.pat? is null, to handle anonymous arguments properly. Here's a snippet to test this case:

trait Foo {
    fn foo(&self, u32);
    fn bar(&self) {
        self.foo(10);    // <-- shouldn't add `null: `
    }
}
@kumbayo

This comment has been minimized.

Copy link
Contributor

kumbayo commented Jun 12, 2017

When calling a method using UFCS syntax, the labels are shifted by 1 for the extra self argument.

struct S;

impl S {
    fn foo_self(self, param1: u32) {}
    fn foo_self_ref(&self, param1: u32) {}
    fn foo_self_mut_ref(&mut self, param1: u32) {}
}

fn test() {
    let mut b = S;

    b.foo_self_ref(1);
    b.foo_self_mut_ref(1);
    b.foo_self(1);

    S::foo_self_ref(&b, 1); //bad
    S::foo_self_mut_ref(&mut b, 1); //bad
    S::foo_self(b, 1); //bad
}

inlay_parameter_hints_ufcs

@farodin91

This comment has been minimized.

Copy link
Contributor Author

farodin91 commented Jun 12, 2017

@alygin For IDEA you can de/select a every single feature. Any idea to integrate it into rust settings panel and in the configure area? At the same time.

@farodin91 farodin91 force-pushed the farodin91:inlay-hints-provider branch from 4ae1a78 to e97fdee Jun 13, 2017

val ident = element.pat as? RsPatIdent ?: return emptyList()
val patBinding = ident.patBinding
val type = inferDeclarationType(patBinding)
if (type is TyUnknown || type is TyUnit) return emptyList()

This comment has been minimized.

@kumbayo

kumbayo Jun 15, 2017

Contributor

Is there a reason / use case why you do not want to show the unit type on local variables?
Noise during typing new code?

I think it might be useful to show it for situations like.

let z = accidentally_returns_nothing();

fn accidentally_returns_nothing() -> () {
    ;
}
import com.intellij.psi.PsiElement
import org.rust.lang.core.psi.*
import org.rust.lang.core.psi.ext.RsCompositeElement
import org.rust.lang.core.psi.ext.valueParameters

This comment has been minimized.

@kumbayo

kumbayo Jun 15, 2017

Contributor

IntelliJ claims this import is unused.

@farodin91 farodin91 force-pushed the farodin91:inlay-hints-provider branch from e97fdee to 25b4588 Jun 18, 2017

@matklad

This comment has been minimized.

Copy link
Member

matklad commented Jun 18, 2017

Hm, compilation fails on 2016.3... I need to check if we can make extensions in plugin.xml dependent on the version of IDE...

@farodin91

This comment has been minimized.

Copy link
Contributor Author

farodin91 commented Jun 18, 2017

But my implementation of settings shouldn't work in any case for 2016.3.

@kumbayo

This comment has been minimized.

Copy link
Contributor

kumbayo commented Jun 20, 2017

It seems the S::foo_self_ref(&b, 1); case has regressed.
In an intermediate version it already worked.
But now it is not working anymore and the test for it is gone again.

@farodin91

This comment has been minimized.

Copy link
Contributor Author

farodin91 commented Jun 21, 2017

@kumbayo Thank i pushed something wrong.

@farodin91 farodin91 force-pushed the farodin91:inlay-hints-provider branch 3 times, most recently from b5ef4da to 7c791bd Jun 21, 2017

@farodin91

This comment has been minimized.

Copy link
Contributor Author

farodin91 commented Jun 28, 2017

@matklad Any progress to fix compilation fails on 2016.3?

@matklad

This comment has been minimized.

Copy link
Member

matklad commented Jun 29, 2017

@farodin91 looks like the easiest solution is just to wait while we drop support for 2016.3. This'll happen once 2017.2 is out, which is pretty soon.

@vlad20012

This comment has been minimized.

Copy link
Member

vlad20012 commented Jul 18, 2017

2017.2 just released

@farodin91

This comment has been minimized.

Copy link
Contributor Author

farodin91 commented Jul 18, 2017

I will rebase after #1487 is merged.

@farodin91 farodin91 force-pushed the farodin91:inlay-hints-provider branch from 7c791bd to f7623b5 Jul 19, 2017

@farodin91

This comment has been minimized.

Copy link
Contributor Author

farodin91 commented Jul 19, 2017

Resolved

@farodin91

This comment has been minimized.

Copy link
Contributor Author

farodin91 commented Jul 27, 2017

@matklad Do you like to merge it?

@matklad

This comment has been minimized.

Copy link
Member

matklad commented Jul 27, 2017

@farodin91 sure!

bors r+

bors bot added a commit that referenced this pull request Jul 27, 2017

Merge #1326
1326: TYPE: Add inlay parameter hints r=matklad

Fixes #1141

@bors bors bot merged commit f7623b5 into intellij-rust:master Jul 27, 2017

2 checks passed

bors Build succeeded
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Jul 27, 2017

@alygin alygin referenced this pull request Jul 28, 2017

Closed

More intelligent hints #1524

@farodin91 farodin91 deleted the farodin91:inlay-hints-provider branch Jul 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.