[clangd] IncludeCleaner missing includes inserts a different path than header-insertion

[torshepherd]

Basically, when I accept "add missing include" from the suggested fixes for IncludeCleaner, clangd inserts headers with various paths:

• just the filename (header.hpp)
• the full path from my desktop (with /home/tor.shepherd/root-dir/relative-path/header.hpp)

However, header-insertion on autocomplete consistently produces the correct path (relative-path/header.hpp) according to my codebase's guidelines.

(Note, both utilities produce the correct headers for stdlib includes, except IncludeCleaner produces a full path to the actual header file for using namespace std::chrono_literals; and autocomplete doesn't add any include for this; I would expect to be added)

[llvmbot]

@llvm/issue-subscribers-clangd

Author: Tor Shepherd (torshepherd)

Basically, when I accept "add missing include" from the suggested fixes for IncludeCleaner, clangd inserts headers with various paths:

• just the filename (header.hpp)
• the full path from my desktop (with /home/tor.shepherd/root-dir/relative-path/header.hpp)

However, header-insertion on autocomplete consistently produces the correct path (relative-path/header.hpp) according to my codebase's guidelines.

(Note, both utilities produce the correct headers for stdlib includes, except IncludeCleaner produces a full path to the actual header file for using namespace std::chrono_literals; and autocomplete doesn't add any include for this; I would expect <chrono> to be added)

[torshepherd]

Seems like IncludeCleaner uses include_cleaner::spellHeader but CodeComplete uses IncludeInserter::calculateIncludePath - which one is preferred? I think they should at least be consolidated/ perhaps a configuration value could choose which one to use

[llvmbot]

@llvm/issue-subscribers-clang-include-cleaner

Author: Tor Shepherd (torshepherd)

Basically, when I accept "add missing include" from the suggested fixes for IncludeCleaner, clangd inserts headers with various paths:

• just the filename (header.hpp)
• the full path from my desktop (with /home/tor.shepherd/root-dir/relative-path/header.hpp)

However, header-insertion on autocomplete consistently produces the correct path (relative-path/header.hpp) according to my codebase's guidelines.

(Note, both utilities produce the correct headers for stdlib includes, except IncludeCleaner produces a full path to the actual header file for using namespace std::chrono_literals; and autocomplete doesn't add any include for this; I would expect <chrono> to be added)

[idrilirdi]

Can confirm, exactly the same is happening to me. I'd prefer IncludeCleaner to return the path like CodeComplete, it's a pain to have to remove all the "/home/user/root_dir"s from the headers when using the code action

[torshepherd]

@idrilirdi I intend to take a look at this this month, please ping this issue to remind me

[idrilirdi]

@torshepherd friendly ping, are there any updates on this? :)

[torshepherd]

@torshepherd friendly ping, are there any updates on this? :)

Thanks for the ping! I haven't had a chance yet; been busy prepping for a talk on the 30th. I'll be looking at this starting after that :)

[torshepherd]

Ok I did a bit more digging and it seems like there was an idea that the spelling strategies could be smart/extensible: https://reviews.llvm.org/D150185

I wonder why they fallback to raw absolute paths then? Seems like there are a few issues with the strategies that I've encountered:

• Too eager to do relative includes (I. E. if you include a header from the same dir, you get "MyHeader.h"). This is not universally used. At my work, for instance, our guidelines prefer to always use a path relative to workspace root. So this spelling strategy is never what we want
• Too eager to fallback to absolute paths. I'm not really sure why this happens at all. This should almost never be the behavior. In fact, the default should just always be relative to base directory, no? I think the majority of codebases prefer this style.

Seems like the root issue is that the spelling strategies are not inferring usage from the codebase, but rather use a predefined hierarchy of strategies. If you just analyzed similar includes either in the same file or in different files, you could pretty easily see "ah this repository seems to use relative includes like "../MyFile.h" or "MyFile.h" for close includes, and "base/stuff/MyFile.h" for other subdirectories, etc.

CC @VitaNuova (who authored the IncludeSpeller PR) - are there plans to work more on this? If not can I take it up?