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

fix: incorrect initialization #146

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: incorrect initialization #146

wants to merge 2 commits into from

Conversation

onion108
Copy link

Fixed incorrect deserialization of some field according to the latest Language Server Protocol specification.

According to the definition of InitializeParams, the field initializationOptions with any type should not be an error while deserializing. In CoC.nvim, this field is marked as "deprecated". After checking more implementations, {}, "" and null should be acceptable for clients that doesn't use this field to send, but sending {} (from CoC.nvim) caused v-analyzer to crash.

According to the definition of CompletionClientCapabilities, the type of documentationFormat should be MarkupKind[]? (where MarkupKind is just "markdown" | "plaintext", according to the definition), but somehow we are deserializing this thing as bool. This also caused v-analyzer to crash.

Fixed incorrect deserialization of some field according to the latest
Language Server Protocol specification.
@onion108
Copy link
Author

related issue:
#145

@onion108
Copy link
Author

oh i read V's source code and find any occurance of 'skip' will cause the field skipped, no matter if field's json name is specified or not. gonna fix it soon (by implementing LSPAny type according to spec)

@onion108
Copy link
Author

find a even more serious problem (in the V source code)... the json decoder codegen doesn't handle optional sum type correctly

But because of some problem within the V's json decode generator it's
not possible to handle it prefectly with V's json module yet (optional
type not handled correctly for sumtype & other types except struct)

fomat
@spytheman
Copy link
Member

spytheman commented Dec 16, 2024

I think those fields are not used by v-analyzer currently for anything, and thus, they can be just deleted. After that, the decoder will not have to know anything more; it will just ignore them.

@@ -9,7 +9,7 @@ pub mut:
pub struct CompletionItemSettings {
snippet_support bool @[json: snippetSupport]
commit_characters_support bool @[json: commitCharactersSupport]
documentation_format bool @[json: documentationFormat]
documentation_format []string @[json: documentationFormat]
Copy link
Member

Choose a reason for hiding this comment

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

It is not used for anything.
Imho just delete documentation_format.

root_uri DocumentUri @[json: rootUri]
root_path DocumentUri @[json: rootPath]
// TODO: Change this to `?LSPAny` once V fixed its JSON decoder codegen. (or shall we use json2?)
initialization_options LSPAny @[json: initializationOptions]
Copy link
Member

Choose a reason for hiding this comment

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

It is not used for anything, imho just delete initialization_options.

Copy link
Author

@onion108 onion108 Dec 16, 2024

Choose a reason for hiding this comment

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

Well they ARE used somewhere...
image
(I tried to change the type of this field without modifying anything and the compiler complains about the type, obviously some part of the code used that)

it IS ok to delete documentation_format though. but deleting initialization_options seems to be way harder than documentation_format.

Comment on lines -28 to +31
ls.initialization_options = params.initialization_options.fields()
options := params.initialization_options
if options is string {
ls.initialization_options = options.fields()
}
Copy link
Member

Choose a reason for hiding this comment

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

This will not be needed, if the fields are deleted.

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.

2 participants