[clangd] Simplify semantic highlighting visitor

[ilya-biryukov]

Summary

• Functions to compute highlighting kinds for things are separated from the ones that add highlighting tokens. This keeps each of them more focused on what they're doing: getting locations and figuring out the kind of the entity, correspondingly.

• Less special cases in visitor for various nodes.

This change is an NFC.

[hokein]

Thanks!

Unfortunately, the patch is a bit large, containing refactoring changes and functionality changes, is it possible to split it into (two) smaller patches?

[ilya-biryukov]

• Turn into NFC, do not highlight lambdas differently

[ilya-biryukov]

In D67341#1663104, @hokein wrote:

Unfortunately, the patch is a bit large, containing refactoring changes and functionality changes, is it possible to split it into (two) smaller patches?

Done. There should be no functional changes now.

[hokein]

thanks, looks good, just a few nits.

clang-tools-extra/clangd/SemanticHighlighting.cpp

34 ↗

(On Diff #219363)

nit: I think the check is redundant, getAsIdentifierInfo() will return nullptr if !Name.isIdentifier().

37 ↗

(On Diff #219363)

nit: !II->getName().empty()

169 ↗

(On Diff #219363)

how about moving out this method (and kindForDecl) of the class, they seem to not depend on any fields of the class?

212 ↗

(On Diff #219363)

nit: auto

260 ↗

(On Diff #219363)

maybe

if (auto K = kindForDecl(D))
   addToken(Loc, *K);

[ilya-biryukov]

• Address comments

clang-tools-extra/clangd/SemanticHighlighting.cpp

169 ↗

(On Diff #219363)

Makes sense. Done.

[ilya-biryukov]

@hokein, do you want to do another round or is this good to go?

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

 //===--- SemanticHighlighting.cpp - ------------------------- ---*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "SemanticHighlighting.h"
 #include "Logger.h"
 #include "ParsedAST.h"
 #include "Protocol.h"
 #include "SourceCode.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclarationName.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/Type.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/SourceLocation.h"
 #include <algorithm>
 
 namespace clang {
 namespace clangd {
 namespace {
 
+/// Some names are not written in the source code and cannot be highlighted,
+/// e.g. anonymous classes. This function detects those cases.
+bool canHighlightName(DeclarationName Name) {
+  if (Name.getNameKind() == DeclarationName::CXXConstructorName ||
+      Name.getNameKind() == DeclarationName::CXXUsingDirective)
+    return true;
+  auto *II = Name.getAsIdentifierInfo();
+  return II && !II->getName().empty();
+}
+
+llvm::Optional<HighlightingKind> kindForType(const Type *TP);
+llvm::Optional<HighlightingKind> kindForDecl(const NamedDecl *D) {
+  if (auto *TD = dyn_cast<TypedefNameDecl>(D)) {
+    // We try to highlight typedefs as their underlying type.
+    if (auto K = kindForType(TD->getUnderlyingType().getTypePtrOrNull()))
+      return K;
+    // And fallback to a generic kind if this fails.
+    return HighlightingKind::Typedef;
+  }
+  // We highlight class decls, constructor decls and destructor decls as
+  // `Class` type. The destructor decls are handled in `VisitTypeLoc` (we
+  // will visit a TypeLoc where the underlying Type is a CXXRecordDecl).
+  if (auto *RD = llvm::dyn_cast<RecordDecl>(D)) {
+    // We don't want to highlight lambdas like classes.
+    if (RD->isLambda())
+      return llvm::None;
+    return HighlightingKind::Class;
+  }
+  if (isa<ClassTemplateDecl>(D) || isa<RecordDecl>(D) ||
+      isa<CXXConstructorDecl>(D))
+    return HighlightingKind::Class;
+  if (auto *MD = dyn_cast<CXXMethodDecl>(D))
+    return MD->isStatic() ? HighlightingKind::StaticMethod
+                          : HighlightingKind::Method;
+  if (isa<FieldDecl>(D))
+    return HighlightingKind::Field;
+  if (isa<EnumDecl>(D))
+    return HighlightingKind::Enum;
+  if (isa<EnumConstantDecl>(D))
+    return HighlightingKind::EnumConstant;
+  if (isa<ParmVarDecl>(D))
+    return HighlightingKind::Parameter;
+  if (auto *VD = dyn_cast<VarDecl>(D))
+    return VD->isStaticDataMember()
+               ? HighlightingKind::StaticField
+               : VD->isLocalVarDecl() ? HighlightingKind::LocalVariable
+                                      : HighlightingKind::Variable;
+  if (isa<BindingDecl>(D))
+    return HighlightingKind::Variable;
+  if (isa<FunctionDecl>(D))
+    return HighlightingKind::Function;
+  if (isa<NamespaceDecl>(D) || isa<NamespaceAliasDecl>(D) ||
+      isa<UsingDirectiveDecl>(D))
+    return HighlightingKind::Namespace;
+  if (isa<TemplateTemplateParmDecl>(D) || isa<TemplateTypeParmDecl>(D) ||
+      isa<NonTypeTemplateParmDecl>(D))
+    return HighlightingKind::TemplateParameter;
+  return llvm::None;
+}
+llvm::Optional<HighlightingKind> kindForType(const Type *TP) {
+  if (!TP)
+    return llvm::None;
+  if (TP->isBuiltinType()) // Builtins are special, they do not have decls.
+    return HighlightingKind::Primitive;
+  if (auto *TD = dyn_cast<TemplateTypeParmType>(TP))
+    return kindForDecl(TD->getDecl());
+  if (auto *TD = TP->getAsTagDecl())
+    return kindForDecl(TD);
+  return llvm::None;
+}
+
 // Collects all semantic tokens in an ASTContext.
 class HighlightingTokenCollector
     : public RecursiveASTVisitor<HighlightingTokenCollector> {
   std::vector<HighlightingToken> Tokens;
   ParsedAST &AST;
 
 public:
   HighlightingTokenCollector(ParsedAST &AST) : AST(AST) {}
for (ArrayRef<HighlightingToken> TokRef = Tokens; !TokRef.empty();) {
       // the end of the Tokens).
       TokRef = TokRef.drop_front(Conflicting.size());
     }
     return NonConflicting;
   }
 
   bool VisitNamespaceAliasDecl(NamespaceAliasDecl *NAD) {
     // The target namespace of an alias can not be found in any other way.
-    addToken(NAD->getTargetNameLoc(), HighlightingKind::Namespace);
+    addToken(NAD->getTargetNameLoc(), NAD->getAliasedNamespace());
     return true;
   }
 
   bool VisitMemberExpr(MemberExpr *ME) {
-    const auto *MD = ME->getMemberDecl();
-    if (isa<CXXDestructorDecl>(MD))
-      // When calling the destructor manually like: AAA::~A(); The ~ is a
-      // MemberExpr. Other methods should still be highlighted though.
-      return true;
-    if (isa<CXXConversionDecl>(MD))
-      // The MemberLoc is invalid for C++ conversion operators. We do not
-      // attempt to add tokens with invalid locations.
-      return true;
-    addToken(ME->getMemberLoc(), MD);
+    if (canHighlightName(ME->getMemberNameInfo().getName()))
+      addToken(ME->getMemberLoc(), ME->getMemberDecl());
     return true;
   }
 
   bool VisitNamedDecl(NamedDecl *ND) {
-    // UsingDirectiveDecl's namespaces do not show up anywhere else in the
-    // Visit/Traverse mehods. But they should also be highlighted as a
-    // namespace.
-    if (const auto *UD = dyn_cast<UsingDirectiveDecl>(ND)) {
-      addToken(UD->getIdentLocation(), HighlightingKind::Namespace);
-      return true;
-    }
-
-    // Constructors' TypeLoc has a TypePtr that is a FunctionProtoType. It has
-    // no tag decl and therefore constructors must be gotten as NamedDecls
-    // instead.
-    if (ND->getDeclName().getNameKind() ==
-        DeclarationName::CXXConstructorName) {
-      addToken(ND->getLocation(), ND);
-      return true;
-    }
-
-    if (ND->getDeclName().getNameKind() != DeclarationName::Identifier)
-      return true;
-
+    if (canHighlightName(ND->getDeclName()))
       addToken(ND->getLocation(), ND);
     return true;
   }
 
   bool VisitDeclRefExpr(DeclRefExpr *Ref) {
-    if (Ref->getNameInfo().getName().getNameKind() !=
-        DeclarationName::Identifier)
-      // Only want to highlight identifiers.
-      return true;
-
+    if (canHighlightName(Ref->getNameInfo().getName()))
       addToken(Ref->getLocation(), Ref->getDecl());
     return true;
   }
 
-  bool VisitTypedefNameDecl(TypedefNameDecl *TD) {
-    addTokenForTypedef(TD->getLocation(), TD);
-    return true;
-  }
-
   bool VisitTypedefTypeLoc(TypedefTypeLoc TL) {
-    addTokenForTypedef(TL.getBeginLoc(), TL.getTypedefNameDecl());
+    addToken(TL.getBeginLoc(), TL.getTypedefNameDecl());
     return true;
   }
 
   bool VisitTemplateSpecializationTypeLoc(TemplateSpecializationTypeLoc TL) {
     if (const TemplateDecl *TD =
             TL.getTypePtr()->getTemplateName().getAsTemplateDecl())
       addToken(TL.getBeginLoc(), TD);
     return true;
   }
 
-  bool VisitTypeLoc(TypeLoc &TL) {
-    // This check is for not getting two entries when there are anonymous
-    // structs. It also makes us not highlight certain namespace qualifiers
-    // twice. For elaborated types the actual type is highlighted as an inner
-    // TypeLoc.
-    if (TL.getTypeLocClass() != TypeLoc::TypeLocClass::Elaborated)
-      addType(TL.getBeginLoc(), TL.getTypePtr());
+  bool WalkUpFromTagTypeLoc(TagTypeLoc L) {
+    if (L.isDefinition())
+      return true; // Definition will be highligthed by VisitNamedDecl.
+    return RecursiveASTVisitor::WalkUpFromTagTypeLoc(L);
+  }
+
+  bool WalkUpFromElaboratedTypeLoc(ElaboratedTypeLoc L) {
+    // Avoid highlighting 'struct' or 'enum' keywords.
+    return true;
+  }
+
+  bool VisitTypeLoc(TypeLoc TL) {
+    if (auto K = kindForType(TL.getTypePtr()))
+      addToken(TL.getBeginLoc(), *K);
     return true;
   }
 
   bool TraverseNestedNameSpecifierLoc(NestedNameSpecifierLoc NNSLoc) {
-    if (NestedNameSpecifier *NNS = NNSLoc.getNestedNameSpecifier())
+    if (auto *NNS = NNSLoc.getNestedNameSpecifier()) {
       if (NNS->getKind() == NestedNameSpecifier::Namespace ||
           NNS->getKind() == NestedNameSpecifier::NamespaceAlias)
         addToken(NNSLoc.getLocalBeginLoc(), HighlightingKind::Namespace);
-
+    }
     return RecursiveASTVisitor<
         HighlightingTokenCollector>::TraverseNestedNameSpecifierLoc(NNSLoc);
   }
 
   bool TraverseConstructorInitializer(CXXCtorInitializer *CI) {
     if (const FieldDecl *FD = CI->getMember())
       addToken(CI->getSourceLocation(), FD);
     return RecursiveASTVisitor<
         HighlightingTokenCollector>::TraverseConstructorInitializer(CI);
   }
 
   bool VisitDeclaratorDecl(DeclaratorDecl *D) {
-    if ((!D->getTypeSourceInfo()))
-      return true;
-
-    if (auto *AT = D->getType()->getContainedAutoType()) {
-      const auto Deduced = AT->getDeducedType();
-      if (!Deduced.isNull())
-        addType(D->getTypeSpecStartLoc(), Deduced.getTypePtr());
-    }
-    return true;
-  }
-
-private:
-  void addTokenForTypedef(SourceLocation Loc, const TypedefNameDecl *TD) {
-    auto *TSI = TD->getTypeSourceInfo();
-    if (!TSI)
-      return;
-    // Try to highlight as underlying type.
-    if (addType(Loc, TSI->getType().getTypePtrOrNull()))
-      return;
-    // Fallback to the typedef highlighting kind.
-    addToken(Loc, HighlightingKind::Typedef);
-  }
-
-  bool addType(SourceLocation Loc, const Type *TP) {
-    if (!TP)
-      return false;
-    if (TP->isBuiltinType()) {
-      // Builtins must be special cased as they do not have a TagDecl.
-      addToken(Loc, HighlightingKind::Primitive);
+    // Highlight 'auto' with its underlying type.
+    auto *AT = D->getType()->getContainedAutoType();
+    if (!AT)
       return true;
-    }
-    if (auto *TD = dyn_cast<TemplateTypeParmType>(TP)) {
-      // TemplateTypeParmType also do not have a TagDecl.
-      addToken(Loc, TD->getDecl());
+    auto K = kindForType(AT->getDeducedType().getTypePtrOrNull());
+    if (!K)
       return true;
-    }
-    if (auto *TD = TP->getAsTagDecl()) {
-      addToken(Loc, TD);
-      return true;
-    }
-    return false;
-  }
-
-  void addToken(SourceLocation Loc, const NamedDecl *D) {
-    if (D->getDeclName().isIdentifier() && D->getName().empty())
-      // Don't add symbols that don't have any length.
-      return;
-    // We highlight class decls, constructor decls and destructor decls as
-    // `Class` type. The destructor decls are handled in `VisitTypeLoc` (we will
-    // visit a TypeLoc where the underlying Type is a CXXRecordDecl).
-    if (isa<ClassTemplateDecl>(D)) {
-      addToken(Loc, HighlightingKind::Class);
-      return;
-    }
-    if (isa<RecordDecl>(D)) {
-      addToken(Loc, HighlightingKind::Class);
-      return;
-    }
-    if (isa<CXXConstructorDecl>(D)) {
-      addToken(Loc, HighlightingKind::Class);
-      return;
-    }
-    if (auto *MD = dyn_cast<CXXMethodDecl>(D)) {
-      addToken(Loc, MD->isStatic() ? HighlightingKind::StaticMethod
-                                   : HighlightingKind::Method);
-      return;
-    }
-    if (isa<FieldDecl>(D)) {
-      addToken(Loc, HighlightingKind::Field);
-      return;
-    }
-    if (isa<EnumDecl>(D)) {
-      addToken(Loc, HighlightingKind::Enum);
-      return;
-    }
-    if (isa<EnumConstantDecl>(D)) {
-      addToken(Loc, HighlightingKind::EnumConstant);
-      return;
-    }
-    if (isa<ParmVarDecl>(D)) {
-      addToken(Loc, HighlightingKind::Parameter);
-      return;
-    }
-    if (const VarDecl *VD = dyn_cast<VarDecl>(D)) {
-      addToken(Loc, VD->isStaticDataMember()
-                        ? HighlightingKind::StaticField
-                        : VD->isLocalVarDecl() ? HighlightingKind::LocalVariable
-                                               : HighlightingKind::Variable);
-      return;
-    }
-    if (isa<BindingDecl>(D)) {
-      addToken(Loc, HighlightingKind::Variable);
-      return;
-    }
-    if (isa<FunctionDecl>(D)) {
-      addToken(Loc, HighlightingKind::Function);
-      return;
-    }
-    if (isa<NamespaceDecl>(D)) {
-      addToken(Loc, HighlightingKind::Namespace);
-      return;
-    }
-    if (isa<NamespaceAliasDecl>(D)) {
-      addToken(Loc, HighlightingKind::Namespace);
-      return;
-    }
-    if (isa<TemplateTemplateParmDecl>(D)) {
-      addToken(Loc, HighlightingKind::TemplateParameter);
-      return;
-    }
-    if (isa<TemplateTypeParmDecl>(D)) {
-      addToken(Loc, HighlightingKind::TemplateParameter);
-      return;
-    }
-    if (isa<NonTypeTemplateParmDecl>(D)) {
-      addToken(Loc, HighlightingKind::TemplateParameter);
-      return;
-    }
-  }
+    addToken(D->getTypeSpecStartLoc(), *K);
 
+private:
   void addToken(SourceLocation Loc, HighlightingKind Kind) {
+    if (Loc.isInvalid())
+      return;
     const auto &SM = AST.getSourceManager();
     if (Loc.isMacroID()) {
       // Only intereseted in highlighting arguments in macros (DEF_X(arg)).
       if (!SM.isMacroArgExpansion(Loc))
         return;
       Loc = SM.getSpellingLoc(Loc);
     }
 
     // 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)
+    // topLevelDecls. (example: method declarations being included from
+    // another file for a class from another file).
     // There are also cases with macros where the spelling loc will not be in
     // the main file and the highlighting would be incorrect.
     if (!isInsideMainFile(Loc, SM))
       return;
 
     auto R = getTokenRange(SM, AST.getASTContext().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()});
   }
+
+  void addToken(SourceLocation Loc, const NamedDecl *D) {
+    if (auto K = kindForDecl(D))
+      addToken(Loc, *K);
+  }
 };
 
 // Encode binary data into base64.
 // This was copied from compiler-rt/lib/fuzzer/FuzzerUtil.cpp.
 // FIXME: Factor this out into llvm/Support?
 std::string encodeBase64(const llvm::SmallVectorImpl<char> &Bytes) {
   static const char Table[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
                               "abcdefghijklmnopqrstuvwxyz"