[clangd] LSP extension to switch between source/header file

[Nebiroth]

Summary

Small extension to LSP to allow clients to use clangd to switch between C header files and source files.
Final version will use the completed clangd indexer to use the index of symbols to be able to switch from header to source file when the file names don't match.

[malaperle]

clangd/ClangdServer.cpp

293

this won't work for other extensions, we need to make this more generic. I believe you had a list of typical source files extensions and header extensions before?

295

we need to try if the file exists otherwise try other typical header extensions

297

needs to be more generic and handle more typical header extensions

test/clangd/hover.test

1 ↗

(On Diff #109110)

this is from another patch

[malaperle]

In the commit message, you could add that we will use and index of symbols later to be able to switch from header to source file when the file names don't match. This is more or less the justification of why we want this in Clangd and not on the client.

[arphaman]

clangd/ClangdServer.cpp

293

LLVM has a wonderful sys::path library for path manipulations. You could rewrite these checks like this:

if (llvm::sys::path::extension(path) == ".cpp") {
 SmallString<128> NewPath = path;
 llvm::sys::path::replace_extension(NewPath, "h");
 return "\"" + NewPath.str() + "\"";
}

[ilya-biryukov]

Also, +1 to the comments about file extensions, we have to cover at least .c, .cc and .cpp for source files, .h and .hpp for header files.

clangd/ClangdLSPServer.cpp

228

We should probably use Uri here.

clangd/ClangdServer.h

185

Please use descriptive typedefs for paths: Path (equivalent to std::string) for return type and PathRef (equivalent to StringRef) for parameter type.

185

Maybe add a small comment for this function to indicate it's a fairly trivial helper?

clangd/ProtocolHandlers.cpp

260

I don't see this method in the LSP spec.
Is this some kind of extension?

Could you add some comments on that with pointers to proposal/explanation of where this extension is used?

[Nebiroth]

[clangd] LSP extension to switch between source/header file

[ilya-biryukov]

This is probably not a final version, but I feel my comments should be helpful anyway.
Also, we're missing testcases now.

clangd/ClangdLSPServer.cpp

225

We use Params.uri.file, not Params.uri.uri when passing paths to clangd.
Also no need for extra local var: LangServer.Server.switchSourceHeader(Params.uri.file); should work.

225

Naming: local vars should start with capital letter.

clangd/ClangdServer.cpp

294

We should check all extensions in both upper-case and lower-case, not just .c and .C
(ideally, we could use case-insensitive comparisons).

301

path is already a StringRef, no need to convert it.
std::string is also implicitly convertible to StringRef, you could pass std::string to every function that accepts a StringRef

306

Maybe replace with:

if (std::find(DEFAULT_SOURCE_EXTESIONS, /*end iterator*/, llvm::sys::path::extensions(path)) {
  isSourceFile = true;
  foundExtension = true;
}

322

Maybe use std::find here too?

338

Don't need temp var here, there's StringRef::toVector method to convert StringRef to SmallString.

340

Naming: EXTENSIONS_ARRAY is a local var, use UpperCamelCase.

Maybe use ArrayRef<std::string> for ExtensionsArray instead?

359

Please use vfs::FileSystem instance, provided by FSProvider.getTaggedFileSystem() here for file accesses.

366

Don't add quotes here, Uri constructor and Uri::unparse is responsible for doing that.

clangd/ClangdServer.h

185

Maybe also change wording to indicate that this function does not mutate any state and is indeed trivial, i.e.:
Helper function that returns a path to the corresponding source file when given a header file and vice versa.

Code style: please end all comments with full stops.

186

Naming: parameter names must be capitalized

clangd/ProtocolHandlers.cpp

260

This comment was not addressed, probably marked as 'Done' by accident.

[malaperle]

clangd/ProtocolHandlers.cpp

260

Is this some kind of extension?

It's briefly mentioned in the summary but could use some more detail.
The first client to use it is Theia but I wonder if it would be easy to add it to the VS Code extension, as an example.

[Nebiroth]

Fixed diff comments.

[ilya-biryukov]

clangd/ClangdServer.cpp

294

It turns out there's a very simple way to compare case-insetively. StringRef(str).compare_lower(str2).
Could we use that instead of extension duplicates?

301

I don't think this comment was addressed. Why do we need pathDataRef variable?

[Nebiroth]

Fixed more diff comments.

[arphaman]

clangd/ClangdServer.cpp

296

You can use LLVM's function array_lengthof here and on the next line instead.

302

It might be better to use a StringSet instead of an array of strings and use one lowercase/uppercase lookup:

llvm::StringSet<> DEFAULT_SOURCE_EXTENSIONS[] = {".cpp", ".c", ".cc", ".cxx", ".c++", ".m", ".mm"};
// Handles both lower and uppercase extensions and source/header files in one if/else construct:
if (DEFAULT_SOURCE_EXTENSIONS.count(llvm::sys::path::extension(path).lower())) {
  ...
  isSourceFile = true;
} else if (DEFAULT_HEADER_EXTENSIONS.count(llvm::sys::path::extension(path).lower())) {
  ...
  isSourceFile = false;
}

[ilya-biryukov]

The function in ClangdServer seems to be getting more complicated and more complicated, mostly because of mixing up std::string, StringRef, SmallString.
Here's a slightly revamped implementation of your function, hope you'll find it (or parts of it) useful to simplify your implementation and address remaining comments.

// Note that the returned value is now llvm::Optional to clearly indicate no matching file was found.
llvm::Optional<Path> ClangdServer::switchSourceHeader(PathRef Path) {
  const StringRef SourceExtensions[] = {/*...*/; // only lower-case extensions
  const StringRef HeaderExtensions[] = {/*...*/}; // ...

  StringRef PathExt = llvm::sys::path::extension(Path);
  
  // Lookup in a list of known extensions.
  auto SourceIter = std::find(SourceExtensions, /*end iterator*/, PathExt, [](StringRef LHS, StringRef RHS) { return LHS.equals_lower(RHS); });
  bool IsSource = SourceIter != /*end iterator*/;
  
  // ...
  bool IsHeader = ...;

  // We can only switch between extensions known extensions.
  if (!IsSource && !IsHeader)
    return llvm::None;

  // Array to lookup extensions for the switch. An opposite of where original extension was found.
  ArrayRef<StringRef> NewExts = IsSource ? HeaderExtensions : SourceExtensions;

  // Storage for the new path.
  SmallString<128> NewPath;
  Path.toVector(NewPath);

  // Instance of vfs::FileSystem, used for file existence checks.
  auto FS = FSProvider.getTaggedFileSystem(Path).Value;

  // Loop through switched extension candidates.
  for (StringRef NewExt : NewExts) { 
    llvm::sys::path::replace_extension(NewPath, NewExt)
    if (FS->exists(NewPath))
      return NewPath.str().str(); // First str() to convert from SmallString to StringRef, second to convert from StringRef to std::string

    // Also check NewExt in upper-case, just in case.
    llvm::sys::path::replace_extension(NewPath, NewExt.upper())
    if (FS->exists(NewPath))
      return NewPath.str().str();
  }

  return llvm::None;
}

clangd/ClangdServer.cpp

404

No need to create a URI here, this should be handled outside ClangdServer, just return a path with replaced extension.

[Nebiroth]

Refactored switchSourceHeader function help from ilya
Added helper function to check for uppercase on current file.

[Nebiroth]

Remove unintentional file addition

Updating D36150: [clangd] LSP extension to switch between source/header file

ll#

[Nebiroth]

Some more code cleanup.

[ilya-biryukov]

Just a few more comments. Should be easy to fix.
Could you also please clang-format the code?

clangd/ClangdLSPServer.cpp

225

What if Result does not contain a value?

225

Use *Result instead of introducing an extra variable?

229

We should convert paths here. Convert to URI and use unparse.

Uri ResultUri = Uri::fromFile(*Result);
//....
        R"(,"result":)" + Uri::unparse(ResultUri) + R"(})");
//...

clangd/ClangdServer.cpp

324

Instead of an checkUpperCaseExtensions and two iterations use a single find_if and equals_lower:

// Checks for both uppercase and lowercase matches in a single loop over extensions array.
std::find_if(std::begin(SourceExtensions), std::end(SourceExtensions), [](PathRef SourceExt) {
  return SourceExt.equals_lower(PathExt);
});

362

Don't add "file://" or quoting here, simply return NewPath.

clangd/ClangdServer.h

186

Any reason not to put it into anonymous namespace inside .cpp file?

clangd/ProtocolHandlers.cpp

214

Code style: we don't use braces around small single-statement control structures

[Nebiroth]

Ran clang-format on modified files.
Minor refactoring.

[ilya-biryukov]

Overall the code looks good, thanks for the clean-ups.

Only a few minor style comments and a major one about return value when no file is matching.
Please add tests for the new feature.

clangd/ClangdLSPServer.cpp

223

Maybe remove this empty line?

227

Replace with more concise form: if (Result)

230

Running unparse on an instance created via URI::fromFile("") will result in "file:///".

Have you considered returning a list of paths as a result from switchHeaderSource?
That way you could capture the "no file matched" case with an empty list.
Later when an implementation will look into index, that would allow to return multiple source files for a header file, which also happens in some C++ projects.

Returning an empty string is also an option we could start with.

clangd/ClangdServer.cpp

22

Do we really need this using namespace or it can be deleted?

344

Code style: we don't use braces around small single-statement control structures

clangd/ClangdServer.h

185

Could you please clang-format this file too?
Comment line length is clearly over the limit.

[Nebiroth]

clangd/ClangdLSPServer.cpp

230

I think I will start by returning an empty string for now. Returning a list of paths sounds like a good idea once an indexer is implemented, but that would require changing some parts of the code like find_if which returns only the first instance of a match.

[Nebiroth]

Return value when no file is found is now an empty string instead of file://
Minor code style changes and cleanup.
Ran clang-format on ClangdServer.h

[ilya-biryukov]

Overall looks good, but still needs at least a few tests.

[Nebiroth]

In D36150#867971, @ilya-biryukov wrote:

Overall looks good, but still needs at least a few tests.

I have a test for this commit that uses included source and header files, would that be okay to do or should I generate files dynamically? If so, how can I accomplish this?

[ilya-biryukov]

In D36150#875623, @Nebiroth wrote:

In D36150#867971, @ilya-biryukov wrote:

Overall looks good, but still needs at least a few tests.

I have a test for this commit that uses included source and header files, would that be okay to do or should I generate files dynamically? If so, how can I accomplish this?

You are right, unfortunately we don't have a good story for multi-file tests yet.
Let's add a unit-test for the implementation in ClangdServer. This can be easily done by using MockFSProvider. You may find some examples in clang-tools-extra/unittest/clangd/ClangdTests.cpp

[Nebiroth]

Added unit test.

[ilya-biryukov]

Looks good modulo a few NITS.
Let me know if you need help landing this. (In case you don't have commit access, I can land it for you)

unittests/clangd/ClangdTests.cpp

910 ↗

(On Diff #116038)

Code style: local variables are UpperCamelCase

916 ↗

(On Diff #116038)

Code style: local variables are UpperCamelCase

967 ↗

(On Diff #116038)

The text is over the line limit.
Could you please clang-format every submission?

[Nebiroth]

Rebased on latest version.
Corrected code style issues in test file.

[ilya-biryukov]

Looks good.
Do you want me to submit this patch for you?

[Nebiroth]

In D36150#879194, @ilya-biryukov wrote:

Looks good.
Do you want me to submit this patch for you?

Yes please.

[malaperle]

I submitted the patch. Thanks a lot William and Ilya!

[ilya-biryukov]

Sorry for delays
@malaperle, thanks for submitting the patch.

clangd/ClangdLSPServer.cpp

public:
   void onDocumentFormatting(DocumentFormattingParams Params, StringRef ID,
                             JSONOutput &Out) override;
   void onCodeAction(CodeActionParams Params, StringRef ID,
                     JSONOutput &Out) override;
   void onCompletion(TextDocumentPositionParams Params, StringRef ID,
                     JSONOutput &Out) override;
   void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
                             JSONOutput &Out) override;
+  void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
+                    JSONOutput &Out) override;                          
 
 private:
   ClangdLSPServer &LangServer;
 };
 
 void ClangdLSPServer::LSPProtocolCallbacks::onInitialize(StringRef ID,
                                                          JSONOutput &Out) {
   Out.writeMessage(
void ClangdLSPServer::LSPProtocolCallbacks::onGoToDefinition(
   }
   if (!Locations.empty())
     Locations.pop_back();
   Out.writeMessage(
       R"({"jsonrpc":"2.0","id":)" + ID.str() +
       R"(,"result":[)" + Locations + R"(]})");
 }
 
+void ClangdLSPServer::LSPProtocolCallbacks::onSwitchSourceHeader(
+    TextDocumentIdentifier Params, StringRef ID, JSONOutput &Out) {
+
+  Path Result = LangServer.Server.switchSourceHeader(Params.uri.file);
+
+    Out.writeMessage(
+        R"({"jsonrpc":"2.0","id":)" + ID.str() +
+        R"(,"result":)" + Result + R"(})");
+}
+
 ClangdLSPServer::ClangdLSPServer(JSONOutput &Out, bool RunSynchronously)
     : Out(Out), DiagConsumer(*this),
       Server(CDB, DiagConsumer, FSProvider, RunSynchronously) {}
 
 void ClangdLSPServer::run(std::istream &In) {
   assert(!IsDone && "Run was called before");
 
   // Set up JSONRPCDispatcher.

clangd/ClangdServer.h

public:
   /// will be used.
   /// This method should only be called for currently tracked files.
   Tagged<std::vector<CompletionItem>>
   codeComplete(PathRef File, Position Pos,
                llvm::Optional<StringRef> OverridenContents = llvm::None);
   /// Get definition of symbol at a specified \p Line and \p Column in \p File.
   Tagged<std::vector<Location>> findDefinitions(PathRef File, Position Pos);
 
+  /// Helper function that returns a path to the corresponding source file when given a header file and vice versa.
+  Path switchSourceHeader(PathRef path);
+
   /// Run formatting for \p Rng inside \p File.
   std::vector<tooling::Replacement> formatRange(PathRef File, Range Rng);
   /// Run formatting for the whole \p File.
   std::vector<tooling::Replacement> formatFile(PathRef File);
   /// Run formatting after a character was typed at \p Pos in \p File.
   std::vector<tooling::Replacement> formatOnType(PathRef File, Position Pos);
 
   /// Gets current document contents for \p File. \p File must point to a

clangd/ClangdServer.cpp

 #include "ClangdServer.h"
 #include "clang/Format/Format.h"
 #include "clang/Frontend/ASTUnit.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 #include <future>
 
 using namespace clang;
 using namespace clang::clangd;
 
 namespace {
 
 std::vector<tooling::Replacement> formatCode(StringRef Code, StringRef Filename,
                                              ArrayRef<tooling::Range> Ranges) {
   // Call clang-format.
   // FIXME: Don't ignore style.
ClangdServer::findDefinitions(PathRef File, Position Pos) {
   std::vector<Location> Result;
   auto TaggedFS = FSProvider.getTaggedFileSystem(File);
   Units.runOnUnit(File, *FileContents.Draft, ResourceDir, CDB, PCHs,
       TaggedFS.Value, [&](ClangdUnit &Unit) {
         Result = Unit.findDefinitions(Pos);
       });
   return make_tagged(std::move(Result), TaggedFS.Tag);
 }
+
+Path ClangdServer::switchSourceHeader(PathRef path) {
+
+  std::string DEFAULT_SOURCE_EXTENSIONS[] = {
+      ".cpp", ".c", ".cc", ".cxx", ".c++", ".m", ".mm",
+      ".CPP", ".C", ".CC", ".CXX", ".C++", ".M", ".MM"};
+  std::string DEFAULT_HEADER_EXTENSIONS[] = {
+      ".h", ".hh", ".hpp", ".hxx", ".inc", ".H", ".HH", ".HPP", ".HXX", ".INC"};
+
+  const int sourceSize =
+      sizeof(DEFAULT_SOURCE_EXTENSIONS) / sizeof(DEFAULT_SOURCE_EXTENSIONS[0]);
+  const int headerSize =
+      sizeof(DEFAULT_HEADER_EXTENSIONS) / sizeof(DEFAULT_HEADER_EXTENSIONS[0]);
+
+  std::string pathDataRef = std::string(path);
+  bool isSourceFile = false, foundExtension = false;
+  SmallString<128> NewPath;
+  std::string *p;
+  p = std::find(DEFAULT_SOURCE_EXTENSIONS,
+                DEFAULT_SOURCE_EXTENSIONS + sourceSize,
+                llvm::sys::path::extension(path));
+  if (p != DEFAULT_SOURCE_EXTENSIONS + sourceSize) 
+  {
+    std::string fileExtension = *p;
+    NewPath = pathDataRef;
+    pathDataRef =
+        pathDataRef.substr(0, (pathDataRef.size() - fileExtension.size()));
+    isSourceFile = true;
+    foundExtension = true;
+  }
+
+  if (!foundExtension) {
+    p = std::find(DEFAULT_HEADER_EXTENSIONS,
+                  DEFAULT_HEADER_EXTENSIONS + headerSize,
+                  llvm::sys::path::extension(path));
+    if (p != DEFAULT_HEADER_EXTENSIONS + headerSize) {
+      std::string fileExtension = *p;
+      NewPath = pathDataRef;
+      pathDataRef =
+          pathDataRef.substr(0, (pathDataRef.size() - fileExtension.size()));
+      isSourceFile = false;
+      foundExtension = true;
+    }
+  }
+
+  SmallString<128> CurrentPath;
+  CurrentPath = std::string(pathDataRef);
+  bool done = false;
+  ArrayRef<std::string> ExtensionsArray;
+
+  if (!isSourceFile)
+    ExtensionsArray = DEFAULT_SOURCE_EXTENSIONS;
+  else
+    ExtensionsArray = DEFAULT_HEADER_EXTENSIONS;
+
+  int i = 0;
+  while (!done && i < ExtensionsArray.size()) {
+    std::string CurrentPathString = CurrentPath.str();
+    std::string NewPathString = NewPath.str();
+    if (i == 0)
+      CurrentPathString = std::string(pathDataRef).append(std::string(ExtensionsArray[i]));
+    else    
+      CurrentPathString = CurrentPathString.append(std::string(ExtensionsArray[i]));
+
+    FILE *file;    
+    auto test = FSProvider.getTaggedFileSystem(std::string(CurrentPathString));
+    bool fileExists = test.Value.get()->exists(std::string(CurrentPathString));
+    if (fileExists) {
+      done = true;
+      llvm::sys::path::replace_extension(NewPath, ExtensionsArray[i]);
+      std::string returnValue = "file://" + std::string(NewPath.str());
+      returnValue = URI::unparse(URI::fromUri(returnValue));
+      return Path(returnValue);
+    }
+    llvm::sys::path::replace_extension(CurrentPath, "");
+    CurrentPathString = CurrentPath.str();
+    i++;
+  }
+}

clangd/ProtocolHandlers.h

public:
   virtual void onDocumentRangeFormatting(DocumentRangeFormattingParams Params,
                                          StringRef ID, JSONOutput &Out) = 0;
   virtual void onCodeAction(CodeActionParams Params, StringRef ID,
                             JSONOutput &Out) = 0;
   virtual void onCompletion(TextDocumentPositionParams Params, StringRef ID,
                             JSONOutput &Out) = 0;
   virtual void onGoToDefinition(TextDocumentPositionParams Params, StringRef ID,
                             JSONOutput &Out) = 0;
+  virtual void onSwitchSourceHeader(TextDocumentIdentifier Params, StringRef ID,
+                            JSONOutput &Out) = 0;
 };
 
 void regiterCallbackHandlers(JSONRPCDispatcher &Dispatcher, JSONOutput &Out,
                              ProtocolCallbacks &Callbacks);
 
 } // namespace clangd
 } // namespace clang
 
 #endif

clangd/ProtocolHandlers.cpp

void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override {
 
     Callbacks.onGoToDefinition(*TDPP, ID, Output);
   }
 
 private:
   ProtocolCallbacks &Callbacks;
 };
 
+struct SwitchSourceHeaderHandler : Handler {
+  SwitchSourceHeaderHandler(JSONOutput &Output, ProtocolCallbacks &Callbacks)
+      : Handler(Output), Callbacks(Callbacks) {}
+
+  void handleMethod(llvm::yaml::MappingNode *Params, StringRef ID) override {
+    auto TDPP = TextDocumentIdentifier::parse(Params);
+    if (!TDPP) {
+      return;
+    }
+
+    Callbacks.onSwitchSourceHeader(*TDPP, ID, Output);
+  }
+
+private:
+  ProtocolCallbacks &Callbacks;
+};
+
 } // namespace
 
 void clangd::regiterCallbackHandlers(JSONRPCDispatcher &Dispatcher,
                                      JSONOutput &Out,
                                      ProtocolCallbacks &Callbacks) {
   Dispatcher.registerHandler(
       "initialize", llvm::make_unique<InitializeHandler>(Out, Callbacks));
   Dispatcher.registerHandler(
void clangd::regiterCallbackHandlers(JSONRPCDispatcher &Dispatcher,
   Dispatcher.registerHandler(
       "textDocument/codeAction",
       llvm::make_unique<CodeActionHandler>(Out, Callbacks));
   Dispatcher.registerHandler(
       "textDocument/completion",
       llvm::make_unique<CompletionHandler>(Out, Callbacks));
   Dispatcher.registerHandler("textDocument/definition",
       llvm::make_unique<GotoDefinitionHandler>(Out, Callbacks));
+  Dispatcher.registerHandler(
+      "textDocument/switchSourceHeader",
+      llvm::make_unique<SwitchSourceHeaderHandler>(Out, Callbacks));
 }