[llvm-bugs] [Bug 45979] New: onInitialize: Capabilities.textDocumentSync.save should be of type SaveOptions

[via llvm-bugs]

https://bugs.llvm.org/show_bug.cgi?id=45979

            Bug ID: 45979
           Summary: onInitialize: Capabilities.textDocumentSync.save
                    should be of type SaveOptions
           Product: clang-tools-extra
           Version: unspecified
          Hardware: PC
                OS: Linux
            Status: NEW
          Severity: normal
          Priority: P
         Component: clangd
          Assignee: unassignedclangbugs at nondot.org
          Reporter: planger at eclipsesource.com
                CC: llvm-bugs at lists.llvm.org

The property textDocumentSync is of type TextDocumentSyncOptions, as defined
below (see also [1])

  export interface TextDocumentSyncOptions {
    ...
    save?: SaveOptions;
  }

and its property save should follow the type SaveOptions (see also [1]):

  export interface SaveOptions {
    /**
      * The client is supposed to include the content on save.
      */
    includeText?: boolean;
  }

Thus, imho the definition of capabilities should imho be:

  capabilities: {
    ...
    textDocumentSync: {
      openClose: true,
      ...,
      save: {  includeText: true  }
    }
    ...
  }

instead of what clangd sends since commit
596b63ad4019e61030803789a1844a0f1aeb34db:

  capabilities: {
    ...
    textDocumentSync: {
      openClose: true,
      ...,
      save: true
    }
    ...
  }

If you agree, this should be a rather easy fix in
clang-tools-extra/clangd/ClangdLSPServer.cpp:582 [2], since it returns just a
static value here.


Note that the LSP spec is (imho) a bit unclear on that, because it also
defines:

"
Server Capability:

property name (optional): textDocumentSync.save
property type: boolean | SaveOptions where SaveOptions is defined as follows:
" [1]

So here the spec defines that property save can also be boolean (as used by
clangd currently). However, in the type definition of TextDocumentSyncOptions,
it doesn't say TextDocumentSyncOptions.save?: SaveOptions | boolean.

Either way, since it is such an easy fix in clangd and because the type
definition of TextDocumentSyncOptions is more explicit, I'd find it great, if
clangd would follow the type definition of TextDocumentSyncOptions.
Otherwise LSP frameworks/clients may fail to parse what is currently sent by
clangd. I've seen, for instance, that LSP4J [3] fails to use clangd-11 right
now because of this inconsistency.

Thanks so much in advance!

[1]
https://microsoft.github.io/language-server-protocol/specifications/specification-current/
[2]
https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clangd/ClangdLSPServer.cpp#L582
[3] https://github.com/eclipse/lsp4j

-- 
You are receiving this mail because:
You are on the CC list for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-bugs/attachments/20200518/2ed37da0/attachment.html>