This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Support iwyu pragma: no_include
Needs ReviewPublic

Authored by zyounan on Feb 4 2023, 4:23 AM.

Details

Summary

This patch is one of the steps trying to optimize experience
of header insertion in code completion.
IWYU supports no_include pragma that helps user to get rid
of specified headers in main file.
The patch consists of two parts: Code completion part that
prevent auto insertion if the header to be inserted is under
the guard of no_include pragma. The other part is for include
cleaner, which emits diagnostic message pointing out that user
is including no_include headers.

A possible workaround for clangd/clangd#1481.

Diff Detail

Event Timeline

zyounan created this revision.Feb 4 2023, 4:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2023, 4:23 AM
zyounan updated this revision to Diff 494815.Feb 4 2023, 4:27 AM

Remove extra headers

zyounan updated this revision to Diff 494816.Feb 4 2023, 4:54 AM

Update test

zyounan published this revision for review.Feb 4 2023, 5:29 AM
zyounan added reviewers: hokein, sammccall, kadircet, nridge.
zyounan updated this revision to Diff 494911.Feb 5 2023, 7:25 AM

Update tests && Use Preprocessor.LookupFile

zyounan updated this revision to Diff 494921.Feb 5 2023, 8:16 AM

Fix test on windows

thanks a lot for the patch!

we're migrating IWYU related functionality in clangd to make use of include-cleaner library nowadays. so can you actually move the IWYU no_include pragma handling logic into https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/include-cleaner/lib/Record.cpp?
This won't have affect on clangd regarding the particular problem you're facing immediately, but we're going to merge the logic inside include-cleaner and clangd soon and such discrepancies should disappear then.

in the meanwhile regarding the particular problem you're facing for experimental symbols. sorry for shutting down D142836, but that actually made us re-evaluate the approach of hand-curated lists and we've decided to introduce such mappings. so if you can revamp D142836 and put TsStdSymbolMap.inc in https://github.com/llvm/llvm-project/tree/main/clang/lib/Tooling/Inclusions/Stdlib/StdTsSymbolMap.inc we can land it and make sure it work as intended inside clangd, even before we merge the IWYU pragma handling logic. Happy to do that myself if you don't feel like doing it, sorry for creating some confusion here :(

thanks a lot for the patch!

we're migrating IWYU related functionality in clangd to make use of include-cleaner library nowadays. so can you actually move the IWYU no_include pragma handling logic into https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/include-cleaner/lib/Record.cpp?
This won't have affect on clangd regarding the particular problem you're facing immediately, but we're going to merge the logic inside include-cleaner and clangd soon and such discrepancies should disappear then.

in the meanwhile regarding the particular problem you're facing for experimental symbols. sorry for shutting down D142836, but that actually made us re-evaluate the approach of hand-curated lists and we've decided to introduce such mappings. so if you can revamp D142836 and put TsStdSymbolMap.inc in https://github.com/llvm/llvm-project/tree/main/clang/lib/Tooling/Inclusions/Stdlib/StdTsSymbolMap.inc we can land it and make sure it work as intended inside clangd, even before we merge the IWYU pragma handling logic. Happy to do that myself if you don't feel like doing it, sorry for creating some confusion here :(

Although I'm convinced that they're not stable enough to migrate into Stdlib directory, I'm still glad to hear that we're going to support TS symbols.
And certainly, I'd love to revamp D142836, while I believe that this IWYU no-include is a more general approach than that ad-hoc way.

zyounan updated this revision to Diff 499417.EditedFeb 22 2023, 1:54 AM

Sorry for the late update :)
Move parsing logic to Record.cpp now.

Thanks for the patch, and sorry for the long delay.

You probably need to do a large rebase when updating this patch (since the include-cleaner clangd code has been changed a lot)

This is a big patch, touching several pieces (I'd suggest split this patch, and narrow its scope.):

  1. support no_include pragma in include-cleaner library (I made some comments regarding the implementation)
  2. usage of no_include pragma:
    • 2.1 include-cleaner diagnostics in clangd -- for missing-includes, we should not insert a spelled include when it is in the NoIncludeSet;
    • 2.2 global code completion respects no_include pragma

It makes sense to support 1 and 2.1. And 2.2 is nice to have -- global code completion was built long time ago, and we haven't heard of any user complains, I'm less certain, it is probably fine if it is trivial to implement.

clang-tools-extra/clangd/IncludeCleaner.cpp
579

I'm not sure it is worth to make a dedicated function for this marginal pragma, I think we can treat it as an unused-include case, and we can move it to issueUnusedIncludesDiagnostics.

clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
44 ↗(On Diff #499417)

I think we can do it simpler. The Resolved field is no used anywhere, and is expensive to get.

The critical information to store is the header spelled in the no_include pragma, so having a StringSet<> NoIncludes field in PragmaIncludes should be sufficient.

165 ↗(On Diff #499417)

This is not a specific issue for no_include pragma, it is a known limitation -- ideally we should make PragmaIncludes collect all IWYU pragmas outside of the preamble region, tracking in https://github.com/clangd/clangd/issues/1571.
I think we should merge this logic to the PragmaIncludes structure.