Page MenuHomePhabricator

[clangd] Add new IncludeType to IncludeHeaderWithReferences

Authored by dgoldman on Jun 23 2022, 12:32 PM.



The IncludeType contains both Include (the current behavior) and Import,
which we can use in the future to provide #import suggestions for
Objective-C files/symbols.

Diff Detail

Event Timeline

dgoldman created this revision.Jun 23 2022, 12:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2022, 12:32 PM
dgoldman requested review of this revision.Jun 23 2022, 12:32 PM
dgoldman updated this revision to Diff 470868.Oct 26 2022, 10:55 AM

Change IncludeTypes to be a bitset

kadircet added inline comments.Oct 27 2022, 6:36 AM

you can use LLVM_MARK_AS_BITMASK_ENUM and relevant magic to implement bitwise operators on the type (see llvm-project/llvm/include/llvm/ADT/BitmaskEnum.h)


i think we already rely on unsigned being 32 bits here. instead of introducing a new byte, can we make References 30 bits long and use 2 bits for the include type? also change the type from unsigned to uint32_t.


the naming here feels a little confusing, can we change the enum name to be IncludeDirective and field name to SupportedDirectives


no need for the comment here.


can we also have an example for both (and none)

dgoldman updated this revision to Diff 473347.Nov 4 2022, 3:01 PM
dgoldman marked an inline comment as done.

Fixes for review

  • Swap over IncludeType to IncludeDirective and update the protos accordingly
dgoldman marked 4 inline comments as done.Nov 4 2022, 3:04 PM

Also LMK if you want more in this change (such as a flag to control it, just not sure where it should live + what it should be called).


Done, although for Serialization I left it as a 32 bit and then 8 bit include directives, LMK if I should swap it over to serialize as a 32 bit single value... Guess I would need to use a union or manually bit manipulate it to store + load it?

1689–1690 ↗(On Diff #473347)

Not sure the best way to fix this, should we also scan the -include files for #imports?

dgoldman updated this revision to Diff 474571.Nov 10 2022, 10:26 AM
dgoldman marked an inline comment as done.

Fix serialization and isSelfContainedHeader

  • Keep serialization as a single var uint32
  • Keep old imported logic in isSelfContainedHeader in addition to a new check for import lines only for main files
dgoldman updated this revision to Diff 475180.Nov 14 2022, 9:39 AM

Run clang format

kadircet added inline comments.Nov 17 2022, 7:38 AM
1241 ↗(On Diff #475180)

can you put a comment here saying any header that contains #imports are supposed to be #import'd so no need to check for anything but the main-file.

1249 ↗(On Diff #475180)

nit: perform .take_front directly here.


nit: SI.SupportedDirectives |= OI.SupportedDirectives;


nit: I'd still keep the enum name singular


can we rather modify the previous function to look like:

IncludeDirective shouldCollectIncludePath(index::SymbolKind Kind) {
  using SK = index::SymbolKind;
  switch (Kind) {
  case SK::Macro:
  case SK::Enum:
  case SK::Struct:
  case SK::Class:
  case SK::Union:
  case SK::TypeAlias:
  case SK::Using:
  case SK::Function:
  case SK::Variable:
  case SK::EnumConstant:
  case SK::Concept:
    return Include | Import;
  case SK::Protocol:
    return Import;
    return Invalid;

can we do this in addDeclaration instead? we already have nameLocation and FileID there, but that's also where we have the decl itself, which might be needed in future if we need more detailed checks.


can we instead do

auto It = map.find(key);
if (It == map.end()) {
  It = map.insert(key, calculateValue()).first;
bool ContainsImports = It->second;

can we perform ContainsImports calculations/caching here instead? that way we can trim a bunch of redundant searches.


i guess IncludeTypes pieces are leftover?


i think this case should still be empty, otherwise we're actually regressing the behaviour by inserting includes for symbols that we previously wouldn't insert.

dgoldman updated this revision to Diff 476220.Nov 17 2022, 1:32 PM
dgoldman marked 10 inline comments as done.

Fixes for review

kadircet added inline comments.Nov 30 2022, 8:13 AM

i don't think this is the right layer to perform such a filtering, we should instead be returning everything here, both the header and the directive to use.

later on inside CodeComplete.cpp, there's headerToInsertIfAllowed for now we can drop headers to be #import'd there, with a fixme saying propagation into other layers.
similarly IncludeFixer should drop these inside fixesForSymbols for now.


nit: while here combine with the condition above


nit: feel free to rewrite as const auto &[SID, FID] : IncludeFiles, as you seem to be referring to Entry.second a bunch.



if ((CollectDirectives & Symbol::Import) != 0) {
     auto [It, Inserted] = FileToContainsImportsOrObjC.try_emplace(Entry.second);
       It->second = FilesWithObjCConstructs.contains(Entry.second) || fileContainsImports(Entry.second,ASTCtx->getSourceManager());
     if(It->second) Directives |= Symbol::Import;

i'd first do this check, as it doesn't require parsing file contents


let's drop the Opts.CollectIncludePath check. it doesn't really align with the model of "we're just marking files that contain objc constructs"

dgoldman updated this revision to Diff 479024.Nov 30 2022, 11:02 AM
dgoldman marked 6 inline comments as done.

Fixes for review

Herald added a project: Restricted Project. · View Herald TranscriptNov 30 2022, 2:00 PM

it feels like rebasing went wrong. changes from 2 unrelated patches seem to be part of this one now. can you make sure this patch only contains the delta for symbolcollector/clangd pieces?


let's rename this to SymbolInclude as it looks too similar to HeaderFile right now.

also adding a comment like: A header and directives as stored in a Symbol.


let's mention that this is either a URI or verbatim include in the comments


can we add a comment like Only allow #import for symbols from objc-like files.


SM.getFileEntryForID(SM.getMainFileID()) == FE

dgoldman updated this revision to Diff 480465.Dec 6 2022, 6:30 AM
dgoldman marked 4 inline comments as done.

Fixes + hopefully proper diffbase

kadircet accepted this revision.Dec 6 2022, 6:47 AM

thanks, lgtm!




i think another important positive test case is, having it after some non PP code blocks (e.g. put it after main)

This revision is now accepted and ready to land.Dec 6 2022, 6:47 AM
dgoldman updated this revision to Diff 480513.Dec 6 2022, 9:02 AM
dgoldman marked 2 inline comments as done.

Update test

This revision was landed with ongoing or failed builds.Dec 6 2022, 10:48 AM
This revision was automatically updated to reflect the committed changes.