Respect server document sync capabilities

[laughedelic]

See #193 (comment) for the context. This is a minor improvement avoiding breaking changes:

• don't send didOpen/didClose notifications if documentSync.openClose is explicitly disabled
• add document content if the documentSync.save.includeText is enabled

---

And here are some thoughts for discussion.

I think that current behaviour is not completely correct, but fixing it may break somebody's server. IMO in LSP v3 all capability options have to be enabled explicitly. So if the server wants to receive open/close notifications, it should set openClose: true, same as willSave/willSaveWaitUntil.

• right now these options are treated differently: willSave* has to be enabled explicitly, but openClose and save not.
• with current behaviour one still can disable open/close/willSave* notifications, but cannot disable save (because it's not a boolean and undefined is implicitly treated as true)
• change option is treated specially: it determines whether all other notifications are even considered

An example: if a server cares only about open, close and willSaveWaitUntil notifications, but doesn't want to receive any other, it cannot communicate it. Turning off change notifications while receiving some others can be useful if server has its own file watching mechanism. But currently it will result in DocumentSyncAdapter.canAdaptV3 returning false and not receiving any notifications at all.

Also, I'm not sure about this, but I think that didChangeWatchedFiles notifications have to be registered independently from all above.

[damieng]

I think that current behaviour is not completely correct, but fixing it may break somebody's server. IMO in LSP v3 all capability options have to be enabled explicitly. So if the server wants to receive open/close notifications, it should set openClose: true, same as willSave/willSaveWaitUntil.

The problem is that openClose capabilities flag does not exist in LSP v2 but didSave and didOpen messages do so back in v2 you definitely had to always send it to be compliant. There is no version level negotiation between a client and server and we definitely still want to support v2 servers so I think what you've done here - defaulting to sending it unless specified otherwise - is the right thing to do.

Save and open events are quite lightweight so I don't think it's too bad for a server to just ignore what they don't want.

Also, I'm not sure about this, but I think that didChangeWatchedFiles notifications have to be registered independently from all above.

I thought that one was capabilities flagged but it seems that was missed.

[laughedelic]

OK, I see that the current behaviour is justified by the compatibility with LSP v2.

There is no version level negotiation between a client and server

But in this particular context there is clear distinction between v2 and v3: the type of the sync options is either TextDocumentSyncKind/number (v2) or TextDocumentSyncOptions (v3). We could preserve compatibility with the LSP v2 servers by setting those defaults explicitly when converting from number to TextDocumentSyncOptions:

  this._documentSync = {
    openClose: true,
    change: documentSync || TextDocumentSyncKind.Full,
    save: { includeText: false }, // not sure what was the default in v2
    // willSave options were not present in v2 AFAIK
  };

While if the server sends TextDocumentSyncOptions it is treated as is and absent options are interpreted as false. What do you think of this approach?

On the second thought, it doesn't make much difference anyway... So don't mind it.

Also, I'm not sure about this, but I think that didChangeWatchedFiles notifications have to be registered independently from all above.

I thought that one was capabilities flagged but it seems that was missed.

I guess, fixing this will require implementing #155 first.

[damieng]

That logic sounds good to me but like you I could go either way so it's up to you :)

[laughedelic]

Let's leave it as is. Now there is an option to disable those features (except didSave), so if server wants or doesn't want some notifications, it can be explicit about it one way or the other, so it doesn't matter how undefined values are interpreted implicitly.

[daviwil]

Looks awesome, thanks a lot for making this improvement!

[laughedelic]

If this is approved, could you merge it and publish a new release?

[laughedelic]

Thanks!