[clangd] Add fix-it for inserting IWYU pragma: keep

[njames93]

Summary

Since IncludeCleaner has some support for IWYU keep pragmas, we can suggest it as a fix-it.

[sammccall]

This is a bit tricky - we'd like to offer more fixes in several scenarios (e.g. replace a header with the transitive includes you're relying on), but some of those require more complex implementation.

On one hand, presenting only one automated fix will bias users towards that option.
On the other hand, it's not clear when the more complex fixes will be ready and this solves a real problem.

I'm inclined toward landing it, but will leave this up to @kadircet

clang-tools-extra/clangd/IncludeCleaner.cpp

502

this FIXME is addressed by this change

511

"directive" isn't accurate here I think

What about just: "add IWYU pragma: keep"?

[kadircet]

as discussed offline I agree that we should have this, as no matter how hard we try there are going to be cases that we can't get right due to ADL/template instantiations or depending on 3rd party code that cannot be edited and also doesn't have relevant pragmas inside.

my hesitation about having them at this stage is, we won't be able to:

• get feedback around most of these issues as people will just put a pragma and be done with it.
• fix them going forward, even after implementation gets better, we won't be able to diagnose these headers and "falsely" issued pragmas will keep includes around.

Hence i'd like to hear a little bit more about what kind of false positives you're facing often enough to need this as an automated fix at this stage, to see if there's something we can do to improve the implementation first.

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

1795

can you also have a test with an include that already has a trailing comment?

[njames93]

I had another idea about offering the export pragma when in a header file but I don't know if that's going too far

In D128204#3599286, @kadircet wrote:

Hence i'd like to hear a little bit more about what kind of false positives you're facing often enough to need this as an automated fix at this stage, to see if there's something we can do to improve the implementation first.

One of the main issues I have is due to templates not being instantiated which can result in symbols that are actually used not being picked up as the template instantiation that uses them isn't actually instantiated.
Unfortunately this isn't something that can be easily fixed without tanking performance.

[kadircet]

I had another idea about offering the export pragma when in a header file but I don't know if that's going too far

There are definitely cases where this is conceptually "right" fix, but it's hard to guess that in clangd (well at least without having some codebase wide analysis), and offering it all the time would definitely confuse users.

One of the main issues I have is due to templates not being instantiated which can result in symbols that are actually used not being picked up as the template instantiation that uses them isn't actually instantiated.

I guess you're talking about having the template pattern in the main file, but all the instantiations are in dependents of the code. My mental model was around making the dependents have the include for the type, instead of having it in the file providing the template pattern (header). Does that make sense?
This kind of breaks down when there's actually some types used by all the instantiations, as you'd want them to be included by the header. But I think we should be able to diagnose these cases, as usage of this type should be non-dependent (at least in the cases I can think of), so this sounds like a bug/improvement.

[njames93]

In D128204#3600973, @kadircet wrote:

One of the main issues I have is due to templates not being instantiated which can result in symbols that are actually used not being picked up as the template instantiation that uses them isn't actually instantiated.

I guess you're talking about having the template pattern in the main file, but all the instantiations are in dependents of the code. My mental model was around making the dependents have the include for the type, instead of having it in the file providing the template pattern (header). Does that make sense?
This kind of breaks down when there's actually some types used by all the instantiations, as you'd want them to be included by the header. But I think we should be able to diagnose these cases, as usage of this type should be non-dependent (at least in the cases I can think of), so this sounds like a bug/improvement.

A specific example i encountered is clang/Tooling/DiagnosticsYaml.h Which defines template specializations for inputting/outputting yaml io. That file must be included if you ever want to emit diagnostics as YAML, but the typical use case is to just use the operator<< on a yaml stream. This function internally will use those specializations, but as clangd doesn't look into the function calls and expand all those instantiations, they are treated as being unused(There's many bugs already related to this) and as such the header is marked as being unused.

[kadircet]

A specific example i encountered is clang/Tooling/DiagnosticsYaml.h Which defines template specializations for inputting/outputting yaml io. That file must be included if you ever want to emit diagnostics as YAML, but the typical use case is to just use the operator<< on a yaml stream. This function internally will use those specializations, but as clangd doesn't look into the function calls and expand all those instantiations, they are treated as being unused(There's many bugs already related to this) and as such the header is marked as being unused.

i see makes sense. this falls in the "adl through templates" bucket, which is annoying and doesn't seem to be as rare in the wild. so we probably need to figure something out here.
as mentioned above i've hesitations for having this automatically available but it's a state we want to get at eventually. i guess there's not much point in delaying that, so LGTM (after comments already raised are addressed), sorry for the late reply and thanks for the patch!

clang-tools-extra/clangd/IncludeCleaner.cpp

for (const auto *Inc : computeUnusedIncludes(AST)) {
     D.Source = Diag::DiagSource::Clangd;
     D.File = FileName;
     D.Severity = DiagnosticsEngine::Warning;
     D.Tags.push_back(Unnecessary);
     D.Range = getDiagnosticRange(Code, Inc->HashOffset);
     // FIXME(kirillbobyrev): Removing inclusion might break the code if the
     // used headers are only reachable transitively through this one. Suggest
     // including them directly instead.
     // FIXME(kirillbobyrev): Add fix suggestion for adding IWYU pragmas
     // (keep/export) remove the warning once we support IWYU pragmas.
     D.Fixes.emplace_back();
     D.Fixes.back().Message = "remove #include directive";
     D.Fixes.back().Edits.emplace_back();
     D.Fixes.back().Edits.back().range.start.line = Inc->HashLine;
     D.Fixes.back().Edits.back().range.end.line = Inc->HashLine + 1;
+    // Add a fix to append IWYU pragma: keep
+    D.Fixes.emplace_back();
+    D.Fixes.back().Message = "add pragma keep directive";
+    D.Fixes.back().Edits.emplace_back();
+    D.Fixes.back().Edits.back().newText = " // IWYU pragma: keep";
+    D.Fixes.back().Edits.back().range.start =
+        D.Fixes.back().Edits.back().range.end = D.Range.end;
     D.InsideMainFile = true;
     Result.push_back(std::move(D));
   }
   return Result;
 }
 
 } // namespace clangd
 } // namespace clang

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

EXPECT_THAT(*TU.build().getDiagnostics(),
                   AllOf(Diag(Test.range("unused"), "unused variable 'x'"),
                         withTag(DiagnosticTag::Unnecessary)),
                   AllOf(Diag(Test.range("deprecated"), "'bar' is deprecated"),
                         withTag(DiagnosticTag::Deprecated))));
 }
 
 TEST(DiagnosticsTest, IncludeCleaner) {
   Annotations Test(R"cpp(
-$fix[[  $diag[[#include "unused.h"]]
+$fix[[  $diag[[#include "unused.h"]]$insert[[]]
 ]]
   #include "used.h"
 
   #include "ignore.h"
 
   #include <system_header.h>
 
   void foo() {
]]
   Cfg.Diagnostics.UnusedIncludes = Config::UnusedIncludesPolicy::Strict;
   // Set filtering.
   Cfg.Diagnostics.Includes.IgnoreHeader.emplace_back(
       [](llvm::StringRef Header) { return Header.endswith("ignore.h"); });
   WithContextValue WithCfg(Config::Key, std::move(Cfg));
   auto AST = TU.build();
   EXPECT_THAT(
       *AST.getDiagnostics(),
-      UnorderedElementsAre(AllOf(
-          Diag(Test.range("diag"),
+      UnorderedElementsAre(
+          AllOf(Diag(Test.range("diag"),
                      "included header unused.h is not used directly"),
                 withTag(DiagnosticTag::Unnecessary), diagSource(Diag::Clangd),
-          withFix(Fix(Test.range("fix"), "", "remove #include directive")))));
+                withFix(Fix(Test.range("fix"), "", "remove #include directive"),
+                        Fix(Test.range("insert"), " // IWYU pragma: keep",
+                            "add pragma keep directive")))));
   auto &Diag = AST.getDiagnostics()->front();
   EXPECT_EQ(getDiagnosticDocURI(Diag.Source, Diag.ID, Diag.Name),
             std::string("https://clangd.llvm.org/guides/include-cleaner"));
   Cfg.Diagnostics.SuppressAll = true;
   WithContextValue SuppressAllWithCfg(Config::Key, std::move(Cfg));
   EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
   Cfg.Diagnostics.SuppressAll = false;
   Cfg.Diagnostics.Suppress = {"unused-includes"};