clangd: IncludeInserter does not deal with directory symlinks within include directories (and VFS overlay workarounds are variously broken)

[lf-]

When using clangd, the auto-include suggestions are broken in a project like the following (which you can clone from https://git.jade.fyi/jade/clangd-bug-repro):

build.sh:

clang++ -std=c++20 -Iinclude/ src/foo.cc -c -o src/foo.cc.o

Generate a compile_commands.json via bear or so.

Tree:

.
├── build.sh
├── compile_commands.json
├── include
│   └── lib2 -> ../lib2
├── lib2
│   ├── bar.hh
│   └── foo.hh
└── src
    ├── foo.cc
    ├── nya.hh
    └── wat.hh

foo.cc:

#include "lib2/foo.hh"
#include "nya.hh"
#include "wat.hh"

void thingy() {
    frob();
    wat();
    lolwut();
}

lib2/foo.hh:

int frob();

lib2/bar.hh:

void showNyanCat();

Then open lib2/bar.hh in a new tab to make sure clangd knows about it. Tab-complete a call to showNyanCat(), which will not add the header include. By comparison, doing the same for a symbol in, e.g., wat.hh, will successfully add the include.

I have debugged this to find that HeaderPath::suggestPathToFileForDiagnostics returns an absolute path for the file in question, which is then promptly rejected downstream by clangd:

llvm-project/clang-tools-extra/clangd/Headers.cpp

Lines 292 to 306 in 3f156ef

	if (HeaderSearchInfo) {
	Suggested = HeaderSearchInfo->suggestPathToFileForDiagnostics(
	InsertedHeader.File, BuildDir, IncludingFile, &IsAngled);
	} else {
	// Calculate include relative to including file only.
	StringRef IncludingDir = llvm::sys::path::parent_path(IncludingFile);
	SmallString<256> RelFile(InsertedHeader.File);
	// Replacing with "" leaves "/RelFile" if IncludingDir doesn't end in "/".
	llvm::sys::path::replace_path_prefix(RelFile, IncludingDir, "./");
	Suggested = llvm::sys::path::convert_to_slash(
	llvm::sys::path::remove_leading_dotslash(RelFile));
	}
	// FIXME: should we allow (some limited number of) "../header.h"?
	if (llvm::sys::path::is_absolute(Suggested))
	return std::nullopt;

The proximate cause of the absolute paths from suggestPathToFileForDiagnostics is that there are no -I directories or parents of the file in question that are common prefixes of the canonical file path of the translation unit and the canonical path of the header file. Seemingly the only way to get this to work today is to make the canonical path of the header a child of the include directory. However, this requires rearranging source code around what is IMO a clang bug or at least deficiency.

Another alternative (which I have not tried, and which has kind of bad ergonomics since it is busted for newly created files) is generating a header map specifically for clangd since those are special cased in suggestPathToFileForDiagnostics. Yet another alternative which does work but is absurd is:

.clangd containing {"CompileFlags": {"Add": ["-I", ".include"], "Remove": ["-I", "../src/include"]}} and then running clangd in a bwrap that bindmounts .include/lix = src (in the actual problem in the field, the problem is that we have includes of the form lix/libfoo/bar.hh which need to resolve to src/libfoo/bar.hh, so just putting more ../ is not a fix).

I have tried the following VFS overlay:

version: 0
roots:
  - name: include/lib2
    type: directory-remap
    external-contents: lib2/
    # plus or minus the following line; same results either way
    # use-external-name: true

and then added via -Xclang -ivfsoverlay -Xclang ./vfsoverlay.yaml. This compiles fine but the include inserter still refuses to work for presumably the same reason. It appears that the remapping is treated as if it is a symlink rather than as a bind mount, I think. I have not shoved the thing into rr yet.

At this point I've spent enough time debugging and writing this up that I am looking for input on whether this is a completely busted use case or if I have missed something in this adventure.

[llvmbot]

@llvm/issue-subscribers-clangd

Author: jade (lf-)

When using clangd, the auto-include suggestions are broken in a project like the following (which you can clone from https://git.jade.fyi/jade/clangd-bug-repro):

build.sh:

clang++ -std=c++20 -Iinclude/ src/foo.cc -c -o src/foo.cc.o

Generate a compile_commands.json via bear or so.

Tree:

.
├── build.sh
├── compile_commands.json
├── include
│   └── lib2 -> ../lib2
├── lib2
│   ├── bar.hh
│   └── foo.hh
└── src
    ├── foo.cc
    ├── nya.hh
    └── wat.hh

foo.cc:

#include "lib2/foo.hh"
#include "nya.hh"
#include "wat.hh"

void thingy() {
    frob();
    wat();
    lolwut();
}

lib2/foo.hh:

int frob();

lib2/bar.hh:

void showNyanCat();

Then open lib2/bar.hh in a new tab to make sure clangd knows about it. Tab-complete a call to showNyanCat(), which will not add the header include. By comparison, doing the same for a symbol in, e.g., wat.hh, will successfully add the include.

I have debugged this to find that HeaderPath::suggestPathToFileForDiagnostics returns an absolute path for the file in question, which is then promptly rejected downstream by clangd:

llvm-project/clang-tools-extra/clangd/Headers.cpp

Lines 292 to 306 in 3f156ef

	if (HeaderSearchInfo) {
	Suggested = HeaderSearchInfo->suggestPathToFileForDiagnostics(
	InsertedHeader.File, BuildDir, IncludingFile, &IsAngled);
	} else {
	// Calculate include relative to including file only.
	StringRef IncludingDir = llvm::sys::path::parent_path(IncludingFile);
	SmallString<256> RelFile(InsertedHeader.File);
	// Replacing with "" leaves "/RelFile" if IncludingDir doesn't end in "/".
	llvm::sys::path::replace_path_prefix(RelFile, IncludingDir, "./");
	Suggested = llvm::sys::path::convert_to_slash(
	llvm::sys::path::remove_leading_dotslash(RelFile));
	}
	// FIXME: should we allow (some limited number of) "../header.h"?
	if (llvm::sys::path::is_absolute(Suggested))
	return std::nullopt;

The proximate cause of the absolute paths from suggestPathToFileForDiagnostics is that there are no -I directories or parents of the file in question that are common prefixes of the canonical file path of the translation unit and the canonical path of the header file. Seemingly the only way to get this to work today is to make the canonical path of the header a child of the include directory. However, this requires rearranging source code around what is IMO a clang bug or at least deficiency.

Another alternative (which I have not tried, and which has kind of bad ergonomics since it is busted for newly created files) is generating a header map specifically for clangd since those are special cased in suggestPathToFileForDiagnostics. Yet another alternative which does work but is absurd is:

.clangd containing {"CompileFlags": {"Add": ["-I", ".include"], "Remove": ["-I", "../src/include"]}} and then running clangd in a bwrap that bindmounts .include/lix = src (in the actual problem in the field, the problem is that we have includes of the form lix/libfoo/bar.hh which need to resolve to src/libfoo/bar.hh, so just putting more ../ is not a fix).

I have tried the following VFS overlay:

version: 0
roots:
  - name: include/lib2
    type: directory-remap
    external-contents: lib2/
    # plus or minus the following line; same results either way
    # use-external-name: true

and then added via -Xclang -ivfsoverlay -Xclang ./vfsoverlay.yaml. This compiles fine but the include inserter still refuses to work for presumably the same reason. It appears that the remapping is treated as if it is a symlink rather than as a bind mount, I think. I have not shoved the thing into rr yet.

At this point I've spent enough time debugging and writing this up that I am looking for input on whether this is a completely busted use case or if I have missed something in this adventure.

[HighCommander4]

Potentially related issue: clangd/clangd#124

[lf-]

Indeed! That issue is slightly different because the include directory itself is a symlink there, but mine is a bit harder.

I think the most plausible heuristic is to observe how includes via -I are used in other files and then, for each canonical directory accessed by crossing a symlink, store a map of include-dir to subpath used to reference that canonical directory in includes. Then when looking how to reference files by canonical path, additionally consider the ways to reference each of their parent directories as candidate paths in the same manner as today.

For example you'd handle #include "lix/foo.hh" by, during the canonicalization of the path once the concrete header has been found, noting down in a map that /path/to/src (the target of the lix symlink) can be accessed through my-include-dir with the subpath /lix.

During the reverse direction lookup, one has to simply check for all the parents of the canonical path of the header being searched for as entries in this map and then if any hit, try each of the -I paths in the second level of map and if that hits, you have a candidate that can be considered for length heuristics etc in the same way as a -I path itself.

Handling symlinks by any means other than following ones that the user specifies by some means is liable to be pretty algorithmically fraught in terms of symlink loops or pointless filesystem traversal and have a lot of inconvenient edge cases compared to the current search which is basically just checking all -I paths for being the parent of the canonical path of the file. So this approach of looking at what symlink access is already used seems likely to be safe and not result in doing, possibly, any more filesystem operations than today.