[clangd] Remove highlightings coming from non topLevelDecls from included files.

[jvikstrom]

Summary

It is possible to write include code from other files so that the decls from there do not become topLevelDecls (For example by including methods for a class). These Decls are not filtered by topLevelDecls and therefore SemanticHighlighting must manually check that every SLoc belongs in the main file. Otherwise there can be highlightings appearing in places where they should not.

[ilya-biryukov]

LGTM. Great example for the test case! It will definitely stay valid even if we fix all problems caused by RecursiveASTVisitor!

[hokein]

clang-tools-extra/clangd/SemanticHighlighting.cpp

236 ↗

(On Diff #214608)

nit: the ) is not match.

237 ↗

(On Diff #214608)

nit: clangd has its own isInsideMainFile, please use it.

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

57 ↗

(On Diff #214608)

nit: maybe just std::vector<std::pair</*FileName*/llvm::StringRef, /*FileContent*/llvm::StringRef>>, it is clearer and would save you a long comment above.

clang-tools-extra/trunk/clangd/SemanticHighlighting.cpp

void addToken(SourceLocation Loc, const NamedDecl *D) {
     }
   }
 
   void addToken(SourceLocation Loc, HighlightingKind Kind) {
     if (Loc.isMacroID())
       // FIXME: skip tokens inside macros for now.
       return;
 
+    // Non top level decls that are included from a header are not filtered by
+    // topLevelDecls. (example: method declarations being included from another
+    // file for a class from another file)
+    if (!isInsideMainFile(Loc, SM))
+      return;
+
     auto R = getTokenRange(SM, Ctx.getLangOpts(), Loc);
     if (!R) {
       // R should always have a value, if it doesn't something is very wrong.
       elog("Tried to add semantic token with an invalid range");
       return;
     }
 
     Tokens.push_back({Kind, R.getValue()});

clang-tools-extra/trunk/clangd/unittests/SemanticHighlightingTests.cpp

for (const auto &KindString : KindToString) {
     std::vector<HighlightingToken> Toks = makeHighlightingTokens(
         Test.ranges(KindString.second), KindString.first);
     ExpectedTokens.insert(ExpectedTokens.end(), Toks.begin(), Toks.end());
   }
   llvm::sort(ExpectedTokens);
   return ExpectedTokens;
 }
 
-void checkHighlightings(llvm::StringRef Code) {
+void checkHighlightings(llvm::StringRef Code,
+                        std::vector<std::pair</*FileName*/ llvm::StringRef,
+                                              /*FileContent*/ llvm::StringRef>>
+                            AdditionalFiles = {}) {
   Annotations Test(Code);
-  auto AST = TestTU::withCode(Test.code()).build();
+  auto TU = TestTU::withCode(Test.code());
+  for (auto File : AdditionalFiles)
+    TU.AdditionalFiles.insert({File.first, File.second});
+  auto AST = TU.build();
   std::vector<HighlightingToken> ActualTokens = getSemanticHighlightings(AST);
   EXPECT_THAT(ActualTokens, getExpectedTokens(Test)) << Code;
 }
 
 // Any annotations in OldCode and NewCode are converted into their corresponding
 // HighlightingToken. The tokens are diffed against each other. Any lines where
 // the tokens should diff must be marked with a ^ somewhere on that line in
 // NewCode. If there are diffs that aren't marked with ^ the test fails. The
R"cpp(
       $Enum[[auto]] &$Variable[[AER]] = $Variable[[AE]];
       $Primitive[[auto]] $Variable[[Form]] = 10.2 + 2 * 4;
       $Primitive[[decltype]]($Variable[[Form]]) $Variable[[F]] = 10;
       auto $Variable[[Fun]] = []()->$Primitive[[void]]{};
     )cpp"};
   for (const auto &TestCase : TestCases) {
     checkHighlightings(TestCase);
   }
+
+  checkHighlightings(R"cpp(
+    class $Class[[A]] {
+      #include "imp.h"
+    };
+    #endif
+  )cpp",
+                     {{"imp.h", R"cpp(
+    int someMethod();
+    void otherMethod();
+  )cpp"}});
 }
 
 TEST(SemanticHighlighting, GeneratesHighlightsWhenFileChange) {
   class HighlightingsCounterDiagConsumer : public DiagnosticsConsumer {
   public:
     std::atomic<int> Count = {0};
 
     void onDiagnosticsReady(PathRef, std::vector<Diag>) override {}