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 client capability for semantic tokens #27

Merged
merged 3 commits into from
Jul 9, 2021
Merged

Fix client capability for semantic tokens #27

merged 3 commits into from
Jul 9, 2021

Conversation

markdumay
Copy link
Contributor

First of all, great to see you guys are making the effort to port the LSP specification to Go. I started experimenting with your package quite recently. I encountered a small problem when trying to unmarshal the InitializeParams as generated by vscode-languageclient with version 7.0.0. VSCode generates the client capabilities automatically, but handling the payload of InitializeParams on the server side (written in Go using your protocol package) gives the following error:

Message: json: cannot unmarshal object into Go struct field SemanticTokensWorkspaceClientCapabilitiesRequests.capabilities.textDocument.semanticTokens.requests.full of type bool

It seems the following section of the InitializeParams JSON is the cause of the issue.

[...]
    "textDocument": {
      "semanticTokens": {
        "requests": {
          "range": true,
          "full": {
            "delta": true
          }
      }
    }
[...]

According to the official specification, the value for full can be either boolean or another embedded struct.

This PR fixes the issue by adjusting the struct SemanticTokensWorkspaceClientCapabilitiesRequests in capabilities_client.go. It simply changes the type of Full from bool to interface{}. Let me know if you need more evidence or supporting files to reproduce the error.

@markdumay markdumay changed the title Fix server capability for semantic tokens Fix client capability for semantic tokens Jun 29, 2021
@zchee zchee self-requested a review July 4, 2021 18:04
@zchee
Copy link
Member

zchee commented Jul 4, 2021

@markdumay Thanks for contributing!
I understand you said problem and approve this change.
But I forgotten enabled run forked pull request on CircleCI. I'll dig CircleCI docs, but could you amend and force push this branch?
Thanks.

@markdumay
Copy link
Contributor Author

Great, good to hear you approve the proposed change. I made a small documentation change and submitted a force push. It seems this has triggered the CircleCI workflows. There is now an error though:

./capabilities_client_gojay.go:1824: cannot use v.Full (type interface {}) as type bool in argument to enc.BoolKeyOmitEmpty: need type assertion
./capabilities_client_gojay.go:1836: cannot use &v.Full (type *interface {}) as type *bool in argument to dec.Bool
WARN invalid TestEvent: FAIL	go.lsp.dev/protocol [build failed]

It looks like the capabilities_client_gojay.go file is out of sync. Is there a specific command that you run to regenerate the gojay files?

@zchee
Copy link
Member

zchee commented Jul 7, 2021

@markdumay sorry for late, I'll comment later, maybe ~6h.

@zchee
Copy link
Member

zchee commented Jul 8, 2021

@markdumay Unfortunately, the gojay bindings are written by my hand.

Could you patch the below diff? It should work.

diff --git a/capabilities_client_gojay.go b/capabilities_client_gojay.go
index 3c2be32..759d87a 100644
--- a/capabilities_client_gojay.go
+++ b/capabilities_client_gojay.go
@@ -1821,7 +1821,7 @@ var (
 // MarshalJSONObject implements gojay.MarshalerJSONObject.
 func (v *SemanticTokensWorkspaceClientCapabilitiesRequests) MarshalJSONObject(enc *gojay.Encoder) {
 	enc.BoolKeyOmitEmpty(keyRange, v.Range)
-	enc.BoolKeyOmitEmpty(keyFull, v.Full)
+	enc.AddInterfaceKey(keyFull, v.Full)
 }
 
 // IsNil implements gojay.MarshalerJSONObject.
@@ -1833,7 +1833,7 @@ func (v *SemanticTokensWorkspaceClientCapabilitiesRequests) UnmarshalJSONObject(
 	case keyRange:
 		return dec.Bool(&v.Range)
 	case keyFull:
-		return dec.Bool(&v.Full)
+		return dec.Interface(&v.Full)
 	}
 	return nil
 }

@markdumay
Copy link
Contributor Author

Thanks @zchee, I force pushed the changes. CircleCI now indicates I'm not authorized to run the workflows though.

@codecov
Copy link

codecov bot commented Jul 9, 2021

Codecov Report

Merging #27 (9f6c20c) into main (c071fd8) will not change coverage.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main     #27   +/-   ##
=====================================
  Coverage   60.5%   60.5%           
=====================================
  Files         40      40           
  Lines       5538    5538           
=====================================
  Hits        3356    3356           
  Misses      2143    2143           
  Partials      39      39           
Flag Coverage Δ
gojay 67.0% <100.0%> (ø)
json 20.4% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
capabilities_client.go 100.0% <ø> (ø)
capabilities_client_gojay.go 91.4% <100.0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c071fd8...9f6c20c. Read the comment docs.

@zchee zchee merged commit d33fc40 into go-language-server:main Jul 9, 2021
@zchee
Copy link
Member

zchee commented Jul 9, 2021

@markdumay merged. Thanks contribute!

@zchee
Copy link
Member

zchee commented Jul 9, 2021

@markdumay
Copy link
Contributor Author

Great, glad to help. Keep up the good work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants