Send LSP Completion Item Kind

[schrieveslaach]

Description

This commit fills in the completion item kind into the textDocument/completion response so that LSP client can present more information to the user.

It is an improvement in the context of #10794

User-Facing Changes

Improved information display in editor's intelli-sense menu

[schrieveslaach]

@fdncred, do you mind to check out some TODO comments, e.g. here, in order to clarify what kind of kind Nushell could send in these cases.

I'm not so familiar with the completion code deep down in Nushell.

[fdncred]

I'm not familiar with code completion either. Sorry. Here's my guess.

// TODO: can we assume that this is kind VALUE?

x is definitely a nushell enum Value here and Value can be any nushell datatype. when it's calling as_string() it's this function, so it's a limited set of nushell values.

nushell/crates/nu-protocol/src/value/mod.rs

Lines 312 to 336 in 21b3eee

	pub fn as_string(&self) -> Result<String, ShellError> {
	match self {
	Value::Int { val, .. } => Ok(val.to_string()),
	Value::Float { val, .. } => Ok(val.to_string()),
	Value::String { val, .. } => Ok(val.to_string()),
	Value::Binary { val, .. } => Ok(match std::str::from_utf8(val) {
	Ok(s) => s.to_string(),
	Err(_) => {
	return Err(ShellError::CantConvert {
	to_type: "string".into(),
	from_type: "binary".into(),
	span: self.span(),
	help: None,
	});
	}
	}),
	Value::Date { val, .. } => Ok(val.to_rfc3339_opts(chrono::SecondsFormat::Millis, true)),
	x => Err(ShellError::CantConvert {
	to_type: "string".into(),
	from_type: x.get_type().to_string(),
	span: self.span(),
	help: None,
	}),
	}
	}

/ TODO: can we assume that this is kind RECORD or STRUCT?

the match for as_record is ensuring this is a nushell record. this is the function so I think we can be pretty sure it's a record.

nushell/crates/nu-protocol/src/value/mod.rs

Lines 403 to 413 in 21b3eee

	pub fn as_record(&self) -> Result<&Record, ShellError> {
	match self {
	Value::Record { val, .. } => Ok(val),
	x => Err(ShellError::CantConvert {
	to_type: "record".into(),
	from_type: x.get_type().to_string(),
	span: self.span(),
	help: None,
	}),
	}
	}

I'm not exactly sure how we map to CompletionItemKind though. Some things seem obvious, like we have these:
TEXT would be Value::String
METHOD might be a subcommand?
FUNCTION is probably a custom command
FIELD might be an element of a Value::Record
VARIABLE would be a variable
MODULE might be a module like when you use modulename.nu
UNIT would maybe be like Value::Filesize such as KB or KiB and durations like sec, min
VALUE would be value?
KEYWORD we have keywords although they're not really called out like if, for, let
CONSTANT we have const's
OPERATOR we have operators
TYPE_PARAMETER I guess this could be the type annotation of custom command parameters?

Not supported I guess?
CLASS
INTERFACE
PROPERTY
ENUM
SNIPPET
COLOR - we have colors but not a color type
FILE - we have SyntaxShape::Filepath i think but not Value::Path
REFERENCE
FOLDER
ENUM_MEMBER
STRUCT
EVENT

Nushell Types

nushell/crates/nu-protocol/src/ty.rs

Lines 11 to 36 in 21b3eee

	pub enum Type {
	Any,
	Binary,
	Block,
	Bool,
	CellPath,
	Closure,
	Custom(String),
	Date,
	Duration,
	Error,
	Filesize,
	Float,
	Int,
	List(Box<Type>),
	ListStream,
	#[default]
	Nothing,
	Number,
	Range,
	Record(Vec<(String, Type)>),
	Signature,
	String,
	Table(Vec<(String, Type)>),
	}

Nushell Values

nushell/crates/nu-protocol/src/value/mod.rs

Lines 44 to 155 in 21b3eee

	pub enum Value {
	Bool {
	val: bool,
	// note: spans are being refactored out of Value
	// please use .span() instead of matching this span value
	internal_span: Span,
	},
	Int {
	val: i64,
	// note: spans are being refactored out of Value
	// please use .span() instead of matching this span value
	internal_span: Span,
	},
	Float {
	val: f64,
	// note: spans are being refactored out of Value
	// please use .span() instead of matching this span value
	internal_span: Span,
	},
	Filesize {
	val: i64,
	// note: spans are being refactored out of Value
	// please use .span() instead of matching this span value
	internal_span: Span,
	},
	Duration {
	val: i64,
	// note: spans are being refactored out of Value
	// please use .span() instead of matching this span value
	internal_span: Span,
	},
	Date {
	val: DateTime<FixedOffset>,
	// note: spans are being refactored out of Value
	// please use .span() instead of matching this span value
	internal_span: Span,
	},
	Range {
	val: Box<Range>,
	// note: spans are being refactored out of Value
	// please use .span() instead of matching this span value
	internal_span: Span,
	},
	String {
	val: String,
	// note: spans are being refactored out of Value
	// please use .span() instead of matching this span value
	internal_span: Span,
	},
	Record {
	val: Record,
	// note: spans are being refactored out of Value
	// please use .span() instead of matching this span value
	internal_span: Span,
	},
	List {
	vals: Vec<Value>,
	// note: spans are being refactored out of Value
	// please use .span() instead of matching this span value
	internal_span: Span,
	},
	Block {
	val: BlockId,
	// note: spans are being refactored out of Value
	// please use .span() instead of matching this span value
	internal_span: Span,
	},
	Closure {
	val: Closure,
	// note: spans are being refactored out of Value
	// please use .span() instead of matching this span value
	internal_span: Span,
	},
	Nothing {
	// note: spans are being refactored out of Value
	// please use .span() instead of matching this span value
	internal_span: Span,
	},
	Error {
	error: Box<ShellError>,
	// note: spans are being refactored out of Value
	// please use .span() instead of matching this span value
	internal_span: Span,
	},
	Binary {
	val: Vec<u8>,
	// note: spans are being refactored out of Value
	// please use .span() instead of matching this span value
	internal_span: Span,
	},
	CellPath {
	val: CellPath,
	// note: spans are being refactored out of Value
	// please use .span() instead of matching this span value
	internal_span: Span,
	},
	#[serde(skip_serializing)]
	CustomValue {
	val: Box<dyn CustomValue>,
	// note: spans are being refactored out of Value
	// please use .span() instead of matching this span value
	internal_span: Span,
	},
	#[serde(skip)]
	LazyRecord {
	val: Box<dyn for<'a> LazyRecord<'a>>,
	// note: spans are being refactored out of Value
	// please use .span() instead of matching this span value
	internal_span: Span,
	},
	}

[fdncred]

I worry about this PR because I'm not sure if we have 100% code coverage in tests and this PR is changing a lot of completion files. I'm not saying it's not right. I'm just saying it makes me nervous.

[schrieveslaach]

@fdncred, I share your worries and therefore, I made my changes the least invasive as possible. I also updated the code and added some LSP tests.

[fdncred]

@schrieveslaach would you mind fixing the conflicts? i'd love to get this in soon.

[Feel-ix-343]

Hey, I have been thinking about Kinds recently and closed a PR realizing you had already started one.

Correct me if I'm wrong, but do we want to couple all of nushell completions to LspTypes? My intuition is that LspTypes should be only used in the LSP crate. To do this, we could possibly implement a completer in the lsp crate, instead of reusing the cli completer.

Beyond the scope of this PR, though, the nushell cli could benefit from giving completion kinds as well. If we were to do this, I feel it would be best to create our own kind type in the cli completer, then implement From for Lsptypes completion kind.

Let me know what you all think

[fdncred]

It would be nice to isolate these changes to nu-lsp if possible. I worry, as stated above already, that negative things might happen as is. Maybe it's ok but I'd rather be as safe as possible. Thoughts?

[fdncred]

@schrieveslaach been missing your prs for nu-lsp lately. hope everything is ok.

do you think these changes can be somehow isolated to the nu-lsp crate?

[schrieveslaach]

hope everything is ok.

A lot of stuff going on here, sorry for not responding earlier. I'll try to reach out to you in the next days.

[fdncred]

A lot of stuff going on here, sorry for not responding earlier. I'll try to reach out to you in the next days.

No worries at all. Thanks for your help! We have a new release Tuesday.

[schrieveslaach]

I revisited the stuff I changed here and I see follow options

1. Add a new enum to the nu-cli crate that represents the kind of completions that is provided
2. Rewrite a new completer in nu-lsp
3. Let lsp-types sink into the rest of nushell (current approach of this PR)

After thinking for a while I prefer option 1 because it seems to me the option with the most clear boundaries among the crates.

I would abandon option 2 because that means there has to a whole rewrite completer.rs in nu-lsp and then repl or lsp completions will be out of sync. Feels like a bad options to me. I would abandon option 3 for the reasons mentioned here.

But still, option 1 requires for me that NuCompleter in nu-cli has an additional method (like fetch_completions_at) that returns then a type that contains the reedline::Suggestion and that new enum.

@fdncred, is that a viable approach to you? Even if I won't be able to provide a whole test suite for the NuCompleter (like you mentioned here)

[fdncred]

@schrieveslaach I think option 1 sounds the safest compared to the other two. Although, at some point in the future, option 2 may be interesting.

[schrieveslaach]

@fdncred, here is a new preliminary version. Do you mind to have a rough look into it? There are still some TODO comments but I want to double check if it is going into the right direction.

[fdncred]

The only real thing that kind of rubs me the wrong way is the kind: None in several places. I wish there was a way to have some generic kind but maybe it doesn't work that way since nushell doesn't support certain language types. But I guess kind: None still completes but it just doesn't give the user more information.

Other than that, it seems good to me. What other changes do you have in mind with this PR?

[fdncred]

@schrieveslaach I think we may be able to land this if we can get the conflict(s) fixed.

[fdncred]

thanks