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 @@ -14,6 +14,7 @@ #include "clang/AST/DeclarationName.h" #include "clang/AST/ExprCXX.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/Basic/Builtins.h" #include "clang/Basic/SourceManager.h" #include "llvm/ADT/ScopeExit.h" @@ -185,6 +186,168 @@ return Designators; } +bool isExpandedParameter(const ParmVarDecl *Param) { + auto PlainType = Param->getType().getNonReferenceType(); + if (const auto *SubstType = dyn_cast(PlainType)) { + return SubstType->getReplacedParameter()->isParameterPack(); + } + return false; +} + +class ForwardingParameterVisitor + : public RecursiveASTVisitor { +public: + void + resolveForwardedParameters(const FunctionDecl *Callee, + llvm::SmallVector &Params) { + if (!TraversedFunctions.contains(Callee)) { + UnresolvedParams.clear(); + size_t ParamIdx = 0; + for (const auto *Param : Params) { + // If the parameter is part of an expanded pack and not yet resolved + if (/*isExpandedParameter(Param) && */ + ForwardedParams.find(Param) == ForwardedParams.end()) { + UnresolvedParams.insert(Param); + } + handleParmVarDeclName(Callee, ParamIdx); + ParamIdx++; + } + // Recursively resolve forwarded parameters + if (!UnresolvedParams.empty()) { + TraversalQueue.push_back(const_cast(Callee)); + while (!TraversalQueue.empty()) { + auto *CurrentFunction = TraversalQueue.front(); + TraversedFunctions.insert(CurrentFunction); + TraverseFunctionDecl(CurrentFunction); + TraversalQueue.pop_front(); + } + } + } + // Replace parameters by their forwarded counterpart + for (auto &Param : Params) { + Param = getForwardedParameter(Param); + Param = getNamedParameter(Param); + } + } + + bool VisitCXXConstructExpr(CXXConstructExpr *E) { + // if there are no va_args, we can forward parameters + if (E->getConstructor()->getNumParams() == E->getNumArgs()) { + handleCallOrConstructExpr(E->getConstructor(), E->arguments()); + } + return false; + } + + bool VisitCallExpr(CallExpr *D) { + if (auto *Callee = dyn_cast(D->getCalleeDecl())) { + // if there are no va_args, we can forward parameters + if (Callee->getNumParams() == D->getNumArgs()) { + handleCallOrConstructExpr(Callee, D->arguments()); + } + } + return false; + } + +private: + void handleParmVarDeclName(const FunctionDecl *Callee, size_t I) { + const auto *Param = Callee->getParamDecl(I); + // If the declaration doesn't provide a name + if (!Param->getDeclName().getAsIdentifierInfo()) { + // Try the definition + if (const auto *Def = Callee->getDefinition()) { + const auto *DefParam = Def->getParamDecl(I); + if (DefParam != Param && + DefParam->getDeclName().getAsIdentifierInfo()) { + // Store the mapping declaration parameter -> definition parameter + ParamDeclToDef.insert({Param, DefParam}); + } + } + } + } + + void handleCallOrConstructExpr(FunctionDecl *Callee, + CallExpr::arg_range Args) { + size_t ArgIdx = 0; + bool Recurse = false; + for (const auto *Arg : Args) { + if (const auto *ArgRef = unpackArgument(Arg)) { + if (const auto *ArgSource = dyn_cast(ArgRef->getDecl())) { + const auto It = UnresolvedParams.find(ArgSource); + if (It == UnresolvedParams.end()) { + continue; + } + const auto *Param = Callee->getParamDecl(ArgIdx); + // If this parameter is part of an expanded pack, we need to recurse + if (isExpandedParameter(Param)) { + UnresolvedParams.insert(Param); + Recurse = true; + } else { + // If it is a concrete parameter, we need to check whether it + // provides a name, and if not check its definition + handleParmVarDeclName(Callee, ArgIdx); + } + // This parameter has now been resolved + UnresolvedParams.erase(It); + ForwardedParams.insert({ArgSource, Param}); + } + } + ArgIdx++; + } + if (Recurse && !TraversedFunctions.contains(Callee)) { + TraversalQueue.push_back(Callee); + } + } + + const Expr *unpackImplicitCast(const Expr *E) { + if (const auto *Cast = dyn_cast(E)) { + return Cast->getSubExpr(); + } + return E; + } + + const Expr *unpackForward(const Expr *E) { + if (const auto *Call = dyn_cast(E)) { + const auto Callee = Call->getBuiltinCallee(); + if (Callee == Builtin::BIforward) { + return Call->getArg(0); + } + } + return E; + } + + const DeclRefExpr *unpackArgument(const Expr *E) { + E = unpackImplicitCast(E); + E = unpackForward(E); + return dyn_cast(E); + } + + const ParmVarDecl *getForwardedParameter(const ParmVarDecl *Param) { + auto It = ForwardedParams.find(Param); + if (It == ForwardedParams.end()) { + return Param; + } + // path compression + It->second = getForwardedParameter(It->second); + return It->second; + } + + const ParmVarDecl *getNamedParameter(const ParmVarDecl *Param) { + auto It = ParamDeclToDef.find(Param); + if (It == ParamDeclToDef.end()) { + return Param; + } + return It->second; + } + + llvm::DenseSet UnresolvedParams; + std::deque TraversalQueue; + llvm::DenseSet TraversedFunctions; + llvm::DenseMap ForwardedParams; + // For parameters that don't have a name in the function declaration, but do + // have one in the definition, store this mapping here. + llvm::DenseMap ParamDeclToDef; +}; + class InlayHintVisitor : public RecursiveASTVisitor { public: InlayHintVisitor(std::vector &Results, ParsedAST &AST, @@ -392,21 +555,29 @@ if (Ctor->isCopyOrMoveConstructor()) return; - // Don't show hints for variadic parameters. - size_t FixedParamCount = getFixedParamCount(Callee); - size_t ArgCount = std::min(FixedParamCount, Args.size()); - auto Params = Callee->parameters(); + SmallVector Params{Callee->param_begin(), + Callee->param_end()}; + + FwdVisitor.resolveForwardedParameters(Callee, Params); - NameVec ParameterNames = chooseParameterNames(Callee, ArgCount); + NameVec ParameterNames = chooseParameterNames(Params); // Exclude setters (i.e. functions with one argument whose name begins with // "set"), as their parameter name is also not likely to be interesting. if (isSetter(Callee, ParameterNames)) return; - for (size_t I = 0; I < ArgCount; ++I) { + // Exclude builtins like std::move and std::forward + if (Callee->getBuiltinID() == Builtin::BImove || + Callee->getBuiltinID() == Builtin::BIforward) + return; + + for (size_t I = 0; I < Params.size(); ++I) { StringRef Name = ParameterNames[I]; - bool NameHint = shouldHintName(Args[I], Name); + // Skip unexpanded pack expansion expressions + if (I >= Args.size() || isa(Args[I])) + break; + bool NameHint = shouldHintName(Args[I], Params[I], Name); bool ReferenceHint = shouldHintReference(Params[I]); if (NameHint || ReferenceHint) { @@ -440,7 +611,8 @@ return WhatItIsSetting.equals_insensitive(ParamNames[0]); } - bool shouldHintName(const Expr *Arg, StringRef ParamName) { + bool shouldHintName(const Expr *Arg, const ParmVarDecl *Param, + StringRef ParamName) { if (ParamName.empty()) return false; @@ -453,6 +625,11 @@ if (isPrecededByParamNameComment(Arg, ParamName)) return false; + // Exclude parameters that are unresolved parameter packs + if (isExpandedParameter(Param)) { + return false; + } + return true; } @@ -507,23 +684,12 @@ return {}; } - NameVec chooseParameterNames(const FunctionDecl *Callee, size_t ArgCount) { - // The current strategy here is to use all the parameter names from the - // canonical declaration, unless they're all empty, in which case we - // use all the parameter names from the definition (in present in the - // translation unit). - // We could try a bit harder, e.g.: - // - try all re-declarations, not just canonical + definition - // - fall back arg-by-arg rather than wholesale - - NameVec ParameterNames = getParameterNamesForDecl(Callee, ArgCount); - - if (llvm::all_of(ParameterNames, std::mem_fn(&StringRef::empty))) { - if (const FunctionDecl *Def = Callee->getDefinition()) { - ParameterNames = getParameterNamesForDecl(Def, ArgCount); - } + NameVec + chooseParameterNames(const SmallVector Parameters) { + NameVec ParameterNames(Parameters.size()); + for (size_t I = 0; I < Parameters.size(); I++) { + ParameterNames[I] = getSimpleName(*Parameters[I]); } - assert(ParameterNames.size() == ArgCount); // Standard library functions often have parameter names that start // with underscores, which makes the hints noisy, so strip them out. @@ -537,26 +703,6 @@ Name = Name.ltrim('_'); } - // Return the number of fixed parameters Function has, that is, not counting - // parameters that are variadic (instantiated from a parameter pack) or - // C-style varargs. - static size_t getFixedParamCount(const FunctionDecl *Function) { - if (FunctionTemplateDecl *Template = Function->getPrimaryTemplate()) { - FunctionDecl *F = Template->getTemplatedDecl(); - size_t Result = 0; - for (ParmVarDecl *Parm : F->parameters()) { - if (Parm->isParameterPack()) { - break; - } - ++Result; - } - return Result; - } - // C-style varargs don't need special handling, they're already - // not included in getNumParams(). - return Function->getNumParams(); - } - static StringRef getSimpleName(const NamedDecl &D) { if (IdentifierInfo *Ident = D.getDeclName().getAsIdentifierInfo()) { return Ident->getName(); @@ -565,17 +711,6 @@ return StringRef(); } - NameVec getParameterNamesForDecl(const FunctionDecl *Function, - size_t ArgCount) { - NameVec Result; - for (size_t I = 0; I < ArgCount; ++I) { - const ParmVarDecl *Parm = Function->getParamDecl(I); - assert(Parm); - Result.emplace_back(getSimpleName(*Parm)); - } - return Result; - } - // We pass HintSide rather than SourceLocation because we want to ensure // it is in the same file as the common file range. void addInlayHint(SourceRange R, HintSide Side, InlayHintKind Kind, @@ -645,6 +780,7 @@ FileID MainFileID; StringRef MainFileBuf; const HeuristicResolver *Resolver; + ForwardingParameterVisitor FwdVisitor; // We want to suppress default template arguments, but otherwise print // canonical types. Unfortunately, they're conflicting policies so we can't // have both. For regular types, suppressing template arguments is more 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 @@ -170,6 +170,43 @@ )cpp"); } +TEST(ParameterHints, NoNameVariadicDeclaration) { + // No hint for anonymous variadic parameter + assertParameterHints(R"cpp( + template + void foo(Args&& ...); + void bar() { + foo(42); + } + )cpp"); +} + +TEST(ParameterHints, NoNameVariadicForwarded) { + // No hint for anonymous variadic parameter + // The forward prototype is not correct, but is converted into builtin anyways + assertParameterHints(R"cpp( + namespace std { template T&& forward(T&); } + void foo(int); + template + void bar(Args&&... args) { return foo(std::forward(args)...); } + void baz() { + bar(42); + } + )cpp"); +} + +TEST(ParameterHints, NoNameVariadicPlain) { + // No hint for anonymous variadic parameter + assertParameterHints(R"cpp( + void foo(int); + template + void bar(Args&&... args) { return foo(args...); } + void baz() { + bar(42); + } + )cpp"); +} + TEST(ParameterHints, NameInDefinition) { // Parameter name picked up from definition if necessary. assertParameterHints(R"cpp( @@ -254,6 +291,123 @@ ExpectedHint{"param: ", "param"}); } +TEST(ParameterHints, VariadicForwardedConstructor) { + // Name hint for variadic parameter + // The forward prototype is not correct, but is converted into builtin anyways + assertParameterHints(R"cpp( + namespace std { template T&& forward(T&); } + struct S { S(int a); }; + template + T bar(Args&&... args) { return T{std::forward(args)...}; } + void baz() { + int b; + bar($param[[b]]); + } + )cpp", + ExpectedHint{"a: ", "param"}); +} + +TEST(ParameterHints, VariadicPlainConstructor) { + // Name hint for variadic parameter + assertParameterHints(R"cpp( + struct S { S(int a); }; + template + T bar(Args&&... args) { return T{args...}; } + void baz() { + int b; + bar($param[[b]]); + } + )cpp", + ExpectedHint{"a: ", "param"}); +} + +TEST(ParameterHints, VariadicForwardedNewConstructor) { + // Name hint for variadic parameter + // The forward prototype is not correct, but is converted into builtin anyways + assertParameterHints(R"cpp( + namespace std { template T&& forward(T&); } + struct S { S(int a); }; + template + T* bar(Args&&... args) { return new T{std::forward(args)...}; } + void baz() { + int b; + bar($param[[b]]); + } + )cpp", + ExpectedHint{"a: ", "param"}); +} + +TEST(ParameterHints, VariadicPlainNewConstructor) { + // Name hint for variadic parameter + assertParameterHints(R"cpp( + struct S { S(int a); }; + template + T* bar(Args&&... args) { return new T{args...}; } + void baz() { + int b; + bar($param[[b]]); + } + )cpp", + ExpectedHint{"a: ", "param"}); +} + +TEST(ParameterHints, VariadicForwarded) { + // Name for variadic parameter + // The forward prototype is not correct, but is converted into builtin anyways + assertParameterHints(R"cpp( + namespace std { template T&& forward(T&); } + void foo(int a); + template + void bar(Args&&... args) { return foo(std::forward(args)...); } + void baz() { + int b; + bar($param[[b]]); + } + )cpp", + ExpectedHint{"a: ", "param"}); +} + +TEST(ParameterHints, VariadicPlain) { + // Name hint for variadic parameter + assertParameterHints(R"cpp( + void foo(int a); + template + void bar(Args&&... args) { return foo(args...); } + void baz() { + bar($param[[42]]); + } + )cpp", + ExpectedHint{"a: ", "param"}); +} + +TEST(ParameterHints, MatchingNameVariadicForwarded) { + // No name hint for variadic parameter with matching name + // The forward prototype is not correct, but is converted into builtin anyways + assertParameterHints(R"cpp( + namespace std { template T&& forward(T&); } + void foo(int a); + template + void bar(Args&&... args) { return foo(std::forward(args)...); } + void baz() { + int a; + bar(a); + } + )cpp"); +} + +TEST(ParameterHints, MatchingNameVariadicPlain) { + // No name hint for variadic parameter with matching name + assertParameterHints(R"cpp( + void foo(int a); + template + void bar(Args&&... args) { return foo(args...); } + void baz() { + int a; + bar(a); + } + )cpp"); +} + TEST(ParameterHints, Operator) { // No hint for operator call with operator syntax. assertParameterHints(R"cpp(