diff --git a/clang-tools-extra/clangd/InlayHints.cpp b/clang-tools-extra/clangd/InlayHints.cpp --- a/clang-tools-extra/clangd/InlayHints.cpp +++ b/clang-tools-extra/clangd/InlayHints.cpp @@ -8,15 +8,176 @@ #include "InlayHints.h" #include "HeuristicResolver.h" #include "ParsedAST.h" -#include "support/Logger.h" +#include "SourceCode.h" #include "clang/AST/DeclarationName.h" #include "clang/AST/ExprCXX.h" #include "clang/AST/RecursiveASTVisitor.h" #include "clang/Basic/SourceManager.h" -#include "llvm/Support/raw_ostream.h" +#include "llvm/ADT/ScopeExit.h" namespace clang { namespace clangd { +namespace { + +// Helper class to iterate over the designator names of an aggregate type. +// +// For an array type, yields [0], [1], [2]... +// For aggregate classes, yields null for each base, then .field1, .field2, ... +class AggregateDesignatorNames { +public: + AggregateDesignatorNames(QualType T) { + if (!T.isNull()) { + T = T.getCanonicalType(); + if (T->isArrayType()) { + IsArray = true; + Valid = true; + return; + } + if (const RecordDecl *RD = T->getAsRecordDecl()) { + Valid = true; + FieldsI = RD->field_begin(); + FieldsE = RD->field_end(); + if (const auto *CRD = llvm::dyn_cast(RD)) { + BasesI = CRD->bases_begin(); + BasesE = CRD->bases_end(); + Valid = CRD->isAggregate(); + } + OneField = Valid && BasesI == BasesE && FieldsI != FieldsE && + std::next(FieldsI) == FieldsE; + } + } + } + // Returns false if the type was not an aggregate. + operator bool() { return Valid; } + // Advance to the next element in the aggregate. + void next() { + if (IsArray) + ++Index; + else if (BasesI != BasesE) + ++BasesI; + else if (FieldsI != FieldsE) + ++FieldsI; + } + // Print the designator to Out. + // Returns false if we could not produce a designator for this element. + bool append(std::string &Out, bool ForSubobject) { + if (IsArray) { + Out.push_back('['); + Out.append(std::to_string(Index)); + Out.push_back(']'); + return true; + } + if (BasesI != BasesE) + return false; // Bases can't be designated. Should we make one up? + if (FieldsI != FieldsE) { + llvm::StringRef FieldName; + if (const IdentifierInfo *II = FieldsI->getIdentifier()) + FieldName = II->getName(); + + // For certain objects, their subobjects may be named directly. + if (ForSubobject && + (FieldsI->isAnonymousStructOrUnion() || + // std::array x = {1,2,3}. Designators not strictly valid! + (OneField && isReservedName(FieldName)))) + return true; + + if (!FieldName.empty() && !isReservedName(FieldName)) { + Out.push_back('.'); + Out.append(FieldName.begin(), FieldName.end()); + return true; + } + return false; + } + return false; + } + +private: + bool Valid = false; + bool IsArray = false; + bool OneField = false; // e.g. std::array { T __elements[N]; } + unsigned Index = 0; + CXXRecordDecl::base_class_const_iterator BasesI; + CXXRecordDecl::base_class_const_iterator BasesE; + RecordDecl::field_iterator FieldsI; + RecordDecl::field_iterator FieldsE; +}; + +// Collect designator labels describing the elements of an init list. +// +// This function contributes the designators of some (sub)object, which is +// represented by the semantic InitListExpr Sem. +// This includes any nested subobjects, but *only* if they are part of the same +// original syntactic init list (due to brace elision). +// In other words, it may descend into subobjects but not written init-lists. +// +// For example: struct Outer { Inner a,b; }; struct Inner { int x, y; } +// Outer o{{1, 2}, 3}; +// This function will be called with Sem = { {1, 2}, {3, ImplicitValue} } +// It should generate designators '.a:' and '.b.x:'. +// '.a:' is produced directly without recursing into the written sublist. +// Recursion with Prefix='.b' and Sem = {3, ImplicitValue} produces '.b.x:'. +void collectDesignators(const InitListExpr *Sem, + llvm::DenseMap &Out, + const llvm::DenseSet &NestedBraces, + std::string &Prefix) { + if (!Sem || Sem->isTransparent()) + return; + assert(Sem->isSemanticForm()); + + // The elements of the semantic form all correspond to direct subobjects of + // the type Type. `Fields` iterates over these subobject names. + AggregateDesignatorNames Fields(Sem->getType()); + if (!Fields) + return; + for (const Expr *Init : Sem->inits()) { + auto Next = llvm::make_scope_exit([&, Size(Prefix.size())] { + Fields.next(); // Always advance to the next subobject name. + Prefix.resize(Size); // Erase any designator we appended. + }); + if (llvm::isa(Init)) + continue; // a "hole" for a subobject that was not explicitly initialized + + const auto *BraceElidedSubobject = llvm::dyn_cast(Init); + if (BraceElidedSubobject && + NestedBraces.contains(BraceElidedSubobject->getLBraceLoc())) + BraceElidedSubobject = nullptr; // there were braces! + + if (!Fields.append(Prefix, BraceElidedSubobject != nullptr)) + continue; // no designator available for this subobject + if (BraceElidedSubobject) { + // If the braces were elided, this aggregate subobject is initialized + // inline in the same syntactic list. + // Descend into the semantic list describing the subobject. + collectDesignators(BraceElidedSubobject, Out, NestedBraces, Prefix); + continue; + } + Prefix.push_back('='); + Out.try_emplace(Init->getBeginLoc(), Prefix); + } +} + +// Get designators describing the elements of a (syntactic) init list. +// This does not produce designators for any explicitly-written nested lists. +llvm::DenseMap +getDesignators(const InitListExpr *Syn) { + assert(Syn->isSyntacticForm()); + + // gatherDesignators needs to know which InitListExprs in the semantic tree + // were actually written, but InitListExpr::isExplicit() lies. + // Instead, record where braces of sub-init-lists occur in the syntactic form. + llvm::DenseSet NestedBraces; + for (const Expr *Init : Syn->inits()) + if (auto *Nested = llvm::dyn_cast(Init)) + NestedBraces.insert(Nested->getLBraceLoc()); + + // Traverse the semantic form to find the designators. + // We use their SourceLocation to correlate with the syntactic form later. + llvm::DenseMap Designators; + std::string Scratch; + collectDesignators(Syn->isSemanticForm() ? Syn : Syn->getSemanticForm(), + Designators, NestedBraces, Scratch); + return Designators; +} class InlayHintVisitor : public RecursiveASTVisitor { public: @@ -119,6 +280,22 @@ return true; } + bool VisitInitListExpr(InitListExpr *Syn) { + if (Syn->isIdiomaticZeroInitializer(AST.getLangOpts())) + return true; + llvm::DenseMap Designators = + getDesignators(Syn); + for (const Expr *Init : Syn->inits()) { + if (llvm::isa(Init)) + continue; + auto It = Designators.find(Init->getBeginLoc()); + if (It != Designators.end() && + !isPrecededByParamNameComment(Init, It->second)) + addDesignatorHint(Init->getSourceRange(), It->second); + } + return true; + } + // FIXME: Handle RecoveryExpr to try to hint some invalid calls. private: @@ -220,12 +397,16 @@ // Check for comment ending. if (!SourcePrefix.consume_back("*/")) return false; - // Allow whitespace and "=" at end of comment. - SourcePrefix = SourcePrefix.rtrim().rtrim('=').rtrim(); + // Ignore some punctuation and whitespace around comment. + // In particular this allows designators to match nicely. + llvm::StringLiteral IgnoreChars = " =."; + SourcePrefix = SourcePrefix.rtrim(IgnoreChars); + ParamName = ParamName.trim(IgnoreChars); // Other than that, the comment must contain exactly ParamName. if (!SourcePrefix.consume_back(ParamName)) return false; - return SourcePrefix.rtrim().endswith("/*"); + SourcePrefix = SourcePrefix.rtrim(IgnoreChars); + return SourcePrefix.endswith("/*"); } // If "E" spells a single unqualified identifier, return that name. @@ -344,6 +525,10 @@ addInlayHint(R, InlayHintKind::TypeHint, std::string(Prefix) + TypeName); } + void addDesignatorHint(SourceRange R, llvm::StringRef Text) { + addInlayHint(R, InlayHintKind::DesignatorHint, Text); + } + std::vector &Results; ASTContext &AST; FileID MainFileID; @@ -361,6 +546,8 @@ static const size_t TypeNameLimit = 32; }; +} // namespace + std::vector inlayHints(ParsedAST &AST) { std::vector Results; InlayHintVisitor Visitor(Results, AST); diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h --- a/clang-tools-extra/clangd/Protocol.h +++ b/clang-tools-extra/clangd/Protocol.h @@ -1533,6 +1533,12 @@ /// which shows the deduced type of the variable. TypeHint, + /// A hint before an element of an aggregate braced initializer list, + /// indicating what it is initializing. + /// Pair{^1, ^2}; + /// Uses designator syntax, e.g. `.first:`. + DesignatorHint, + /// Other ideas for hints that are not currently implemented: /// /// * Chaining hints, showing the types of intermediate expressions diff --git a/clang-tools-extra/clangd/Protocol.cpp b/clang-tools-extra/clangd/Protocol.cpp --- a/clang-tools-extra/clangd/Protocol.cpp +++ b/clang-tools-extra/clangd/Protocol.cpp @@ -1326,6 +1326,8 @@ return "parameter"; case InlayHintKind::TypeHint: return "type"; + case InlayHintKind::DesignatorHint: + return "designator"; } llvm_unreachable("Unknown clang.clangd.InlayHintKind"); } diff --git a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp --- a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp +++ b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp @@ -11,18 +11,21 @@ #include "TestTU.h" #include "TestWorkspace.h" #include "XRefs.h" +#include "llvm/Support/ScopedPrinter.h" #include "gmock/gmock.h" #include "gtest/gtest.h" namespace clang { namespace clangd { -std::ostream &operator<<(std::ostream &Stream, const InlayHint &Hint) { - return Stream << Hint.label; +llvm::raw_ostream &operator<<(llvm::raw_ostream &Stream, + const InlayHint &Hint) { + return Stream << Hint.label << "@" << Hint.range; } namespace { +using ::testing::ElementsAre; using ::testing::UnorderedElementsAre; std::vector hintsOfKind(ParsedAST &AST, InlayHintKind Kind) { @@ -38,15 +41,24 @@ std::string Label; std::string RangeName; - friend std::ostream &operator<<(std::ostream &Stream, - const ExpectedHint &Hint) { - return Stream << Hint.RangeName << ": " << Hint.Label; + friend llvm::raw_ostream &operator<<(llvm::raw_ostream &Stream, + const ExpectedHint &Hint) { + return Stream << Hint.Label << "@$" << Hint.RangeName; } }; -MATCHER_P2(HintMatcher, Expected, Code, "") { - return arg.label == Expected.Label && - arg.range == Code.range(Expected.RangeName); +MATCHER_P2(HintMatcher, Expected, Code, llvm::to_string(Expected)) { + if (arg.label != Expected.Label) { + *result_listener << "label is " << arg.label; + return false; + } + if (arg.range != Code.range(Expected.RangeName)) { + *result_listener << "range is " << arg.label << " but $" + << Expected.RangeName << " is " + << llvm::to_string(Code.range(Expected.RangeName)); + return false; + } + return true; } template @@ -58,7 +70,7 @@ auto AST = TU.build(); EXPECT_THAT(hintsOfKind(AST, Kind), - UnorderedElementsAre(HintMatcher(Expected, Source)...)); + ElementsAre(HintMatcher(Expected, Source)...)); } template @@ -73,6 +85,12 @@ assertHints(InlayHintKind::TypeHint, AnnotatedSource, Expected...); } +template +void assertDesignatorHints(llvm::StringRef AnnotatedSource, + ExpectedHints... Expected) { + assertHints(InlayHintKind::DesignatorHint, AnnotatedSource, Expected...); +} + TEST(ParameterHints, Smoke) { assertParameterHints(R"cpp( void foo(int param); @@ -631,6 +649,71 @@ ExpectedHint{": int", "var"}); } +TEST(DesignatorHints, Basic) { + assertDesignatorHints(R"cpp( + struct S { int x, y, z; }; + S s {$x[[1]], $y[[2+2]]}; + + int x[] = {$0[[0]], $1[[1]]}; + )cpp", + ExpectedHint{".x=", "x"}, ExpectedHint{".y=", "y"}, + ExpectedHint{"[0]=", "0"}, ExpectedHint{"[1]=", "1"}); +} + +TEST(DesignatorHints, Nested) { + assertDesignatorHints(R"cpp( + struct Inner { int x, y; }; + struct Outer { Inner a, b; }; + Outer o{ $a[[{ $x[[1]], $y[[2]] }]], $bx[[3]] }; + )cpp", + ExpectedHint{".a=", "a"}, ExpectedHint{".x=", "x"}, + ExpectedHint{".y=", "y"}, ExpectedHint{".b.x=", "bx"}); +} + +TEST(DesignatorHints, AnonymousRecord) { + assertDesignatorHints(R"cpp( + struct S { + union { + struct { + struct { + int y; + }; + } x; + }; + }; + S s{$xy[[42]]}; + )cpp", + ExpectedHint{".x.y=", "xy"}); +} + +TEST(DesignatorHints, Suppression) { + assertDesignatorHints(R"cpp( + struct Point { int a, b, c, d, e, f, g, h; }; + Point p{/*a=*/1, .c=2, /* .d = */3, $e[[4]]}; + )cpp", + ExpectedHint{".e=", "e"}); +} + +TEST(DesignatorHints, StdArray) { + // Designators for std::array should be [0] rather than .__elements[0]. + // While technically correct, the designator is useless and horrible to read. + assertDesignatorHints(R"cpp( + template struct Array { T __elements[N]; }; + Array x = {$0[[0]], $1[[1]]}; + )cpp", + ExpectedHint{"[0]=", "0"}, ExpectedHint{"[1]=", "1"}); +} + +TEST(DesignatorHints, OnlyAggregateInit) { + assertDesignatorHints(R"cpp( + struct Copyable { int x; } c; + Copyable d{c}; + + struct Constructible { Constructible(int x); }; + Constructible x{42}; + )cpp" /*no hints expected*/); +} + // FIXME: Low-hanging fruit where we could omit a type hint: // - auto x = TypeName(...); // - auto x = (TypeName) (...);