Parameter inlay hint separate from variable type inlay? #2876 #3543

slyngbaek · 2020-03-10T07:59:35Z

Add setting to allow enabling either type inlay hints or parameter
inlay hints or both. Group the the max inlay hint length option
into the object.

Add a new type for the inlayHint options.
Add tests to ensure the inlays don't happen on the server side

crates/ra_project_model/src/lib.rs

SomeoneToIgnore

Thanks for the PR.

I've added a few comments on how I think it can be improved to scale better in the future.

Out CI shows a small compile error: https://github.com/rust-analyzer/rust-analyzer/pull/3543/checks?check_run_id=497229725#step:8:250

so you might want to fix that also :)

crates/ra_project_model/src/lib.rs

SomeoneToIgnore · 2020-03-10T10:09:33Z

crates/ra_project_model/src/lib.rs

+#[derive(Deserialize, Clone, Debug, PartialEq, Eq)]
+#[serde(rename_all = "camelCase", default)]
+pub struct InlayHintOptions {
+    pub display_type: InlayHintDisplayType,


#2741 have plans to add more various inlay hint types later.

I think we'd better allow to have various combinations of the types turned on simultaneously, since there will be requests for those.
So should we go with a set of InlayKind's instead of this enum?

A bitset I guess?

Imo, just the HashSet<InlayKind> is enough.
Or its fx counterpart, since we use FxHashMap in the project.

I recall @matklad once said it's better not to use HashSet where Vec is enough

I'm still a bit of a beginner with rust, but @matklad suggested it be a POD (I'm assuming meaning that it implements Copy?). If I use a Vec or any of these structures it can't be a Copy type.

crates/ra_project_model/src/lib.rs

editors/code/src/config.ts

crates/ra_ide/src/lib.rs

Add setting to allow enabling either type inlay hints or parameter inlay hints or both. Group the the max inlay hint length option into the object. - Add a new type for the inlayHint options. - Add tests to ensure the inlays don't happen on the server side

Veetaha · 2020-03-10T21:33:55Z

@matklad What came to my mind is a feature flag for ra_ide.
I saw some crate that did place #[derive(Serialize, Deserialize)] on exported structs but only if serde feature is enabled (i.e. one that is passed in Cargo.toml crate_name = { features = ["serde"]}).
Would it make sense to follow the same pattern and put serde as an optional dependency on ra_ide to prevent copy-pasting the code?

- Updated naming of config - Define struct in ra_ide and use remote derive in rust-analyzer/config - Make inlayConfig type more flexible to support more future types - Remove constructor only used in tests

slyngbaek · 2020-03-10T22:46:49Z

I fixed the build error and addressed the issues mentioned above.

SomeoneToIgnore · 2020-03-10T23:07:43Z

crates/rust-analyzer/src/config.rs

 use rustc_hash::FxHashMap;

 use ra_project_model::CargoFeatures;
 use serde::{Deserialize, Deserializer};

+#[derive(Deserialize)]
+#[serde(remote = "InlayKind")]
+pub enum InlayKindDef {


I think there's already an enum for this entity:
https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/rust-analyzer/src/req.rs#L199

so let's not create a copy of it.
A few more observations, feel free to implement any (or none) of them, if you want to:

It could be a bit simpler to use a match to map between both InlayKind enums, similar to the https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/rust-analyzer/src/main_loop/handlers.rs#L1006

Since you can easily deserialize a Vec<InlayKind>, you can ask for that in the package.json instead and get rid of all changes in the ra_project_model crate.
Less code with more flexibility for the end users (as long as they know which options are available)

Does it make sense to deduplicate the InlayKind as mentioned and then just use the serde remote feature so we don't even need to do the match in the first place?

Personally, I'd prefer using match, but if your way works, it's also good, thanks for the dedup.

editors/code/src/config.ts

- Remove match conversion for InlayKind since we're using remote

SomeoneToIgnore · 2020-03-11T08:48:34Z

Did you try to install and run the branch after all the changes?

I've tried and got

[ERROR rust_analyzer] Failed to deserialize config: missing field `display_type`; {"cargoFeatures":{"allFeatures":true,"features":[],"noDefaultFeatures":false},"cargoWatchAllTargets":true,"cargoWatchArgs":[],"cargoWatchCommand":"check","cargoWatchEnable":false,"excludeGlobs":[],"featureFlags":{},"inlayHintOpts":{"displayType":"full","maxLength":20},"lruCapacity":null,"publishDecorations":false,"rustfmtArgs":[],"useClientWatching":false,"withSysroot":true}

I think #[serde(rename_all = "camelCase")] annotation is missing on the config struct.

I also notice that the ra_project_model::InlayHintDisplayType is not used anywhere, not even in a single match statement.
This feels like an error, since we need to fill the Vec<InlayKind> based on it, don't we?

Or, as proposed earlier, we can fully remove the chages in ra_project_model and change the code and package.json to autoamtically deserialize into the Vec<InlayKind>.

Veetaha · 2020-03-11T09:29:01Z

editors/code/package.json

+                "rust-analyzer.inlayHintOpts.displayType": {
+                    "type": "string",
+                    "enum": [
+                        "off",
+                        "typeHints",
+                        "parameterHints",
+                        "full"
+                    ],
+                    "enumDescriptions": [
+                        "No type inlay hints",
+                        "Type inlays hints only",
+                        "Parameter inlays hints only",
+                        "All inlay hints types"
+                    ],


Suggested change

"rust-analyzer.inlayHintOpts.displayType": {

"type": "string",

"enum": [

"off",

"typeHints",

"parameterHints",

"full"

],

"enumDescriptions": [

"No type inlay hints",

"Type inlays hints only",

"Parameter inlays hints only",

"All inlay hints types"

],

"rust-analyzer.inlayHints": {

"type": "object",

"properties": {

"types": {

"type": "boolean",

"default": true,

"description": "Whether to show inlay type hints"

},

"parameterNames": {

"type": "boolean",

"default": true,

"description": "Whether to show function parameter names inlay hints at the call site"

}

}

I believe just a struct with booleans will play well. This should be more user-friendly, since people tend to misspel enum string values to much (and we don't validate this on client-side, but misspelling of config keys is handled by vscode). This also gives us flexibility to add new kinds of inlay hints without having to create enum value for each of their combination (i.e. off, full, inlay1, inlay1_and_inlay2, inlay1_and_inlay3 ... etc. this gives us a combinatorial explosion)
And maybe this makes sense to use such a struct on the server-side as well @SomeoneToIgnore, @slyngbaek ?

I converted from using the Vec<InlayKind> to an object with props. Instead of using a single object type I made several nested props so that they show up in the GUI.

@SomeoneToIgnore You're right. I tested it before pushing this time. I also removed the dead code in the ra_project_model.

- Instead of a single object type, use several individual nested types to allow toggling from the settings GUI - Remove unused struct definitions - Install and test that the toggles work

editors/code/package.json

editors/code/src/config.ts

SomeoneToIgnore

Nice, thanks for the fixes.
I think we're good to merge it.

matklad · 2020-03-12T16:02:39Z

bors r+

Veetaha · 2020-03-12T16:03:31Z

@matklad any comments on this? This is not regarding the PR, but about possible general design?

matklad · 2020-03-12T16:08:24Z

ra_ide knows nothing about LSP by design. The cost of copy-paste is much smaller than the cost of breaching the strong abstraction boundary ra_ide is.

EDIT: sorry, this is a too short reply,let me actually take some time to write smth more reasonable :)

bors · 2020-03-12T16:11:38Z

Build succeeded

Veetaha · 2020-03-12T16:11:42Z

Yeah, I think we are just choosing between what looks less workaround-y

matklad · 2020-03-12T16:17:07Z

Longer reply.

rust-analyzer is intended to be isolated from LSP. I feel that the potential use-cases for rust-analyzer stretch far beyond just LSP, and, down the road, hard dependency on LSP would be a liability.

This is the reason why we spend 700 lines of code in conv.rs doing mostly trivial conversions between the types. It's a design goal that all the types exported from ra_ide are 100% tailored to the rust language, and not to the LSP protocol.

In this particlar case, what happens is that we need two completely unrelated things:

we need a way to tweak how type hints internally work
we need to deserialize config from JSON

The fact that the the same struct can server both cases, now, is a pure coincidence. Specifically, if we upstream this feature to LSP, the LSP would likely use the type of the different shape. This is kind of a general instance of the advice that you don't make your domain objects directly serializable, but instead have an intermediate layer which specifically handles serizliation. This makes it possible to vary domain objects and serialization independently.

Veetaha · 2020-03-12T16:25:42Z

Okay you persuaded me, thanks for taking the time!

Veetaha reviewed Mar 10, 2020

View reviewed changes

crates/ra_project_model/src/lib.rs Outdated Show resolved Hide resolved

SomeoneToIgnore reviewed Mar 10, 2020

View reviewed changes

matklad reviewed Mar 10, 2020

View reviewed changes

crates/ra_ide/src/lib.rs Outdated Show resolved Hide resolved

Address Issues from Github

cfb48df

- Updated naming of config - Define struct in ra_ide and use remote derive in rust-analyzer/config - Make inlayConfig type more flexible to support more future types - Remove constructor only used in tests

slyngbaek force-pushed the 2876 branch from db56392 to cfb48df Compare March 10, 2020 21:45

SomeoneToIgnore reviewed Mar 10, 2020

View reviewed changes

Deduplicate some Inlay definitions

974ed71

- Remove match conversion for InlayKind since we're using remote

Veetaha reviewed Mar 11, 2020

View reviewed changes

Switch from Vec<InlayKind> to object with props

58248e2

- Instead of a single object type, use several individual nested types to allow toggling from the settings GUI - Remove unused struct definitions - Install and test that the toggles work

SomeoneToIgnore reviewed Mar 12, 2020

View reviewed changes

editors/code/package.json Outdated Show resolved Hide resolved

editors/code/src/config.ts Outdated Show resolved Hide resolved

editors/code/src/config.ts Outdated Show resolved Hide resolved

Make maxLength nullable again

a153b90

SomeoneToIgnore approved these changes Mar 12, 2020

View reviewed changes

SomeoneToIgnore requested a review from matklad March 12, 2020 15:57

bors bot merged commit d98a5fa into rust-lang:master Mar 12, 2020

Veetaha mentioned this pull request Mar 14, 2020

Parameter inlay hint separate from variable type inlay? #2876

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parameter inlay hint separate from variable type inlay? #2876 #3543

Parameter inlay hint separate from variable type inlay? #2876 #3543

slyngbaek commented Mar 10, 2020

SomeoneToIgnore left a comment

SomeoneToIgnore Mar 10, 2020 •

edited

Loading

Veetaha Mar 10, 2020

SomeoneToIgnore Mar 10, 2020

Veetaha Mar 10, 2020

slyngbaek Mar 10, 2020

Veetaha commented Mar 10, 2020

slyngbaek commented Mar 10, 2020

SomeoneToIgnore Mar 10, 2020

slyngbaek Mar 11, 2020

SomeoneToIgnore Mar 11, 2020

SomeoneToIgnore commented Mar 11, 2020

Veetaha Mar 11, 2020 •

edited

Loading

slyngbaek Mar 12, 2020

slyngbaek Mar 12, 2020 •

edited

Loading

SomeoneToIgnore left a comment

matklad commented Mar 12, 2020

Veetaha commented Mar 12, 2020

matklad commented Mar 12, 2020 •

edited

Loading

bors bot commented Mar 12, 2020

Veetaha commented Mar 12, 2020

matklad commented Mar 12, 2020

Veetaha commented Mar 12, 2020

-                "rust-analyzer.inlayHintOpts.displayType": {
-                    "type": "string",
-                    "enum": [
-                        "off",
-                        "typeHints",
-                        "parameterHints",
-                        "full"
-                    ],
-                    "enumDescriptions": [
-                        "No type inlay hints",
-                        "Type inlays hints only",
-                        "Parameter inlays hints only",
-                        "All inlay hints types"
-                    ],
+                "rust-analyzer.inlayHints": {
+                    "type": "object",
+                    "properties": {
+                        "types": {
+                            "type": "boolean",
+                            "default": true,
+                            "description": "Whether to show inlay type hints"
+                        },
+                        "parameterNames": {
+                            "type": "boolean",
+                            "default": true,
+                            "description": "Whether to show function parameter names inlay hints at the call site"
+                        }
+                    }

Parameter inlay hint separate from variable type inlay? #2876 #3543

Parameter inlay hint separate from variable type inlay? #2876 #3543

Conversation

slyngbaek commented Mar 10, 2020

SomeoneToIgnore left a comment

Choose a reason for hiding this comment

SomeoneToIgnore Mar 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Veetaha commented Mar 10, 2020

slyngbaek commented Mar 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SomeoneToIgnore commented Mar 11, 2020

Veetaha Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slyngbaek Mar 12, 2020 • edited Loading

Choose a reason for hiding this comment

SomeoneToIgnore left a comment

Choose a reason for hiding this comment

matklad commented Mar 12, 2020

Veetaha commented Mar 12, 2020

matklad commented Mar 12, 2020 • edited Loading

bors bot commented Mar 12, 2020

Build succeeded

Veetaha commented Mar 12, 2020

matklad commented Mar 12, 2020

Veetaha commented Mar 12, 2020

SomeoneToIgnore Mar 10, 2020 •

edited

Loading

Veetaha Mar 11, 2020 •

edited

Loading

slyngbaek Mar 12, 2020 •

edited

Loading

matklad commented Mar 12, 2020 •

edited

Loading