[clangd] Add option to always insert headers with <> instead of ""

[ADKaster]

Summary

Projects can now add the following config fragment to their .clangd:

Style:
  IncludeDelimiter: AlwaysBrackets

to force headers inserted via the --header-insertion=iwyu mode to have
<> around them in all cases, rather than relying on whether the header
was included via -isystem or not.

Ref https://github.com/clangd/clangd/issues/1247

This solution does not allow forcing "" in all cases, nor does it affect
other clang tools like clang-format.

[ADKaster]

If this patch is accepted, I don't have commit access. If someone could commit as "Andrew Kaster <[e-mail]>" that would be much appreciated.

[nridge]

Thanks for the patch!

May I suggest the flag name IncludeDelimiter, with values Auto and AlwaysBrackets (with the default being Auto)? This leaves room for other styles in the future if desired.

It would mean a slight implementation change since the YAML representation would now be a string rather than a bool, but nothing too involved; you can use the existing code for parsing IndexBlock::Background (whose allowed values are Build and Skip) as a guide.

[ADKaster]

Change from a boolean parameter to an enum.

[ADKaster]

clang-tools-extra/clangd/Headers.cpp

353

I couldn't tell looking at other code in clangd whether it would be preferred to fetch the value from Config::current() here every time, or if having the option saved as a member variable of the IncludeInserter is fine.

[kadircet]

sorry but I am not sure what's the value proposed by this patch in its current form. in https://github.com/clangd/clangd/issues/1247 and other places we've discussed this, i believe the sentiment was towards providing a config option that'll let people customize header insertion style in combination with a directory filter, e.g:

Style:
  IncludeInsertion:
      Directory: foo/
      Delimeter: <
   ... More IncludeInsertion customizations.

Any specific reasons for going with the hard-coded approach? As I don't think we can accept all this extra code to make sure it works for a "handful" projects/users (in the wild I haven't seen many projects that strictly use < includes). The use case you want can still be accommodated by such a generic approach, you can have Directory: / or we can even treat absence of Directory field as an "always" matcher.

[nridge]

My understanding is that a more elaborate configuration scheme has been proposed in https://github.com/clangd/clangd/issues/1367, and the feedback there was (quoting Sam from this comment):

I think this design is very complicated for the value it provides. [...]
For <> vs "" in particular, we do have multiple reports suggesting this. I think we should be open to a either a simple config-based solution

The approach taken in this patch seemed to me to be in line with this direction of a "simple config-based solution".

[kadircet]

In D145843#4207101, @nridge wrote:

My understanding is that a more elaborate configuration scheme has been proposed in https://github.com/clangd/clangd/issues/1367, and the feedback there was (quoting Sam from this comment):
The approach taken in this patch seemed to me to be in line with this direction of a "simple config-based solution".

I am afraid this approach is a little "too simple". The intent on @sammccall 's comment there is probably around getting rid of the re-writing of include spellings completely, not for specifying quoted or system includes based on the include spelling.
Even if not, I feel like my argument above still applies. I still can't think of many projects benefiting from always using angles/quotes for include spellings.

[sammccall]

In D145843#4207466, @kadircet wrote:

In D145843#4207101, @nridge wrote:

My understanding is that a more elaborate configuration scheme has been proposed in https://github.com/clangd/clangd/issues/1367, and the feedback there was (quoting Sam from this comment):
The approach taken in this patch seemed to me to be in line with this direction of a "simple config-based solution".

I am afraid this approach is a little "too simple". The intent on @sammccall 's comment there is probably around getting rid of the re-writing of include spellings completely, not for specifying quoted or system includes based on the include spelling.
Even if not, I feel like my argument above still applies. I still can't think of many projects benefiting from always using angles/quotes for include spellings.

Right - my impression is that most projects that want to force a certain quote style want to do so for certain paths only.

e.g. from #1367:

I'm working on a project where the style dictates that all the includes from our public SDK are included using angle brackets.

A recent comment on #1247:

Since clangd works with little configuring for SerenityOS, we do not need to modify -isystem which is picked up from our toolchain. However, those includes do not include all the headers we include with the bracketed style, so a better way of configuring this is needed.

So while I do think it's important to limit the complexity, a version of the feature that isn't actually usable to most people would just be an attractive nuisance.

I'd suggest something like:

Style:
  QuotedHeaders: "path/to/.*"
  AngledHeaders: "path/sdk/.*"

where the regexes can be single or array, and match any suffix (like Diagnostics.Includes.IgnoreHeader)

[nridge]

Thanks for the clarification and suggested formulation.

In D145843#4209750, @sammccall wrote:

I'd suggest something like:

Style:
  QuotedHeaders: "path/to/.*"
  AngledHeaders: "path/sdk/.*"

where the regexes can be single or array, and match any suffix (like Diagnostics.Includes.IgnoreHeader)

@ADKaster are you interested in updating the patch to implement this approach?

[nridge]

In D145843#4221826, @nridge wrote:

Thanks for the clarification and suggested formulation.

In D145843#4209750, @sammccall wrote:

I'd suggest something like:

Style:
  QuotedHeaders: "path/to/.*"
  AngledHeaders: "path/sdk/.*"

where the regexes can be single or array, and match any suffix (like Diagnostics.Includes.IgnoreHeader)

@ADKaster are you interested in updating the patch to implement this approach?

It looks like someone has done this and submitted the updated patch as a PR, linking to it for reference: https://github.com/llvm/llvm-project/pull/67749

clang-tools-extra/clangd/CodeComplete.cpp

 // it's purely AST-based, and there are few candidates.
 //
 //===----------------------------------------------------------------------===//
 
 #include "CodeComplete.h"
 #include "AST.h"
 #include "CodeCompletionStrings.h"
 #include "Compiler.h"
+#include "Config.h"
 #include "ExpectedTypes.h"
 #include "Feature.h"
 #include "FileDistance.h"
 #include "FuzzyMatch.h"
 #include "Headers.h"
 #include "Hover.h"
 #include "Preamble.h"
 #include "Protocol.h"
auto RecorderOwner = std::make_unique<CompletionRecorder>(Opts, [&]() {
           Recorder->CCSema->getSourceManager(), Recorder->CCSema->LangOpts);
       if (NextToken)
         NextTokenKind = NextToken->getKind();
       // If preprocessor was run, inclusions from preprocessor callback should
       // already be added to Includes.
       Inserter.emplace(
           SemaCCInput.FileName, SemaCCInput.ParseInput.Contents, Style,
           SemaCCInput.ParseInput.CompileCommand.Directory,
-          &Recorder->CCSema->getPreprocessor().getHeaderSearchInfo());
+          &Recorder->CCSema->getPreprocessor().getHeaderSearchInfo(),
+          Config::current().Style.AlwaysBracketedInclude);
       for (const auto &Inc : Includes.MainFileIncludes)
         Inserter->addExisting(Inc);
 
       // Most of the cost of file proximity is in initializing the FileDistance
       // structures based on the observed includes, once per query. Conceptually
       // that happens here (though the per-URI-scheme initialization is lazy).
       // The per-result proximity scoring is (amortized) very cheap.
       FileDistanceOptions ProxOpts{}; // Use defaults.
CodeCompleteResult runWithoutSema(llvm::StringRef Content, size_t Offset,
 
     llvm::StringMap<SourceParams> ProxSources;
     ProxSources[FileName].Cost = 0;
     FileProximity.emplace(ProxSources);
 
     auto Style = getFormatStyleForFile(FileName, Content, TFS);
     // This will only insert verbatim headers.
     Inserter.emplace(FileName, Content, Style,
-                     /*BuildDir=*/"", /*HeaderSearchInfo=*/nullptr);
+                     /*BuildDir=*/"", /*HeaderSearchInfo=*/nullptr,
+                     Config::current().Style.AlwaysBracketedInclude);
 
     auto Identifiers = collectIdentifiers(Content, Style);
     std::vector<RawIdentifier> IdentifierResults;
     for (const auto &IDAndCount : Identifiers) {
       RawIdentifier ID;
       ID.Name = IDAndCount.first();
       ID.References = IDAndCount.second;
       // Avoid treating typed filter as an identifier.

clang-tools-extra/clangd/Config.h

struct Config {
   } Diagnostics;
 
   /// Style of the codebase.
   struct {
     // Namespaces that should always be fully qualified, meaning no "using"
     // declarations, always spell out the whole name (with or without leading
     // ::). All nested namespaces are affected as well.
     std::vector<std::string> FullyQualifiedNamespaces;
+
+    // Inserted includes should always use <> over "", regardless of heuristics
+    bool AlwaysBracketedInclude = false;
   } Style;
 
   /// Configures code completion feature.
   struct {
     /// Whether code completion includes results that are not visible in current
     /// scopes.
     bool AllScopes = true;
   } Completion;

clang-tools-extra/clangd/ConfigCompile.cpp

if (!F.FullyQualifiedNamespaces.empty()) {
       Out.Apply.push_back([FullyQualifiedNamespaces(
                               std::move(FullyQualifiedNamespaces))](
                               const Params &, Config &C) {
         C.Style.FullyQualifiedNamespaces.insert(
             C.Style.FullyQualifiedNamespaces.begin(),
             FullyQualifiedNamespaces.begin(), FullyQualifiedNamespaces.end());
       });
     }
+    if (F.AlwaysBracketedInclude) {
+      if (auto Val = F.AlwaysBracketedInclude)
+        Out.Apply.push_back([Val](const Params &, Config &C) {
+          C.Style.AlwaysBracketedInclude = **Val;
+        });
+    }
   }
 
   void appendTidyCheckSpec(std::string &CurSpec,
                            const Located<std::string> &Arg, bool IsPositive) {
     StringRef Str = StringRef(*Arg).trim();
     // Don't support negating here, its handled if the item is in the Add or
     // Remove list.
     if (Str.startswith("-") || Str.contains(',')) {

clang-tools-extra/clangd/ConfigFragment.h

struct Fragment {
 
   // Describes the style of the codebase, beyond formatting.
   struct StyleBlock {
     // Namespaces that should always be fully qualified, meaning no "using"
     // declarations, always spell out the whole name (with or without leading
     // ::). All nested namespaces are affected as well.
     // Affects availability of the AddUsing tweak.
     std::vector<Located<std::string>> FullyQualifiedNamespaces;
+
+    // Inserted includes should always use <> over "", regardless of heuristics
+    std::optional<Located<bool>> AlwaysBracketedInclude;
   };
   StyleBlock Style;
 
   /// Describes code completion preferences.
   struct CompletionBlock {
     /// Whether code completion should include suggestions from scopes that are
     /// not visible. The required scope prefix will be inserted.
     std::optional<Located<bool>> AllScopes;

clang-tools-extra/clangd/ConfigYAML.cpp

private:
   }
 
   void parse(Fragment::StyleBlock &F, Node &N) {
     DictParser Dict("Style", this);
     Dict.handle("FullyQualifiedNamespaces", [&](Node &N) {
       if (auto Values = scalarValues(N))
         F.FullyQualifiedNamespaces = std::move(*Values);
     });
+    Dict.handle("AlwaysBracketedInclude", [&](Node &N) {
+      if (auto AlwaysBracketedInclude = boolValue(N, "AlwaysBracketedInclude"))
+        F.AlwaysBracketedInclude = *AlwaysBracketedInclude;
+    });
     Dict.parse(N);
   }
 
   void parse(Fragment::DiagnosticsBlock &F, Node &N) {
     DictParser Dict("Diagnostics", this);
     Dict.handle("Suppress", [&](Node &N) {
       if (auto Values = scalarValues(N))
         F.Suppress = std::move(*Values);

clang-tools-extra/clangd/Headers.h

 // Calculates insertion edit for including a new header in a file.
 class IncludeInserter {
 public:
   // If \p HeaderSearchInfo is nullptr (e.g. when compile command is
   // infeasible), this will only try to insert verbatim headers, and
   // include path of non-verbatim header will not be shortened.
   IncludeInserter(StringRef FileName, StringRef Code,
                   const format::FormatStyle &Style, StringRef BuildDir,
-                  HeaderSearch *HeaderSearchInfo)
+                  HeaderSearch *HeaderSearchInfo, bool AlwaysBracketedInclude)
       : FileName(FileName), Code(Code), BuildDir(BuildDir),
         HeaderSearchInfo(HeaderSearchInfo),
-        Inserter(FileName, Code, Style.IncludeStyle) {}
+        Inserter(FileName, Code, Style.IncludeStyle),
+        AlwaysBracketedInclude(AlwaysBracketedInclude) {}
 
   void addExisting(const Inclusion &Inc);
 
   /// Checks whether to add an #include of the header into \p File.
   /// An #include will not be added if:
   ///   - Either \p DeclaringHeader or \p InsertedHeader is already (directly)
   ///   in \p Inclusions (including those included via different paths).
   ///   - \p DeclaringHeader or \p InsertedHeader is the same as \p File.

 
 private:
   StringRef FileName;
   StringRef Code;
   StringRef BuildDir;
   HeaderSearch *HeaderSearchInfo = nullptr;
   llvm::StringSet<> IncludedHeaders; // Both written and resolved.
   tooling::HeaderIncludes Inserter;  // Computers insertion replacement.
+  bool AlwaysBracketedInclude = false;
 };
 
 } // namespace clangd
 } // namespace clang
 
 namespace llvm {
 
 // Support HeaderIDs as DenseMap keys.

clang-tools-extra/clangd/Headers.cpp

if (HeaderSearchInfo) {
     // 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;
-  if (IsSystem)
+  if (AlwaysBracketedInclude || IsSystem)
     Suggested = "<" + Suggested + ">";
   else
     Suggested = "\"" + Suggested + "\"";
   return Suggested;
 }
 
 std::optional<TextEdit>
 IncludeInserter::insert(llvm::StringRef VerbatimHeader,
                         tooling::IncludeDirective Directive) const {

clang-tools-extra/clangd/ParsedAST.cpp

if (PreserveDiags) {
 
     // Add IncludeFixer which can recover diagnostics caused by missing includes
     // (e.g. incomplete type) and attach include insertion fixes to diagnostics.
     if (Inputs.Index && !BuildDir.getError()) {
       auto Style =
           getFormatStyleForFile(Filename, Inputs.Contents, *Inputs.TFS);
       auto Inserter = std::make_shared<IncludeInserter>(
           Filename, Inputs.Contents, Style, BuildDir.get(),
-          &Clang->getPreprocessor().getHeaderSearchInfo());
+          &Clang->getPreprocessor().getHeaderSearchInfo(),
+          Cfg.Style.AlwaysBracketedInclude);
       ArrayRef<Inclusion> MainFileIncludes;
       if (Preamble) {
         MainFileIncludes = Preamble->Includes.MainFileIncludes;
         for (const auto &Inc : Preamble->Includes.MainFileIncludes)
           Inserter->addExisting(Inc);
       }
       // FIXME: Consider piping through ASTSignals to fetch this to handle the
       // case where a header file contains ObjC decls but no #imports.

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

 }
 
 TEST_F(ConfigCompileTests, Style) {
   Frag = {};
   Frag.Style.FullyQualifiedNamespaces.push_back(std::string("foo"));
   Frag.Style.FullyQualifiedNamespaces.push_back(std::string("bar"));
   EXPECT_TRUE(compileAndApply());
   EXPECT_THAT(Conf.Style.FullyQualifiedNamespaces, ElementsAre("foo", "bar"));
+
+  Frag = {};
+  EXPECT_TRUE(compileAndApply());
+  // Off by default.
+  EXPECT_EQ(Conf.Style.AlwaysBracketedInclude, false);
+
+  Frag.Style.AlwaysBracketedInclude.emplace(true);
+  EXPECT_TRUE(compileAndApply());
+  EXPECT_EQ(Conf.Style.AlwaysBracketedInclude, true);
+
+  Frag.Style.AlwaysBracketedInclude.emplace(false);
+  EXPECT_TRUE(compileAndApply());
+  EXPECT_EQ(Conf.Style.AlwaysBracketedInclude, false);
 }
 
 TEST_F(ConfigCompileTests, AllowDiagsFromStalePreamble) {
   Frag = {};
   EXPECT_TRUE(compileAndApply());
   // Off by default.
   EXPECT_EQ(Conf.Diagnostics.AllowStalePreamble, false);
 

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

Diagnostics:
   EXPECT_THAT(Results[0].Diagnostics.Includes.IgnoreHeader,
               ElementsAre(val("foo"), val("bar")));
 }
 
 TEST(ParseYAML, Style) {
   CapturedDiags Diags;
   Annotations YAML(R"yaml(
 Style:
-  FullyQualifiedNamespaces: [foo, bar])yaml");
+  FullyQualifiedNamespaces: [foo, bar]
+  AlwaysBracketedInclude: Yes)yaml");
   auto Results =
       Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback());
   ASSERT_THAT(Diags.Diagnostics, IsEmpty());
   ASSERT_EQ(Results.size(), 1u);
   EXPECT_THAT(Results[0].Style.FullyQualifiedNamespaces,
               ElementsAre(val("foo"), val("bar")));
+  EXPECT_THAT(Results[0].Style.AlwaysBracketedInclude,
+              llvm::ValueIs(val(true)));
 }
 
 TEST(ParseYAML, DiagnosticsMode) {
   CapturedDiags Diags;
   {
     Annotations YAML(R"yaml(
 Diagnostics:
   AllowStalePreamble: Yes)yaml");

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

if (Preferred.empty())
       Preferred = Original;
     auto ToHeaderFile = [](llvm::StringRef Header) {
       return HeaderFile{std::string(Header),
                         /*Verbatim=*/!llvm::sys::path::is_absolute(Header)};
     };
 
     IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
                              CDB.getCompileCommand(MainFile)->Directory,
-                             &Clang->getPreprocessor().getHeaderSearchInfo());
+                             &Clang->getPreprocessor().getHeaderSearchInfo(),
+                             AlwaysBracketedInclude);
     for (const auto &Inc : Inclusions)
       Inserter.addExisting(Inc);
     auto Inserted = ToHeaderFile(Preferred);
     if (!Inserter.shouldInsertInclude(Original, Inserted))
       return "";
     auto Path = Inserter.calculateIncludePath(Inserted, MainFile);
     Action.EndSourceFile();
     return Path.value_or("");
   }
 
   std::optional<TextEdit> insert(llvm::StringRef VerbatimHeader,
                                  tooling::IncludeDirective Directive) {
     Clang = setupClang();
     PreprocessOnlyAction Action;
     EXPECT_TRUE(
         Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0]));
 
     IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
                              CDB.getCompileCommand(MainFile)->Directory,
-                             &Clang->getPreprocessor().getHeaderSearchInfo());
+                             &Clang->getPreprocessor().getHeaderSearchInfo(),
+                             AlwaysBracketedInclude);
     auto Edit = Inserter.insert(VerbatimHeader, Directive);
     Action.EndSourceFile();
     return Edit;
   }
 
   MockFS FS;
   MockCompilationDatabase CDB;
   std::string MainFile = testPath("main.cpp");
   std::string Subdir = testPath("sub");
   std::string SearchDirArg = (llvm::Twine("-I") + Subdir).str();
+  bool AlwaysBracketedInclude = false;
   IgnoringDiagConsumer IgnoreDiags;
   std::unique_ptr<CompilerInstance> Clang;
 };
 
 MATCHER_P(written, Name, "") { return arg.Written == Name; }
 MATCHER_P(resolved, Name, "") { return arg.Resolved == Name; }
 MATCHER_P(includeLine, N, "") { return arg.HashLine == N; }
 MATCHER_P(directive, D, "") { return arg.Directive == D; }
EXPECT_THAT(
       UnorderedElementsAre(AllOf(written("\"foo.h\""), hasPragmaKeep(false)),
                            AllOf(written("\"bar.h\""), hasPragmaKeep(true))));
 }
 
 TEST_F(HeadersTest, InsertInclude) {
   std::string Path = testPath("sub/bar.h");
   FS.Files[Path] = "";
   EXPECT_EQ(calculate(Path), "\"bar.h\"");
+
+  AlwaysBracketedInclude = true;
+  EXPECT_EQ(calculate(Path), "<bar.h>");
 }
 
 TEST_F(HeadersTest, DoNotInsertIfInSameFile) {
   MainFile = testPath("main.h");
   EXPECT_EQ(calculate(MainFile), "");
 }
 
 TEST_F(HeadersTest, DoNotInsertOffIncludePath) {
   MainFile = testPath("sub/main.cpp");
   EXPECT_EQ(calculate(testPath("sub2/main.cpp")), "");
 }
 
 TEST_F(HeadersTest, ShortenIncludesInSearchPath) {
   std::string BarHeader = testPath("sub/bar.h");
   EXPECT_EQ(calculate(BarHeader), "\"bar.h\"");
 
   SearchDirArg = (llvm::Twine("-I") + Subdir + "/..").str();
   CDB.ExtraClangFlags = {SearchDirArg.c_str()};
   BarHeader = testPath("sub/bar.h");
   EXPECT_EQ(calculate(BarHeader), "\"sub/bar.h\"");
 }
 
+TEST_F(HeadersTest, ShortenIncludesInSearchPathBracketed) {
+  AlwaysBracketedInclude = true;
+  std::string BarHeader = testPath("sub/bar.h");
+  EXPECT_EQ(calculate(BarHeader), "<bar.h>");
+
+  SearchDirArg = (llvm::Twine("-I") + Subdir + "/..").str();
+  CDB.ExtraClangFlags = {SearchDirArg.c_str()};
+  BarHeader = testPath("sub/bar.h");
+  EXPECT_EQ(calculate(BarHeader), "<sub/bar.h>");
+}
+
 TEST_F(HeadersTest, ShortenedIncludeNotInSearchPath) {
   std::string BarHeader =
       llvm::sys::path::convert_to_slash(testPath("sub-2/bar.h"));
   EXPECT_EQ(calculate(BarHeader, ""), "\"sub-2/bar.h\"");
 }
 
 TEST_F(HeadersTest, PreferredHeader) {
   std::string BarHeader = testPath("sub/bar.h");
   EXPECT_EQ(calculate(BarHeader, "<bar>"), "<bar>");
 
   std::string BazHeader = testPath("sub/baz.h");
   EXPECT_EQ(calculate(BarHeader, BazHeader), "\"baz.h\"");
+
+  AlwaysBracketedInclude = true;
+  std::string BiffHeader = testPath("sub/biff.h");
+  EXPECT_EQ(calculate(BarHeader, BiffHeader), "<biff.h>");
 }
 
 TEST_F(HeadersTest, DontInsertDuplicatePreferred) {
   Inclusion Inc;
   Inc.Written = "\"bar.h\"";
   Inc.Resolved = "";
   EXPECT_EQ(calculate(testPath("sub/bar.h"), "\"bar.h\"", {Inc}), "");
   EXPECT_EQ(calculate("\"x.h\"", "\"bar.h\"", {Inc}), "");
TEST_F(HeadersTest, PreferInserted) {
   Edit = insert("\"header.h\"", tooling::IncludeDirective::Import);
   ASSERT_TRUE(Edit);
   EXPECT_EQ(Edit->newText, "#import \"header.h\"\n");
 }
 
 TEST(Headers, NoHeaderSearchInfo) {
   std::string MainFile = testPath("main.cpp");
   IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
-                           /*BuildDir=*/"", /*HeaderSearchInfo=*/nullptr);
+                           /*BuildDir=*/"", /*HeaderSearchInfo=*/nullptr,
+                           /*AlwaysBracketedInclude*/ false);
 
   auto HeaderPath = testPath("sub/bar.h");
   auto Inserting = HeaderFile{HeaderPath, /*Verbatim=*/false};
   auto Verbatim = HeaderFile{"<x>", /*Verbatim=*/true};
 
   EXPECT_EQ(Inserter.calculateIncludePath(Inserting, MainFile),
             std::string("\"sub/bar.h\""));
   EXPECT_EQ(Inserter.shouldInsertInclude(HeaderPath, Inserting), false);