[clangd] Fix tests with forward slash as separator on windows

[mstorsjo]

Summary

If running on Windows with a build configured to prefer forward
slashes, these changes fix the tests.

[sammccall]

TL;DR: clangd is unsupported/broken on mingw, this patch only makes the tests pass.
The current maintainers don't plan to invest in this, or accept much extra complexity to support it.
If you or someone wants to find a clean way to support the configuration, it'd be great. Otherwise making the tests pass has little intrinsic value.

---

It sounds like this is the build option WINDOWS_PREFER_FORWARD_SLASH_DEFAULT, which is set for mingw builds (and could be set manually too).

I think the current state is:

• clangd+mingw is effectively unsupported (no bots, no official binaries, no devs who can verify bugs)
• clangd has path-handling problems when built under mingw.
• this patch doesn't fix clangd, it only makes the tests pass. Our tests mostly attempt to be "cross-platform", path-handling problems turn up as weird interactions between tools that get polished though bug reports.

Supporting windows-forward-slash paths would probably be a quite an effort (based on the effort of supporting "standard" windows paths).
This includes code changes, setting up testing infrastructure, refining behavior through bug reports, the ongoing cost of dealing with more complex/subtle path handling code.

I'm not sure this effort is justified by the impact. I'm open to ideas about how to support this but can't dedicate my own time to it. I am opposed to making the code much more complex/fragile to deal with it.
If someone's interested in maintaining and testing this configuration and can find good ways to encapsulate the extra complexity, it'd be great to support this.
Unless we have a plan to support it I don't think we should go to any lengths to make the tests pass.

(The current state of "it builds but doesn't work or pass tests" is bad, maybe we should be disabling clangd in CMake like we do when threads are disabled).

clang-tools-extra/clangd/tool/ClangdMain.cpp

545

This change is an example of something subtle that I don't understand, and don't expect other maintainers to understand, which is therefore fragile.

clang-tools-extra/clangd/unittests/TestFS.h

79

If we want to make this more broadly compatible, we'd need to finder cleaner/less invasive ways...

clang-tools-extra/clangd/unittests/URITests.cpp

137

This seems incorrect. Any build running on windows should be able to handle backslash paths read from compile_commands.json etc.

[mstorsjo]

FWIW, this change is not about mingw, it's about making the windows-with-forward-slashes configuration usable.

The windows-with-forward-slashes configuration works just as fine in MSVC configurations, and that's where I've tested it. A MSVC build with ninja check-all that succeeded before, still succeed with -DLLVM_ WINDOWS_PREFER_FORWARD_SLASH=ON (with respect to clangd) after this change.

As for testing strategy, it could be concievable to add a configuration to e.g. the buildkite premerge tests that build+test everything in a -DLLVM_WINDOWS_PREFER_FORWARD_SLASH=ON environment too. (I've seen interest from @rnk to push towards such a setup, possibly, so adding testing configurations for both modes is probably not inconcievable.)

clang-tools-extra/clangd/tool/ClangdMain.cpp

545

Hmm, I'll have to revisit this particular change and see if it really was necessary in the end, and add a comment explaining the subtlety.

clang-tools-extra/clangd/unittests/TestFS.h

79

Sure, this is admittedly a bit hacky, but managed to make tests pass with a fairly minimal amount of changes.

clang-tools-extra/clangd/unittests/URITests.cpp

137

Yes, but that's not what the changes does. Regardless of the preference for forward or backwards slashes, both configurations (at least when it comes to LLVMSupport general functions) support both types of paths, that's not changed.

Previously, this test verified that if you resolve file:///c%a/x/y/z you get c:\\x\\y\\z. Now in a forward slash environment, you get c:/x/y/z - which is expected.

[sammccall]

Ooh sorry, I was missing some context.
I see now that LLVM_WINDOWS_PREFER_FORWARD_SLASH, windows_backslash etc are new work. And maybe buildbots are coming?

It's possible that the combination of our windows support plus improved llvm::sys::path will make things work.
But it's also likely there's rough edges to work out, and this will lead to bug reports we can't reproduce or understand.

[mstorsjo]

I forgot to add - I agree with the general sentiment though, that it’s not worth bending over backwards to make tests pass in a setup that’s known not to work in real use though.

Then again, if someone were to want to actually step up to make it work in mingw setups, I guess that shouldn’t be blocked either (although I can’t say I have the bandwidth to take on that - I’m not currently a user of clangd).

[sammccall]

In D113268#3111709, @mstorsjo wrote:

FWIW, this change is not about mingw, it's about making the windows-with-forward-slashes configuration usable.

OK - can I ask why the windows-with-forward-slashes configuration is valuable to support?
I was assuming it was mostly useful to folks building with mingw, sounds like that's not all!

The windows-with-forward-slashes configuration works just as fine in MSVC configurations, and that's where I've tested it. A MSVC build with ninja check-all that succeeded before, still succeed with -DLLVM_ WINDOWS_PREFER_FORWARD_SLASH=ON (with respect to clangd) after this change.

As for testing strategy, it could be concievable to add a configuration to e.g. the buildkite premerge tests that build+test everything in a -DLLVM_WINDOWS_PREFER_FORWARD_SLASH=ON environment too. (I've seen interest from @rnk to push towards such a setup, possibly, so adding testing configurations for both modes is probably not inconcievable.)

That's great to hear. It'd really be a prerequisite to keeping this configuration passing, as we don't have regular access to windows machines & rely on the buildbots to catch test problems.

I think the biggest testing problem we have though is that we don't have a good way of defining integration tests with paths set up the right way (since we can't really use absolute paths in our tests, and most of our bugs involve comparing absolute paths).

clang-tools-extra/clangd/unittests/URITests.cpp

137

Of course, sorry!

[mstorsjo]

Oh, I realized that issues relating to “mingw” probably are from msys (which often is used together, but is an entirely different thing) - yeah I can see that there’d be lots of unfixable issues with that, and I’m not signing up for looking into that…

Msys is the unix emulation layer/runtime, which uses unix style paths like /c/Windows. Mingw is just normal plain win32 with slightly different tools.

This patch and the new option is just about C:/Windows vs C:\Windows. Most win32 apis (and llvm in general) accept both forms, the new option is just about which one of the two is generated when llvm/clang generate paths.

[mstorsjo]

In D113268#3111798, @sammccall wrote:

In D113268#3111709, @mstorsjo wrote:

FWIW, this change is not about mingw, it's about making the windows-with-forward-slashes configuration usable.

OK - can I ask why the windows-with-forward-slashes configuration is valuable to support?
I was assuming it was mostly useful to folks building with mingw, sounds like that's not all!

Well you're right that my own original need is for mingw setups, with various tools parsing and using the output of e.g. clang -v and clang --print-resource-dir etc, and using it in contexts where backslashes require extra care. I'm not familiar with all the other cases that others have had that have indicated a need for this though, but it's something that does come up semi-regularly, and as Windows itself mostly accepts this alternative form, it's a setup with possibly "less sharp corners".

The windows-with-forward-slashes configuration works just as fine in MSVC configurations, and that's where I've tested it. A MSVC build with ninja check-all that succeeded before, still succeed with -DLLVM_ WINDOWS_PREFER_FORWARD_SLASH=ON (with respect to clangd) after this change.

As for testing strategy, it could be concievable to add a configuration to e.g. the buildkite premerge tests that build+test everything in a -DLLVM_WINDOWS_PREFER_FORWARD_SLASH=ON environment too. (I've seen interest from @rnk to push towards such a setup, possibly, so adding testing configurations for both modes is probably not inconcievable.)

That's great to hear. It'd really be a prerequisite to keeping this configuration passing, as we don't have regular access to windows machines & rely on the buildbots to catch test problems.

I think the biggest testing problem we have though is that we don't have a good way of defining integration tests with paths set up the right way (since we can't really use absolute paths in our tests, and most of our bugs involve comparing absolute paths).

Yup, I can totally understand that. And as I haven't tested clangd in this setup other than the unit tests, I'm not sure if it works entirely in practice though - with other unit tests in Clang I've seen lots of cases where e.g. paths aren't considered as in the same directory when paths are expressed with different separators.

For someone unfamiliar with the tool, is there a simple "smoke test procedure" I could try out with it to kick the tyres?

Anyway, I'll have a look at the seemingly odd/fragile change and get back to you on that.

[sammccall]

In D113268#3111995, @mstorsjo wrote:

I think the biggest testing problem we have though is that we don't have a good way of defining integration tests with paths set up the right way (since we can't really use absolute paths in our tests, and most of our bugs involve comparing absolute paths).

Yup, I can totally understand that. And as I haven't tested clangd in this setup other than the unit tests, I'm not sure if it works entirely in practice though - with other unit tests in Clang I've seen lots of cases where e.g. paths aren't considered as in the same directory when paths are expressed with different separators.

For someone unfamiliar with the tool, is there a simple "smoke test procedure" I could try out with it to kick the tyres?

There's clangd --check=some_file.cc which simulates opening up a file in an editor and clicking around in it.
It does require a compile_command.json in a parent directory to know about build flags.
To try it out for real you need to connect an editor to it with a plugin (like vscode-clangd).

But realistic problems with paths tend to come from interactions with external sources.
For example, we had a bug that involved:

• vscode sending paths like file://c:/test.cc (lowercase C), which made its way into our AST cache
• cmake creating compile_commands.json like C:\test.cc (uppercase C), which made its way into our index
• when renaming a symbol, results from the two weren't deduplicated against each other so the edit was applied twice

Anyway, I'll have a look at the seemingly odd/fragile change and get back to you on that.

Thanks!

[mstorsjo]

clang-tools-extra/clangd/tool/ClangdMain.cpp

545

Ah, yes, now I remember what this particular change does:

TestScheme::TestDir is a haredcoded constant string; fs::make_absolute(TestScheme::TestDir, Path); concatenates the always-backslashes TestDir with Path (with the currently preferred separator inbetween). By calling native() afterwards, it converts the whole resulting path to the native form.

An alternative would be to provide TestDir in the preferred form (e.g. like wrapping it in a function that returns a variable string, like in TestFS.{h,cpp}.) - that'd maybe be more straightforward, but also feels a bit needless as we're calling native() on it anyway.

[mstorsjo]

Added a comment about the surprising change where the order of append and native is switched. Made the modification to testRoot() less hacky.

[mstorsjo]

Sorry for taking so long to get back to you on this matter...

In D113268#3112026, @sammccall wrote:

For someone unfamiliar with the tool, is there a simple "smoke test procedure" I could try out with it to kick the tyres?

There's clangd --check=some_file.cc which simulates opening up a file in an editor and clicking around in it.
It does require a compile_command.json in a parent directory to know about build flags.
To try it out for real you need to connect an editor to it with a plugin (like vscode-clangd).

Thanks! I tested both clangd --check=some_file.cc and vscode-clangd with a build of clangd set to prefer forward slashes, and it all worked fine so far. I didn't do any exhaustive use of it in vscode, but at least regular use seemed just fine.

But realistic problems with paths tend to come from interactions with external sources.
For example, we had a bug that involved:

• vscode sending paths like file://c:/test.cc (lowercase C), which made its way into our AST cache
• cmake creating compile_commands.json like C:\test.cc (uppercase C), which made its way into our index
• when renaming a symbol, results from the two weren't deduplicated against each other so the edit was applied twice

Yup - I've seen lots of similar issues in other clang tests too. The best way forward to fix those would probably be to add some helper to llvm::sys::path for doing path comparisons, which would ignore case differences (on windows and macos) and/or separator style differences (on windows). But

Anyway, this change (and a build that prefers generating paths with forward slashes) seems to work as expected with regards to that - and I updated the patch with what was requested.

The whole setup for this mode isn't settled, so there's no buildbot for this mode (yet), we'll see if we end up setting one up. In any case, most of this change shouldn't be too intrusive I hope.

clang-tools-extra/clangd/tool/ClangdMain.cpp

getAbsolutePath(llvm::StringRef /*Authority*/, llvm::StringRef Body,
     // Still require "/" in body to mimic file scheme, as we want lengths of an
     // equivalent URI in both schemes to be the same.
     if (!Body.startswith("/"))
       return error(
           "Expect URI body to be an absolute path starting with '/': {0}",
           Body);
     Body = Body.ltrim('/');
     llvm::SmallString<16> Path(Body);
-    path::native(Path);
     fs::make_absolute(TestScheme::TestDir, Path);
+    // Convert the whole path to native separators after concatenating with
+    // TestDir; TestDir might contain separators other than the preferred
+    // on Windows if forward slashes are preferred.
+    path::native(Path);
     return std::string(Path);
   }
 
   llvm::Expected<URI>
   uriFromAbsolutePath(llvm::StringRef AbsolutePath) const override {
     llvm::StringRef Body = AbsolutePath;
-    if (!Body.consume_front(TestScheme::TestDir))
+    llvm::SmallString<16> NativeTestDir(TestDir);
+    llvm::sys::path::native(NativeTestDir);
+    if (!Body.consume_front(NativeTestDir))
       return error("Path {0} doesn't start with root {1}", AbsolutePath,
                    TestDir);
 
     return URI("test", /*Authority=*/"",
                llvm::sys::path::convert_to_slash(Body));
   }
 
 private:

clang-tools-extra/clangd/unittests/TestFS.h

public:
   std::vector<std::string> ExtraClangFlags;
 
 private:
   StringRef Directory;
   StringRef RelPathPrefix;
 };
 
 // Returns an absolute (fake) test directory for this OS.
-const char *testRoot();
+std::string testRoot();
 
 // Returns a suitable absolute path for this OS.
 std::string testPath(PathRef File,
                      llvm::sys::path::Style = llvm::sys::path::Style::native);
 
 // unittest: is a scheme that refers to files relative to testRoot()
 // This anchor is used to force the linker to link in the generated object file
 // and thus register unittest: URI scheme plugin.
 extern volatile int UnittestSchemeAnchorSource;
 
 } // namespace clangd
 } // namespace clang
 #endif

clang-tools-extra/clangd/unittests/TestFS.cpp

MockCompilationDatabase::getCompileCommand(PathRef File) const {
   }
 
   return {tooling::CompileCommand(Directory != llvm::StringRef()
                                       ? Directory
                                       : llvm::sys::path::parent_path(File),
                                   FileName, std::move(CommandLine), "")};
 }
 
-const char *testRoot() {
+std::string testRoot() {
 #ifdef _WIN32
-  return "C:\\clangd-test";
+  llvm::SmallString<16> NativeRoot("C:\\clangd-test");
+  llvm::sys::path::native(NativeRoot);
+  return std::string(NativeRoot);
 #else
   return "/clangd-test";
 #endif
 }
 
 std::string testPath(PathRef File, llvm::sys::path::Style Style) {
   assert(llvm::sys::path::is_relative(File) && "FileName should be relative");
 

clang-tools-extra/clangd/unittests/URITests.cpp

TEST(URITest, ParseFailed) {
   // Empty.
   EXPECT_ERROR(URI::parse(""));
   EXPECT_ERROR(URI::parse(":/a/b/c"));
   EXPECT_ERROR(URI::parse("\"/a/b/c\" IWYU pragma: abc"));
 }
 
 TEST(URITest, Resolve) {
 #ifdef _WIN32
-  EXPECT_THAT(resolveOrDie(parseOrDie("file:///c%3a/x/y/z")), "c:\\x\\y\\z");
-  EXPECT_THAT(resolveOrDie(parseOrDie("file:///c:/x/y/z")), "c:\\x\\y\\z");
+  llvm::SmallString<16> Ref("c:\\x\\y\\z");
+  llvm::sys::path::native(Ref);
+  EXPECT_THAT(resolveOrDie(parseOrDie("file:///c%3a/x/y/z")), Ref);
+  EXPECT_THAT(resolveOrDie(parseOrDie("file:///c:/x/y/z")), Ref);
 #else
   EXPECT_EQ(resolveOrDie(parseOrDie("file:/a/b/c")), "/a/b/c");
   EXPECT_EQ(resolveOrDie(parseOrDie("file://auth/a/b/c")), "//auth/a/b/c");
   EXPECT_THAT(resolveOrDie(parseOrDie("file://au%3dth/%28x%29/y/%20z")),
               "//au=th/(x)/y/ z");
   EXPECT_THAT(resolveOrDie(parseOrDie("file:///c:/x/y/z")), "c:/x/y/z");
 #endif
   EXPECT_EQ(resolveOrDie(parseOrDie("unittest:///a"), testPath("x")),
             testPath("a"));
 }
 
 TEST(URITest, ResolveUNC) {
 #ifdef _WIN32
+  llvm::SmallString<32> RefExample("\\\\example.com\\x\\y\\z");
+  llvm::sys::path::native(RefExample);
+  llvm::SmallString<16> RefLocalhost("\\\\127.0.0.1\\x\\y\\z");
+  llvm::sys::path::native(RefLocalhost);
   EXPECT_THAT(resolveOrDie(parseOrDie("file://example.com/x/y/z")),
-              "\\\\example.com\\x\\y\\z");
+              RefExample);
   EXPECT_THAT(resolveOrDie(parseOrDie("file://127.0.0.1/x/y/z")),
-              "\\\\127.0.0.1\\x\\y\\z");
+              RefLocalhost);
   // Ensure non-traditional file URI still resolves to correct UNC path.
   EXPECT_THAT(resolveOrDie(parseOrDie("file:////127.0.0.1/x/y/z")),
-              "\\\\127.0.0.1\\x\\y\\z");
+              RefLocalhost);
 #else
   EXPECT_THAT(resolveOrDie(parseOrDie("file://example.com/x/y/z")),
               "//example.com/x/y/z");
   EXPECT_THAT(resolveOrDie(parseOrDie("file://127.0.0.1/x/y/z")),
               "//127.0.0.1/x/y/z");
 #endif
 }