Show inferred types as inline code

[gaaclarke]

I've been writing some Rust code and I've appreciated the inline declarations of inferred types. It makes reading and working with code much easier. This is apropos Dart where some repositories enforce the explicit declaration of types whereas others don't.

Here is an example of the Rust extension showing inferred types (and parameter names but this issue isn't asking for that):

: &Option<Rc<Node<i32>>> is the inferred type of result. It doesn't exist in the file but it would be valid code had it been written there. The inline hint is itself valid code, which is important.

So in Dart's case it would look like this:

final x = add(1, 2);

becomes:

final int x = add(1, 2);

Where int doesn't exist in the code but is displayed as above as if it was.

[DanTup]

I thought we already had an issue for this, but can't find it (perhaps remembering #3609 which is for parameter names).

Started on this, right now it's just the labels:

But I've filed #4156 to see whether we can support showing the type dartdocs on mouseover too.

The change is in the analysis server so will ship as part of an SDK.

[gaaclarke]

That looks awesome. The only question I have is if we should show var int x1. In all the other cases the hint is valid code. I went back and looked a bit into what Rust was doing with theirs and not all of their hints are valid code either. I wonder if users would find that case annoying. My personal take is that its use outweighs the slight oddity.

[DanTup]

The only question I have is if we should show var int x1.

I think ideally we should replace var with int, but we're not able to do that. LSP only allows us to provide a Position and not a Range. I don't think not being able to replace it should avoid us showing the type though (the purpose of this feature is to let the user see types that aren't explicitly in the code), so I think this is all we have.

I do generally think we should try to stick to "valid code" where possible though, just not at the expense of not including a type.

[DanTup]

This has landed in the SDK repo in dart-lang/sdk@8e2dbc1 / dart-lang/sdk@ba47931.

[jakemac53]

Is this an option we will be able to turn off? Basically for me it's violating effective dart by putting types on the left that shouldn't be there... so that would be quite triggering if I can't turn it off. It adds a lot of noise in the screenshot above that isn't providing much value for me personally. I agree there are plenty of people who like types on the left though, and would like to be able to turn it on.

[jacob314]

This feature probably makes sense to have off by default. We should definitely promote it though as there will be a fraction of users who will love it.
IntelliJ has supported this feature for Kotlin for a while but my impression is it has never been that popular even though for Kotlin there isn't the issue of showing invalid code due to types on the RHS.
One other minor challenge with this functionality is that it messes with how code looks like it is indented and creates lines that appear to be more than 80 chars even when dartfmt is set to 80 chars.

[jacob314]

Fyi @munificent

[gaaclarke]

I just checked Rusts plugin to see what they do to and they have individual controls over all the contexts in which the inlay hints can show up. There is also a broad switch inside VSCode for turning off inlay hints.

This feature probably makes sense to have off by default.

I disagree. It's easier to turn off something you know exists than to turn something on you don't know exists.

I also think you have a bit of an expert bias. There are 2 reasons to look at code, you are writing code or you are reading code someone else wrote. I agree that if you are writing code, presumably you know the types so it isn't as useful then. That isn't the case when jumping into code you don't know. Also, from years of performing hiring interviews you'd be surprised at how many errors candidates created just because they lost track of their types in Python.

[DanTup]

Yes, I agree this should not be visible by default. I hadn't realised it was on because I had toggled the settings while testing (it's a VS Code feature/setting - we just provide the data).

I'll override the default for Dart for the next extension release. I think "offUnlessPressed" is a better default than "off" though. That shows them while you're holding down Ctrl+Option and they disappear again when you release (this is how I have it set). This avoids people needing to change settings to get the benefit of the feature - they can press and hold the keys if they want to quickly see the types (and parameter names which are also now included) without having to hover things.

I've filed #4162 about this.

[DanTup]

I disagree. It's easier to turn off something you know exists than to turn something on you don't know exists.

I do usually agree with this, but these labels can be very noisy and can cause your cursor to bounce around as you're typing (though there are some VS Code issues about adding debouncing to reduce that). I think it's probably better to just call out in release notes/on twitter/etc. and hope those that would like it will discover it rather than have people dislike it and not know if they can turn it off (it's not obvious what they are called if you don't know, so you wouldn't know what to search for).

[jacob314]

C# has the same challenges Dart has due to types on the LHS and they have this option off by default.

[gaaclarke]

@DanTup It doesn't have to be an all or nothing choice. If there was one case where we thought it was an obvious win and have it turned on I'd be happier since it would advertise the feature's existence. A user not knowing it exists is the worst case scenario imo, not a user being temporarily annoyed.

[jacob314]

I'd suggest we try a gradual rollout starting opt in rather than risking annoy users. Typescript also has this disabled by default and they are a better fit for the feature due to types on the RHS.

[jacob314]

Related: typescript has a nice option we should also leverage of avoiding showing types if the variable name matches the type.

[DanTup]

@gaaclarke

A user not knowing it exists is the worst case scenario imo, not a user being temporarily annoyed.

I think this is subjective. While it's a shame for users to not know about features, that's not disruptive. I've seen people roll back software or be reluctant to update frequently because of issues that occur after updating, and I definitely don't want to contribute to that just because people might not know how to turn a new feature off.

Having this appear like this was entirely accidental - even if we did plan to ship it that way, I try to test features myself for a little while before enabling them for users (I often don't find some bugs until I've used it for real work for a while).

@jacob314

typescript has a nice option we should also leverage of avoiding showing types if the variable name matches the type.

That's interesting, though I wonder if it might look odd if a user is not expecting that. I don't know if it saves a lot of most people will use it on offUnlessPressed mode. Interested to hear feedback as people start to use it though.

FWIW, I've pushed a hotfix release v3.48.4 (and pre-release v3.49.20220916) that overrides this default so it shouldn't just show up unexpected for any master users. If we want to revisit whether it should be completely visible by default in future, we can do that - but not before we know the feature is solid (and right now in master, it's not - there are at least a few bug fixes making their way through the machine).