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

x/tools/gopls: tolerate JSON decoding failures from type mismatch if the affected field is unused #45316

Open
hyangah opened this issue Mar 31, 2021 · 3 comments
Labels
Milestone

Comments

@hyangah
Copy link
Contributor

@hyangah hyangah commented Mar 31, 2021

During VSCode v0.23.1 release that included backward-incompatible change (upgrading LSP 3.16 prerelease to stable), we encountered an error like

Starting client failed
  Message: JSON RPC parse error: json: cannot unmarshal number into Go struct field RenameClientCapabilities.capabilities.textDocument.rename.prepareSupportDefaultBehavior of type bool: JSON RPC parse error
  Code: -32700 

that stopped gopls. (golang/vscode-go#1328)

The problematic field prepareSupportDefaultBehavior is not used by gopls in this specific case. Please investigate if gopls can handle this kind of type mismatch gracefully (i.e. decoding only necessary, relevant fields) for improved forward compatibility.

@pjweinb
Copy link

@pjweinb pjweinb commented Apr 5, 2021

I propose closing this issue. Fixing it would appear to require figuring out which fields in which LSP protocol messages gopls doesn't use (and won't use). I see no reasonable way of doing that, other than by hand. (If we knew which fields these were, then we could either just leave them out of the generated tsprotocol.go, or write special code in place of json.Unmarshal.)

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Apr 6, 2021

We've discussed this issue and decided that it makes sense to pursue this.

One option is to compile a list of the fields that we use in the initialization options and only add those fields to the struct in the LSP API generator. If a new field gets used in a future version, we will have to manually add it to the list of used fields.

@heschi and @ianthehat also suggested the possibility of checking the error message to get the name of the unknown field, unmarshaling the message to a map[string]interface{}, deleting the problematic field, and then trying to unmarshal to a struct again, until there are no errors. If that is feasible, that seems like it may be a better approach.

@stamblerre stamblerre modified the milestones: Unreleased, gopls/v1.0.0 Apr 6, 2021
@pjweinb
Copy link

@pjweinb pjweinb commented Apr 11, 2021

The simplest way to handle this error is to keep going if the error is an *json.UnmarshalType error. Tests show that the unmarshalled struct has all its other fields filled in properly. Probably the error should be logged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants