-
Notifications
You must be signed in to change notification settings - Fork 16.1k
LSP 3.18 - Symbol Tags #167536
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LSP 3.18 - Symbol Tags #167536
Changes from 30 commits
103b18f
18c71ac
af6491c
baa29b1
64d5a36
49551b5
8a4b314
f29267e
49574d8
a5a8555
979f7e7
b52210f
8131e6f
317eb1d
d899c71
1b39d9c
c0c6506
346fa11
4117ba1
e181082
b7a2003
6f0deff
ea5fd1c
438b02f
53a8c8f
31dc8a5
04144e5
64978c3
b265a9c
bd233a4
2eb7d3b
3333550
49a4a78
1ae72f4
585210a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,13 +23,210 @@ | |
| #include "llvm/ADT/StringRef.h" | ||
| #include <limits> | ||
| #include <optional> | ||
| #include <tuple> | ||
|
|
||
| #define DEBUG_TYPE "FindSymbols" | ||
|
|
||
| namespace clang { | ||
| namespace clangd { | ||
|
|
||
| namespace { | ||
| SymbolTags toSymbolTagBitmask(const SymbolTag ST) { | ||
|
||
| return (1 << static_cast<unsigned>(ST)); | ||
| } | ||
|
|
||
| // "Static" means many things in C++, only some get the "static" modifier. | ||
| // | ||
| // Meanings that do: | ||
| // - Members associated with the class rather than the instance. | ||
| // This is what 'static' most often means across languages. | ||
| // - static local variables | ||
| // These are similarly "detached from their context" by the static keyword. | ||
| // In practice, these are rarely used inside classes, reducing confusion. | ||
| // | ||
| // Meanings that don't: | ||
| // - Namespace-scoped variables, which have static storage class. | ||
| // This is implicit, so the keyword "static" isn't so strongly associated. | ||
| // If we want a modifier for these, "global scope" is probably the concept. | ||
| // - Namespace-scoped variables/functions explicitly marked "static". | ||
| // There the keyword changes *linkage* , which is a totally different concept. | ||
| // If we want to model this, "file scope" would be a nice modifier. | ||
| // | ||
| // This is confusing, and maybe we should use another name, but because "static" | ||
| // is a standard LSP modifier, having one with that name has advantages. | ||
| bool isStatic(const Decl *D) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please put the helpers (i.e. anything not exposed in the header) in the anonymous namespace.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| if (const auto *CMD = llvm::dyn_cast<CXXMethodDecl>(D)) | ||
| return CMD->isStatic(); | ||
| if (const VarDecl *VD = llvm::dyn_cast<VarDecl>(D)) | ||
| return VD->isStaticDataMember() || VD->isStaticLocal(); | ||
| if (const auto *OPD = llvm::dyn_cast<ObjCPropertyDecl>(D)) | ||
| return OPD->isClassProperty(); | ||
| if (const auto *OMD = llvm::dyn_cast<ObjCMethodDecl>(D)) | ||
| return OMD->isClassMethod(); | ||
| if (const auto *FD = llvm::dyn_cast<FunctionDecl>(D)) | ||
| return FD->isStatic(); | ||
| return false; | ||
| } | ||
|
|
||
| // Whether T is const in a loose sense - is a variable with this type readonly? | ||
| bool isConst(QualType T) { | ||
| if (T.isNull()) | ||
| return false; | ||
| T = T.getNonReferenceType(); | ||
| if (T.isConstQualified()) | ||
| return true; | ||
| if (const auto *AT = T->getAsArrayTypeUnsafe()) | ||
| return isConst(AT->getElementType()); | ||
| if (isConst(T->getPointeeType())) | ||
| return true; | ||
| return false; | ||
| } | ||
|
|
||
| // Whether D is const in a loose sense (should it be highlighted as such?) | ||
| // FIXME: This is separate from whether *a particular usage* can mutate D. | ||
| // We may want V in V.size() to be readonly even if V is mutable. | ||
| bool isConst(const Decl *D) { | ||
| if (llvm::isa<EnumConstantDecl>(D) || llvm::isa<NonTypeTemplateParmDecl>(D)) | ||
| return true; | ||
| if (llvm::isa<FieldDecl>(D) || llvm::isa<VarDecl>(D) || | ||
| llvm::isa<MSPropertyDecl>(D) || llvm::isa<BindingDecl>(D)) { | ||
| if (isConst(llvm::cast<ValueDecl>(D)->getType())) | ||
| return true; | ||
| } | ||
| if (const auto *OCPD = llvm::dyn_cast<ObjCPropertyDecl>(D)) { | ||
| if (OCPD->isReadOnly()) | ||
| return true; | ||
| } | ||
| if (const auto *MPD = llvm::dyn_cast<MSPropertyDecl>(D)) { | ||
| if (!MPD->hasSetter()) | ||
| return true; | ||
| } | ||
| if (const auto *CMD = llvm::dyn_cast<CXXMethodDecl>(D)) { | ||
| if (CMD->isConst()) | ||
| return true; | ||
| } | ||
| if (const auto *FD = llvm::dyn_cast<FunctionDecl>(D)) | ||
| return isConst(FD->getReturnType()); | ||
| return false; | ||
| } | ||
|
|
||
| // Indicates whether declaration D is abstract in cases where D is a struct or a | ||
| // class. | ||
| bool isAbstract(const Decl *D) { | ||
| if (const auto *CMD = llvm::dyn_cast<CXXMethodDecl>(D)) | ||
| return CMD->isPureVirtual(); | ||
| if (const auto *CRD = llvm::dyn_cast<CXXRecordDecl>(D)) | ||
| return CRD->hasDefinition() && CRD->isAbstract(); | ||
| return false; | ||
| } | ||
|
|
||
| // Indicates whether declaration D is virtual in cases where D is a method. | ||
| bool isVirtual(const Decl *D) { | ||
| if (const auto *CMD = llvm::dyn_cast<CXXMethodDecl>(D)) | ||
| return CMD->isVirtual(); | ||
| return false; | ||
| } | ||
|
|
||
| // Indicates whether declaration D is final in cases where D is a struct, class | ||
| // or method. | ||
| bool isFinal(const Decl *D) { | ||
| if (const auto *CRD = dyn_cast<CXXMethodDecl>(D)) | ||
| return CRD->hasAttr<FinalAttr>(); | ||
|
|
||
| if (const auto *CRD = dyn_cast<CXXRecordDecl>(D)) | ||
| return CRD->hasAttr<FinalAttr>(); | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| // Indicates whether declaration D is a unique definition (as opposed to a | ||
| // declaration). | ||
| bool isUniqueDefinition(const NamedDecl *Decl) { | ||
| if (auto *Func = dyn_cast<FunctionDecl>(Decl)) | ||
| return Func->isThisDeclarationADefinition(); | ||
| if (auto *Klass = dyn_cast<CXXRecordDecl>(Decl)) | ||
| return Klass->isThisDeclarationADefinition(); | ||
| if (auto *Iface = dyn_cast<ObjCInterfaceDecl>(Decl)) | ||
| return Iface->isThisDeclarationADefinition(); | ||
| if (auto *Proto = dyn_cast<ObjCProtocolDecl>(Decl)) | ||
| return Proto->isThisDeclarationADefinition(); | ||
| if (auto *Var = dyn_cast<VarDecl>(Decl)) | ||
| return Var->isThisDeclarationADefinition(); | ||
| return isa<TemplateTypeParmDecl>(Decl) || | ||
| isa<NonTypeTemplateParmDecl>(Decl) || | ||
| isa<TemplateTemplateParmDecl>(Decl) || isa<ObjCCategoryDecl>(Decl) || | ||
| isa<ObjCImplDecl>(Decl); | ||
| } | ||
| } // namespace | ||
|
|
||
| SymbolTags computeSymbolTags(const NamedDecl &ND) { | ||
| SymbolTags Result = 0; | ||
| const auto IsDef = isUniqueDefinition(&ND); | ||
|
|
||
| if (ND.isDeprecated()) | ||
| Result |= toSymbolTagBitmask(SymbolTag::Deprecated); | ||
|
|
||
| if (isConst(&ND)) | ||
| Result |= toSymbolTagBitmask(SymbolTag::ReadOnly); | ||
|
|
||
| if (isStatic(&ND)) | ||
| Result |= toSymbolTagBitmask(SymbolTag::Static); | ||
|
|
||
| if (isVirtual(&ND)) | ||
| Result |= toSymbolTagBitmask(SymbolTag::Virtual); | ||
|
|
||
| if (isAbstract(&ND)) | ||
| Result |= toSymbolTagBitmask(SymbolTag::Abstract); | ||
|
|
||
| if (isFinal(&ND)) | ||
| Result |= toSymbolTagBitmask(SymbolTag::Final); | ||
|
|
||
| if (not isa<UnresolvedUsingValueDecl>(ND)) { | ||
| // Do not treat an UnresolvedUsingValueDecl as a declaration. | ||
| // It's more common to think of it as a reference to the | ||
| // underlying declaration. | ||
| Result |= toSymbolTagBitmask(SymbolTag::Declaration); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a bit unclear on the intended semantics of the Is the intention that every document symbol is a The wording in the PR -- specifically "Render a symbol as definition (in contrast to declaration)" -- makes me think perhaps (I have some ideas about cleaning up the API to avoid the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In if (R.IsDecl) {
// Do not treat an UnresolvedUsingValueDecl as a declaration.
// It's more common to think of it as a reference to the
// underlying declaration.
if (!isa<UnresolvedUsingValueDecl>(Decl))
Tok.addModifier(HighlightingModifier::Declaration);
if (isUniqueDefinition(Decl))
Tok.addModifier(HighlightingModifier::Definition);
}now the if-statement is located in computeSymbolTags and the lines ...
const auto SymbolTags = computeSymbolTags(*Decl, R.IsDecl);
...
if (SymbolTags & toSymbolTagBitmask(SymbolTag::Declaration))
Tok.addModifier(HighlightingModifier::Declaration);
if (SymbolTags & toSymbolTagBitmask(SymbolTag::Definition))
Tok.addModifier(HighlightingModifier::Definition);
...replacing the code block, mentioned above.
When we decide to either ... or, some tests will break.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm assuming you're referring to semantic highlighting tests. I'm talking here about the expected behaviour of Say you're a client like Eclipse, and you send a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are no such cases. The code block below arises from the assumption that a definition is always also a declaration. A definition cannot exist without a declaration. I now think that my previous assumption is incorrect, especially in this context.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ok. (This makes me question a bit why the So, in terms of implementation strategy, I would suggest the following:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
|
|
||
| if (IsDef) | ||
| Result |= toSymbolTagBitmask(SymbolTag::Definition); | ||
| } | ||
|
|
||
| switch (ND.getAccess()) { | ||
| case AS_public: | ||
| Result |= toSymbolTagBitmask(SymbolTag::Public); | ||
| break; | ||
| case AS_protected: | ||
| Result |= toSymbolTagBitmask(SymbolTag::Protected); | ||
| break; | ||
| case AS_private: | ||
| Result |= toSymbolTagBitmask(SymbolTag::Private); | ||
| break; | ||
| default: | ||
| break; | ||
| } | ||
|
|
||
| return Result; | ||
| } | ||
|
|
||
| std::vector<SymbolTag> getSymbolTags(const NamedDecl &ND) { | ||
| const auto symbolTags = computeSymbolTags(ND); | ||
| std::vector<SymbolTag> Tags; | ||
|
|
||
| if (symbolTags == 0) | ||
| return Tags; | ||
|
|
||
| // Iterate through SymbolTag enum values and collect any that are present in | ||
| // the bitmask. SymbolTag values are in the numeric range | ||
| // [FirstTag .. LastTag]. | ||
| constexpr unsigned MinTag = static_cast<unsigned>(SymbolTag::FirstTag); | ||
| constexpr unsigned MaxTag = static_cast<unsigned>(SymbolTag::LastTag); | ||
| for (unsigned I = MinTag; I <= MaxTag; ++I) { | ||
| auto ST = static_cast<SymbolTag>(I); | ||
| if (symbolTags & toSymbolTagBitmask(ST)) | ||
| Tags.push_back(ST); | ||
| } | ||
| return Tags; | ||
| } | ||
|
|
||
| namespace { | ||
| using ScoredSymbolInfo = std::pair<float, SymbolInformation>; | ||
| struct ScoredSymbolGreater { | ||
|
|
@@ -242,6 +439,7 @@ std::optional<DocumentSymbol> declToSym(ASTContext &Ctx, const NamedDecl &ND) { | |
| SI.range = Range{sourceLocToPosition(SM, SymbolRange->getBegin()), | ||
| sourceLocToPosition(SM, SymbolRange->getEnd())}; | ||
| SI.detail = getSymbolDetail(Ctx, ND); | ||
| SI.tags = getSymbolTags(ND); | ||
|
|
||
| SourceLocation NameLoc = ND.getLocation(); | ||
| SourceLocation FallbackNameLoc; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,13 +14,23 @@ | |
|
|
||
| #include "Protocol.h" | ||
| #include "index/Symbol.h" | ||
| #include "clang/AST/Decl.h" | ||
| #include "llvm/ADT/StringRef.h" | ||
|
|
||
| namespace clang { | ||
| namespace clangd { | ||
| class ParsedAST; | ||
| class SymbolIndex; | ||
|
|
||
| /// A bitmask type representing symbol tags supported by LSP. | ||
| /// \see | ||
| /// https://microsoft.github.io/language-server-protocol/specifications/specification-current/#symbolTag | ||
| using SymbolTags = uint32_t; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| /// Ensure we have enough bits to represent all SymbolTag values. | ||
| static_assert(static_cast<unsigned>(SymbolTag::LastTag) <= 32, | ||
| "Too many SymbolTags to fit in uint32_t. Change to uint64_t if " | ||
| "we ever have more than 32 tags."); | ||
|
|
||
| /// Helper function for deriving an LSP Location from an index SymbolLocation. | ||
| llvm::Expected<Location> indexToLSPLocation(const SymbolLocation &Loc, | ||
| llvm::StringRef TUPath); | ||
|
|
@@ -47,6 +57,13 @@ getWorkspaceSymbols(llvm::StringRef Query, int Limit, | |
| /// same order that they appear. | ||
| llvm::Expected<std::vector<DocumentSymbol>> getDocumentSymbols(ParsedAST &AST); | ||
|
|
||
| /// Returns the symbol tags for the given declaration. | ||
|
||
| /// \p ND The declaration to get tags for. | ||
| std::vector<SymbolTag> getSymbolTags(const NamedDecl &ND); | ||
|
|
||
| /// Computes symbol tags for a given NamedDecl. | ||
| SymbolTags computeSymbolTags(const NamedDecl &ND); | ||
|
|
||
| } // namespace clangd | ||
| } // namespace clang | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: This include is not necessary any more.
I would suggest removing the changes to
AST.cppandcall-hierarchy.testfrom this diff altogether.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.