This is an archive of the discontinued LLVM Phabricator instance.

[clangd][Protocol] Drop optional from WorkspaceEdit::changes
ClosedPublic

Authored by kadircet on Jun 1 2021, 4:25 AM.

Details

Summary

This is causing weird code patterns in various places and I can't see
any difference between None and empty change list. Neither in the current use
cases nor in the spec.

Diff Detail

Event Timeline

kadircet created this revision.Jun 1 2021, 4:25 AM
kadircet requested review of this revision.Jun 1 2021, 4:25 AM
sammccall accepted this revision.Jun 2 2021, 11:16 AM

changes is in fact optional, indicated by the ? in changes?: { [uri: DocumentUri]: TextEdit[]; };.

But the spec requires *some* field to be set, and this is the only one we support, so it's not optional in practice for us.

clang-tools-extra/clangd/Protocol.cpp
810–811

I'd suggest deleting this special case.
We no longer have two distinct states of our WorkspaceEdit struct to represent {changes:{}} and {}.

{changes:{}} is a well-defined empty edit, even if that never makes sense to actually send.
{} conforms to the typescript type definition of the spec, but doesn't actually define an edit per the spec text.

This revision is now accepted and ready to land.Jun 2 2021, 11:16 AM
kadircet updated this revision to Diff 349371.Jun 2 2021, 1:59 PM
kadircet marked an inline comment as done.

Get rid of the special case around empty changes.

This revision was landed with ongoing or failed builds.Jun 2 2021, 2:03 PM
This revision was automatically updated to reflect the committed changes.