[PATCH] D140875: [clangd] prototype: Implement unused include warnings with include-cleaner library.

Thu Jan 19 00:48:30 PST 2023

kadircet added inline comments.

================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:479
+    }
+    // FIXME: !!this is a hacky way to collect macro references.
+    std::vector<include_cleaner::SymbolReference> Macros;
----------------
hokein wrote:
> kadircet wrote:
> > this might behave slightly different than what we have in `RecordedPP`, and rest of the applications of include-cleaner will be using that. can we expose the pieces in RecordedPP that collects MacroRefs as a separate handler and attach that collector (combined with the skipped range collection inside `CollectMainFileMacros` and also still converting to `MainFileMacros` in the end (as we can't store sourcelocations/identifierinfos from preamble)?
> > 
> > afterwards we can use the names stored in there to get back to `IdentifierInfo`s and Ranges stored to get back to `SourceLocation`s. WDYT?
> Yeah, this is a better solution, but I'm not sure whether we should do this before the release cut, it has a side effect of changing the find-macro-reference behavior in clangd. It requires some design/implement work.
> 
> I agree that the current solution is hacky, and eventually will be replaced, but it follows the existing `findReferencedMacros`, so it is not that bad. I tend to land this version before the release cut. What do you think?
> Yeah, this is a better solution, but I'm not sure whether we should do this before the release cut, it has a side effect of changing the find-macro-reference behavior in clangd.

OK, i think you're right about possibly changing semantic highlighting&refs for clangd. Let's push this as a follow-up after the branch cut then.

> It requires some design/implement work.

No action needed here just wanted to layout some ideas;
We just need to change the implementation details of CollectMainFileMacros to wrap the RecordedPP and add skipped ranges support to it. We might need to do a little plumbing to convert between types (as RecordedPP stores SourceLocations/MacroInfos that can't be used across preamble and AST) but that should be trivial to marshal (we can do the conversion inside EndOfMainFile). I haven't fully tried this though, so if you tried and faced certain incompatibilities PLMK.

> I agree that the current solution is hacky, and eventually will be replaced, but it follows the existing findReferencedMacros, so it is not that bad. I tend to land this version before the release cut. What do you think?

I am worried that landing this version and letting people use it for 6 months might result in us getting bug reports that we can't address until we converge (or even worse get bug reports due to behavior change after we converge). But changes here are somewhat more justified as we put this as an experimental feature, compared to regressions in existing clangd behavior as you pointed out.

================
Comment at: clang-tools-extra/clangd/IncludeCleaner.h:100
 std::vector<const Inclusion *> computeUnusedIncludes(ParsedAST &AST);
+std::vector<const Inclusion *> computeUnusedIncludesNew(ParsedAST &AST);

----------------
s/computeUnusedIncludesNew/computeUnusedIncludesExperimental/

can you also add a comment saying that this uses include-cleaner library to perform usage analysis?

================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:609
+      Config::UnusedIncludesPolicy::Experiment)
+    Pragmas.record(*Clang);
   // Copy over the macros in the preamble region of the main file, and combine
----------------
hokein wrote:
> kadircet wrote:
> > why do we need to collect pragmas in main file? i think we already have necessary information available via `IncludeStructure` (it stores keeps and exports, and we don't care about anything else in the main file AFAICT). so let's just use the PragmaIncludes we're getting from the Preamble directly? without even making a copy and returning a reference from the `Preamble` instead in `ParsedAST::getPragmaIncludes`
> > i think we already have necessary information available via IncludeStructure (it stores keeps and exports, and we don't care about anything else in the main file AFAICT)
> 
> The IncludeStructure doesn't have a full support for IWYU export pragma, it only tracks the headers that have the export pragma.
> 
> My understand of the end goal is to use the `PragmaInclude` to handle every IWYU-related things, and we can remove all these IWYU bits in the `IncludeStructure`,  clangd IWYU pragma handling [code](https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/Headers.cpp#L127-L166).
> The IncludeStructure doesn't have a full support for IWYU export pragma, it only tracks the headers that have the export pragma.

And I think that's all we need for IWYU pragmas inside the main file (as main file is a leaf and exporting headers from it doesn't change anything apart from making sure we keep them around)

> My understand of the end goal is to use the PragmaInclude to handle every IWYU-related things, and we can remove all these IWYU bits in the IncludeStructure, clangd IWYU pragma handling code.

Yes, I agree with some version of that, but until then this is introducing some extra changes (+ copying around more information in every AST build). So can we leave this piece out of the initial patch and just use the PragmaInclude we have from Preamble without copying it around?

================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:345
   auto Unused = computeUnusedIncludes(AST);
   EXPECT_THAT(Unused, ElementsAre(Pointee(writtenInclusion("<queue>"))));
+  EXPECT_THAT(computeUnusedIncludesNew(AST),
----------------
can you inline the call to `computeUnusedIncludes` into the EXPECT statement here?

================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:382
     UnusedIncludes.push_back(Include->Written);
   EXPECT_THAT(UnusedIncludes,
               UnorderedElementsAre("\"unused.h\"", "\"dir/unused.h\""));
----------------
can you also update the test here to have same structure as below?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140875/new/

https://reviews.llvm.org/D140875