[PATCH] D143496: [clangd] Add support for missing includes analysis.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

thanks, looks great!



================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:451
+  tooling::HeaderIncludes HeaderIncludes(AST.tuPath(), Code,
+                                         tooling::IncludeStyle{});
+  const SourceManager &SM = AST.getSourceManager();
----------------
we should respect the style configurations (sorry for missing this in previous iterations).

you can get the relevant style with: `clang::format::getStyle`, which has an [IncludeStyle](https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Format/Format.h#L2188). in case the `getStyle` fails, we should fallback to `clang::format::getLLVMStyle` as we do in other places. you can get at the relevant VFS instance through sourcemanager.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:712
+          for (auto *Inc : ConvertedIncludes.match(H)) {    
+            if (Pragmas == nullptr || Pragmas->getPublic(Inc->Resolved).empty())
+              Satisfied = true;    
----------------
you can directly use `!Pragmas->isPrivate(Inc->Resolved)` here, instead of getpublic


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:712
+          for (auto *Inc : ConvertedIncludes.match(H)) {    
+            if (Pragmas == nullptr || Pragmas->getPublic(Inc->Resolved).empty())
+              Satisfied = true;    
----------------
kadircet wrote:
> you can directly use `!Pragmas->isPrivate(Inc->Resolved)` here, instead of getpublic
this check seems to be new. what's the reason for rejecting private providers? I can see that we might want to be conservative by not inserting private providers, but treating symbols as unsatisfied when a private provider is **already** included doesn't feel right. e.g. the code being analyzed might be allowed to depend on this private header, because it's also part of the library, or it's the public header that's exposing this private header. in such a scenario we shouldn't try to insert the public header again. is there a more concrete issue this code is trying to address?


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:443
+    std::string Name = llvm::dyn_cast<NamedDecl>(D)->getQualifiedNameAsString();
+    if (Name != "b") {
+      continue;
----------------
nit: braces


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:456
+
+// TODO(bakalova) examples with std::vector and IWYU private pragma still needed
+TEST(IncludeCleaner, GenerateMissingHeaderDiags) {
----------------
i think the example for `std::vector` is solid, and `IWYU pragma private` needs a little adjustment.


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:469
+#include "header.h"
+#include "private.h"
+$insert_f[[]]$insert_vector[[]]
----------------
we should include private.h through some indirection (not public.h) to check `IWYU pragma private` spellings are respected.


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:475
+
+      ns::$d[[Bar]] bar;
+      bar.d();
----------------
name this range as `bar` instead of `d`?


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:479
+
+      buzz(); 
+
----------------
could you add a comment here saying this shouldn't be diagnosed?


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:449
+    include_cleaner::Symbol B{*D};
+    syntax::FileRange BRange{SM, B.declaration().getBeginLoc(), 1};
+    include_cleaner::Header Header{*SM.getFileManager().getFile("b.h")};
----------------
VitaNuo wrote:
> kadircet wrote:
> > this is pointing at the declaration inside `b.h` not to the reference inside the main file. are you sure this test passes?
> Yes, all the tests pass. 
> `D` is a `Decl` from the main file, otherwise it wouldn't have passed the safeguard ` if (!SM.isWrittenInMainFile(SM.getExpansionLoc(D->getLocation()))) continue;` above.
this is passing because `bool BDeclFound;` is uninitialized above, if you set it to `bool BDeclFound = false;` you should see the test fail.

there's no declaration for `b` inside the main file, it's declared in `b.h` and **referenced** inside the main file. you still need to search for the decl (without the constraint of being written in main file), use it to build an include_cleaner::Symbol, and use a `clangd::Annotation` range for the range of the reference.

it might be easer to write this as:
```
const NamedDecl* B = nullptr;
for (...) {
 ...
 B = D;
}
ASSERT_TRUE(B);
// build expected diagnostic info based on B and check that it's equal to what we've produced
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143496