This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add new IncludeType to IncludeHeaderWithReferences
ClosedPublic

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

Details

Summary

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
clang-tools-extra/clangd/index/Symbol.h
96

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)

119–126

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.

122

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

clang-tools-extra/clangd/index/remote/Index.proto
110

no need for the comment here.

clang-tools-extra/clangd/unittests/SerializationTests.cpp
59

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

clang-tools-extra/clangd/index/Symbol.h
122

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?

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
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
clang-tools-extra/clangd/SourceCode.cpp
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.

clang-tools-extra/clangd/index/Merge.cpp
251

nit: SI.SupportedDirectives |= OI.SupportedDirectives;

clang-tools-extra/clangd/index/Symbol.h
90

nit: I'd still keep the enum name singular

clang-tools-extra/clangd/index/SymbolCollector.cpp
106

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;
  default:
    return Invalid;
  }
}
812

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.

860

can we instead do

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

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

clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp
431–436

i guess IncludeTypes pieces are leftover?

clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
2736

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
clang-tools-extra/clangd/Headers.cpp
225

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.

clang-tools-extra/clangd/index/SymbolCollector.cpp
809–810

nit: while here combine with the condition above

843–844

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

871

nit:

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

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

964

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?

clang-tools-extra/clangd/Headers.h
48

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.

50

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

clang-tools-extra/clangd/index/SymbolCollector.cpp
870

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

clang/lib/Tooling/Inclusions/HeaderAnalysis.cpp
76

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!

clang/unittests/Tooling/HeaderAnalysisTest.cpp
69

indentation

69

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.