[clangd] Add support for missing includes analysis.

[VitaNuo]

clang-tools-extra/clangd/IncludeCleaner.cpp

751

Ok should be done now.

766

Thanks. This might very well be the case, but this comment also seems to suggest some premature optimization (in a way). It totally makes sense to re-use what's re-usable, but this sort of refactoring really only makes sense once we have a clear use case (and get there :)

clang-tools-extra/clangd/IncludeCleaner.h

40

Ok, agreed. Storing symbols now. Having only unused include analysis on is a convincing use case.

[VitaNuo]

Upload once again.

[VitaNuo]

Merge upstream changes.

[kadircet]

thanks! looks amazing, we're missing a little bit of test coverage though

clang-tools-extra/clangd/IncludeCleaner.cpp

284

s/HeaderSpelling/HeaderPath

286

s/Path/NormalizedPath

418

what about just resolvedPath, if you'd rather keep the verb, i think get makes more sense than find. we're not really searching anything.

422

nit: you can directly return SymProvider.physical()->tryGetRealPathName(); (same for other 2 cases) and have an llvm_unreachable("Unknown symbol kind"); after the switch statement.

425

in this and the next case we need to trim <>"

434

same as above, either just symbolName or get

438

again you can just return here and below

438

getName is a StringRef, and unfortunately there are some platforms (like darwin) that don't support implicit conversion from stringrefs to std::string. so can you call .str() explicitly in the end?

760

i think for now this should be

if (Cfg.Diagnostics.MissingIncludes == Config::IncludesPolicy::Strict ||
  Cfg.Diagnostics.UnusedIncludes == Config::IncludesPolicy::Experiment) {

otherwise we'll run both legacy and new analysis for UnusedIncludes == Strict

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

450

this is pointing at the declaration inside b.h not to the reference inside the main file. are you sure this test passes?

470

can you also add a reference (and declaration) for std::vector, and have an IWYU private pragma in one of the headers to test code paths that spell verbatim and standard headers? also having some diagnostic suppressed via IgnoreHeaders is important to check

482

can you make one of these names qualified? e.g. namespace ns { struct Bar { void f(); }; }

[VitaNuo]

Improve test coverage.

[VitaNuo]

Thank you for all the thoughtful comments!

clang-tools-extra/clangd/IncludeCleaner.cpp

418

Ok, let's call it get. I do prefer verbs for methods, that's correct.

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

450

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.

470

Thank you for the great tips on improving test coverage!

In fact, I had to also introduce support for private pragmas, as they were not taken care of. Hopefully, the solution will make sense to you.

[kadircet]

thanks, looks great!

clang-tools-extra/clangd/IncludeCleaner.cpp

453

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. 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.

731

you can directly use !Pragmas->isPrivate(Inc->Resolved) here, instead of getpublic

731

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?

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

445

nit: braces

450

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

458

i think the example for std::vector is solid, and IWYU pragma private needs a little adjustment.

471

we should include private.h through some indirection (not public.h) to check IWYU pragma private spellings are respected.

477

name this range as bar instead of d?

481

could you add a comment here saying this shouldn't be diagnosed?

[VitaNuo]

Address review comments.

[VitaNuo]

Thanks for the comments!

clang-tools-extra/clangd/IncludeCleaner.cpp

731

Ok makes sense. No, I guess I was just confused, because I understood that you wanted a test that includes "private.h" with a diagnostic generated saying that "public.h" should be included instead, so I assumed that was expected behaviour. But that's not what you meant, so I misunderstood.

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

450

Didn't know there was a difference between uninitialized and false..

Thanks for the idea with ASSERT_TRUE(Decl). Please check out the new version.

[kadircet]

clang-tools-extra/clangd/IncludeCleaner.cpp

456

creating a copy of LLVM style unnecessarily all the time is not really great, can you move this into the failure case instead?

also you can drop the clang:: here and elsewhere, as this code is already part of clang:: namespace.

457

as mentioned above we also need to make sure we're passing the relevant VFS instance inside the source manager, rather than using the real file system (as some clients rely on the VFS).

458

s/MainFile->getName()/AST.tuPath()/

to be consistent with other places.

460

can you also elog this error? as it should be rare and when this goes wrong, having this mentioned in the logs are really useful for debugging (since the failure is actually outside of clangd, it usually means a malformed config file somewhere)

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

428

nit: instead of using a point, can you use a range here instead (i.e. [[b]])? afterwards you can have a FileRange pointing at both offsets, rather than relying on the length of the identifier.

448

rest of the code here doesn't really belong to the for loop, can you take them out?

[VitaNuo]

Address review comments.

[VitaNuo]

Thanks for the comments.

[kadircet]

thanks for bearing with me, let's ship it!

clang-tools-extra/clangd/IncludeCleaner.cpp

456

nit: this could be shorter with

auto FileStyle = format::getStyle(..);
if (!FileStyle) {
  elog("...");
  FileStyle = format::getLLVMStyle();
}
tooling::HeaderIncludes HeaderIncludes(AST.tuPath(), Code, FileStyle->IncludeStyle);

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

450

nit:

size_t Start = llvm::cantFail(positionToOffset(MainFile.code(), Range.start));
size_t End = llvm::cantFail(positionToOffset(MainFile.code(), Range.end));

no need for EXPECT_FALSE(..takeError())s as llvm::cantFail will fail (no pun intended :P), static_casts are also redundant

460

it'd be better to ASSERT_TRUE(BDecl); right after the for loop, as rest of the code will crash (and even trigger undefined behavior because we're dereferencing nullptr in failure case).

difference between ASSERT_X and EXPECT_X macros are, the former will stop execution of the particular test (hence we'll never trigger a nullptr deref with ASSERT_TRUE), whereas the latter just prints the failure, but doesn't abort the execution of test (hence helps print multiple failures at once, when they're non fatal).

[VitaNuo]

Address review comments.

[VitaNuo]

Rebase to main.

[PiotrZSL]

This change broke regression, none-one read: "This revision was landed with ongoing or failed builds."
./ClangdTests.exe/IncludeCleaner/GenerateMissingHeaderDiags tests fails on windows

[thakis]

This breaks tests on windows: http://45.33.8.238/win/75486/step_9.txt

Please take a look and revert for now if it takes a while to fix.

[thakis]

Oh, that was reported a while ago already. Reverted in 2eb5ac99a76dbbf8ac68c538211fabeaa5ac0bfd for now.

[hokein]

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

464

Looks like this filter doesn't work on windows (the / vs \ path separator might be the root cause here), I think a fix can be

• change the check to return Header.endsWith("buzz.h")
• or return Header == testPath("buzz.h", llvm::sys::path::Style::posix).

[PiotrZSL]

@VitaNuo Why did you recommit this again without any fix, breaking regression again.

[VitaNuo]

Try to fix windows build.

[VitaNuo]

Try another approach.

[VitaNuo]

Fix formatting.