-
Notifications
You must be signed in to change notification settings - Fork 15.9k
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 34 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,211 @@ | |
| #include "llvm/ADT/StringRef.h" | ||
| #include <limits> | ||
| #include <optional> | ||
| #include <tuple> | ||
|
|
||
| #define DEBUG_TYPE "FindSymbols" | ||
|
|
||
| namespace clang { | ||
| namespace clangd { | ||
|
|
||
| namespace { | ||
|
|
||
| // "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) { | ||
| 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 toSymbolTagBitmask(const SymbolTag ST) { | ||
| return (1 << static_cast<unsigned>(ST)); | ||
| } | ||
|
|
||
| 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 +440,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,18 @@ getWorkspaceSymbols(llvm::StringRef Query, int Limit, | |
| /// same order that they appear. | ||
| llvm::Expected<std::vector<DocumentSymbol>> getDocumentSymbols(ParsedAST &AST); | ||
|
|
||
| /// Converts a single SymbolTag to a bitmask. | ||
| SymbolTags toSymbolTagBitmask(SymbolTag ST); | ||
|
|
||
| /// Computes symbol tags for a given NamedDecl. | ||
| SymbolTags computeSymbolTags(const NamedDecl &ND); | ||
|
|
||
| /// Returns the symbol tags for the given declaration. | ||
| /// This is a wrapper around computeSymbolTags() which unpacks | ||
| /// the tags into a vector. | ||
| /// \p ND The declaration to get tags for. | ||
| std::vector<SymbolTag> getSymbolTags(const NamedDecl &ND); | ||
|
|
||
| } // namespace clangd | ||
| } // namespace clang | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -281,7 +281,7 @@ struct TextDocumentEdit { | |
| /// The text document to change. | ||
| VersionedTextDocumentIdentifier textDocument; | ||
|
|
||
| /// The edits to be applied. | ||
| /// The edits to be applied. | ||
| /// FIXME: support the AnnotatedTextEdit variant. | ||
| std::vector<TextEdit> edits; | ||
| }; | ||
|
|
@@ -560,7 +560,7 @@ struct ClientCapabilities { | |
|
|
||
| /// The client supports versioned document changes for WorkspaceEdit. | ||
| bool DocumentChanges = false; | ||
|
|
||
| /// The client supports change annotations on text edits, | ||
| bool ChangeAnnotation = false; | ||
|
|
||
|
|
@@ -1027,12 +1027,12 @@ struct WorkspaceEdit { | |
| /// Versioned document edits. | ||
| /// | ||
| /// If a client neither supports `documentChanges` nor | ||
| /// `workspace.workspaceEdit.resourceOperations` then only plain `TextEdit`s | ||
| /// using the `changes` property are supported. | ||
| /// `workspace.workspaceEdit.resourceOperations` then only plain `TextEdit`s | ||
| /// using the `changes` property are supported. | ||
| std::optional<std::vector<TextDocumentEdit>> documentChanges; | ||
|
|
||
| /// A map of change annotations that can be referenced in | ||
| /// AnnotatedTextEdit. | ||
| /// AnnotatedTextEdit. | ||
| std::map<std::string, ChangeAnnotation> changeAnnotations; | ||
| }; | ||
| bool fromJSON(const llvm::json::Value &, WorkspaceEdit &, llvm::json::Path); | ||
|
|
@@ -1104,6 +1104,35 @@ struct CodeAction { | |
| }; | ||
| llvm::json::Value toJSON(const CodeAction &); | ||
|
|
||
| /// Symbol tags are extra annotations that can be attached to a symbol. | ||
| /// \see https://github.com/microsoft/language-server-protocol/pull/2003 | ||
| enum class SymbolTag { | ||
|
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. If I'm understanding this correctly, the list and order here is coming from microsoft/language-server-protocol#2003. Can we link to that PR from a comment here? That makes it clear that we're implementing something that's not in the spec yet, and serves as a reminder for us to update the implementation if the spec ends up being different from the current list in the PR.
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
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 not seeing the requested comment.
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 |
||
| Deprecated = 1, | ||
| Private = 2, | ||
| Package = 3, | ||
| Protected = 4, | ||
| Public = 5, | ||
| Internal = 6, | ||
| File = 7, | ||
| Static = 8, | ||
| Abstract = 9, | ||
| Final = 10, | ||
| Sealed = 11, | ||
| Transient = 12, | ||
| Volatile = 13, | ||
| Synchronized = 14, | ||
| Virtual = 15, | ||
| Nullable = 16, | ||
| NonNull = 17, | ||
| Declaration = 18, | ||
| Definition = 19, | ||
| ReadOnly = 20, | ||
|
|
||
| // Update as needed | ||
| FirstTag = Deprecated, | ||
| LastTag = ReadOnly | ||
| }; | ||
| llvm::json::Value toJSON(SymbolTag); | ||
| /// Represents programming constructs like variables, classes, interfaces etc. | ||
| /// that appear in a document. Document symbols can be hierarchical and they | ||
| /// have two ranges: one that encloses its definition and one that points to its | ||
|
|
@@ -1121,6 +1150,9 @@ struct DocumentSymbol { | |
| /// Indicates if this symbol is deprecated. | ||
| bool deprecated = false; | ||
|
|
||
| /// The tags for this symbol. | ||
| std::vector<SymbolTag> tags; | ||
|
|
||
| /// The range enclosing this symbol not including leading/trailing whitespace | ||
| /// but everything else like comments. This information is typically used to | ||
| /// determine if the clients cursor is inside the symbol to reveal in the | ||
|
|
@@ -1146,6 +1178,9 @@ struct SymbolInformation { | |
| /// The kind of this symbol. | ||
| SymbolKind kind; | ||
|
|
||
| /// Tags for this symbol, e.g public, private, static, const etc. | ||
| std::vector<SymbolTag> tags; | ||
|
|
||
| /// The location of this symbol. | ||
| Location location; | ||
|
|
||
|
|
@@ -1288,13 +1323,13 @@ enum class InsertTextFormat { | |
| /// Additional details for a completion item label. | ||
| struct CompletionItemLabelDetails { | ||
| /// An optional string which is rendered less prominently directly after label | ||
| /// without any spacing. Should be used for function signatures or type | ||
| /// without any spacing. Should be used for function signatures or type | ||
| /// annotations. | ||
| std::string detail; | ||
|
|
||
| /// An optional string which is rendered less prominently after | ||
| /// CompletionItemLabelDetails.detail. Should be used for fully qualified | ||
| /// names or file path. | ||
| /// CompletionItemLabelDetails.detail. Should be used for fully qualified | ||
| /// names or file path. | ||
| std::string description; | ||
| }; | ||
| llvm::json::Value toJSON(const CompletionItemLabelDetails &); | ||
|
|
@@ -1572,9 +1607,6 @@ struct ResolveTypeHierarchyItemParams { | |
| bool fromJSON(const llvm::json::Value &, ResolveTypeHierarchyItemParams &, | ||
| llvm::json::Path); | ||
|
|
||
| enum class SymbolTag { Deprecated = 1 }; | ||
| llvm::json::Value toJSON(SymbolTag); | ||
|
|
||
| /// The parameter of a `textDocument/prepareCallHierarchy` request. | ||
| struct CallHierarchyPrepareParams : public TextDocumentPositionParams {}; | ||
|
|
||
|
|
||
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.
Please put the helpers (i.e. anything not exposed in the header) in the anonymous namespace.
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.