[clang-tools-extra] 5c46fef - [clangd] Improve hover on arguments to function call
Kadir Cetinkaya via cfe-commits
cfe-commits at lists.llvm.org
Fri Jul 3 02:51:32 PDT 2020
Author: Adam Czachorowski
Date: 2020-07-03T11:51:15+02:00
New Revision: 5c46fefdba3b18329dc331e69d1d1d6550c4075f
URL: https://github.com/llvm/llvm-project/commit/5c46fefdba3b18329dc331e69d1d1d6550c4075f
DIFF: https://github.com/llvm/llvm-project/commit/5c46fefdba3b18329dc331e69d1d1d6550c4075f.diff
LOG: [clangd] Improve hover on arguments to function call
Summary:
In cases like:
foo(a, ^b);
We now additionally show the name and type of the parameter to foo that
corresponds that "b" is passed as.
The name should help with understanding what it's used for and type can
be useful to find out if call to foo() can mutate variable "b" or not
(i.e. if it is pass by value, reference, const reference, etc).
Patch By: adamcz@ !
Reviewers: kadircet
Reviewed By: kadircet
Subscribers: nridge, ilya-biryukov, MaskRay, jkorous, arphaman, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D81169
Added:
Modified:
clang-tools-extra/clangd/Hover.cpp
clang-tools-extra/clangd/Hover.h
clang-tools-extra/clangd/unittests/HoverTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/Hover.cpp b/clang-tools-extra/clangd/Hover.cpp
index bd5b7114cc35..616d4c91c0f7 100644
--- a/clang-tools-extra/clangd/Hover.cpp
+++ b/clang-tools-extra/clangd/Hover.cpp
@@ -289,23 +289,27 @@ const Expr *getDefaultArg(const ParmVarDecl *PVD) {
: PVD->getDefaultArg();
}
+HoverInfo::Param toHoverInfoParam(const ParmVarDecl *PVD,
+ const PrintingPolicy &Policy) {
+ HoverInfo::Param Out;
+ Out.Type = printType(PVD->getType(), Policy);
+ if (!PVD->getName().empty())
+ Out.Name = PVD->getNameAsString();
+ if (const Expr *DefArg = getDefaultArg(PVD)) {
+ Out.Default.emplace();
+ llvm::raw_string_ostream OS(*Out.Default);
+ DefArg->printPretty(OS, nullptr, Policy);
+ }
+ return Out;
+}
+
// Populates Type, ReturnType, and Parameters for function-like decls.
void fillFunctionTypeAndParams(HoverInfo &HI, const Decl *D,
const FunctionDecl *FD,
const PrintingPolicy &Policy) {
HI.Parameters.emplace();
- for (const ParmVarDecl *PVD : FD->parameters()) {
- HI.Parameters->emplace_back();
- auto &P = HI.Parameters->back();
- P.Type = printType(PVD->getType(), Policy);
- if (!PVD->getName().empty())
- P.Name = PVD->getNameAsString();
- if (const Expr *DefArg = getDefaultArg(PVD)) {
- P.Default.emplace();
- llvm::raw_string_ostream Out(*P.Default);
- DefArg->printPretty(Out, nullptr, Policy);
- }
- }
+ for (const ParmVarDecl *PVD : FD->parameters())
+ HI.Parameters->emplace_back(toHoverInfoParam(PVD, Policy));
// We don't want any type info, if name already contains it. This is true for
// constructors/destructors and conversion operators.
@@ -678,6 +682,92 @@ void addLayoutInfo(const NamedDecl &ND, HoverInfo &HI) {
}
}
+// If N is passed as argument to a function, fill HI.CalleeArgInfo with
+// information about that argument.
+void maybeAddCalleeArgInfo(const SelectionTree::Node *N, HoverInfo &HI,
+ const PrintingPolicy &Policy) {
+ const auto &OuterNode = N->outerImplicit();
+ if (!OuterNode.Parent)
+ return;
+ const auto *CE = OuterNode.Parent->ASTNode.get<CallExpr>();
+ if (!CE)
+ return;
+ const FunctionDecl *FD = CE->getDirectCallee();
+ // For non-function-call-like operatators (e.g. operator+, operator<<) it's
+ // not immediattely obvious what the "passed as" would refer to and, given
+ // fixed function signature, the value would be very low anyway, so we choose
+ // to not support that.
+ // Both variadic functions and operator() (especially relevant for lambdas)
+ // should be supported in the future.
+ if (!FD || FD->isOverloadedOperator() || FD->isVariadic())
+ return;
+
+ // Find argument index for N.
+ for (unsigned I = 0; I < CE->getNumArgs() && I < FD->getNumParams(); ++I) {
+ if (CE->getArg(I) != OuterNode.ASTNode.get<Expr>())
+ continue;
+
+ // Extract matching argument from function declaration.
+ if (const ParmVarDecl *PVD = FD->getParamDecl(I))
+ HI.CalleeArgInfo.emplace(toHoverInfoParam(PVD, Policy));
+ break;
+ }
+ if (!HI.CalleeArgInfo)
+ return;
+
+ // If we found a matching argument, also figure out if it's a
+ // [const-]reference. For this we need to walk up the AST from the arg itself
+ // to CallExpr and check all implicit casts, constructor calls, etc.
+ HoverInfo::PassType PassType;
+ if (const auto *E = N->ASTNode.get<Expr>()) {
+ if (E->getType().isConstQualified())
+ PassType.PassBy = HoverInfo::PassType::ConstRef;
+ }
+
+ for (auto *CastNode = N->Parent;
+ CastNode != OuterNode.Parent && !PassType.Converted;
+ CastNode = CastNode->Parent) {
+ if (const auto *ImplicitCast = CastNode->ASTNode.get<ImplicitCastExpr>()) {
+ switch (ImplicitCast->getCastKind()) {
+ case CK_NoOp:
+ case CK_DerivedToBase:
+ case CK_UncheckedDerivedToBase:
+ // If it was a reference before, it's still a reference.
+ if (PassType.PassBy != HoverInfo::PassType::Value)
+ PassType.PassBy = ImplicitCast->getType().isConstQualified()
+ ? HoverInfo::PassType::ConstRef
+ : HoverInfo::PassType::Ref;
+ break;
+ case CK_LValueToRValue:
+ case CK_ArrayToPointerDecay:
+ case CK_FunctionToPointerDecay:
+ case CK_NullToPointer:
+ case CK_NullToMemberPointer:
+ // No longer a reference, but we do not show this as type conversion.
+ PassType.PassBy = HoverInfo::PassType::Value;
+ break;
+ default:
+ PassType.PassBy = HoverInfo::PassType::Value;
+ PassType.Converted = true;
+ break;
+ }
+ } else if (const auto *CtorCall =
+ CastNode->ASTNode.get<CXXConstructExpr>()) {
+ // We want to be smart about copy constructors. They should not show up as
+ // type conversion, but instead as passing by value.
+ if (CtorCall->getConstructor()->isCopyConstructor())
+ PassType.PassBy = HoverInfo::PassType::Value;
+ else
+ PassType.Converted = true;
+ } else { // Unknown implicit node, assume type conversion.
+ PassType.PassBy = HoverInfo::PassType::Value;
+ PassType.Converted = true;
+ }
+ }
+
+ HI.CallPassType.emplace(PassType);
+}
+
} // namespace
llvm::Optional<HoverInfo> getHover(ParsedAST &AST, Position Pos,
@@ -740,6 +830,7 @@ llvm::Optional<HoverInfo> getHover(ParsedAST &AST, Position Pos,
// Look for a close enclosing expression to show the value of.
if (!HI->Value)
HI->Value = printExprValue(N, AST.getASTContext());
+ maybeAddCalleeArgInfo(N, *HI, AST.getASTContext().getPrintingPolicy());
} else if (const Expr *E = N->ASTNode.get<Expr>()) {
HI = getHoverContents(E, AST);
}
@@ -820,6 +911,24 @@ markup::Document HoverInfo::present() const {
Output.addParagraph().appendText(
llvm::formatv("Size: {0} byte{1}", *Size, *Size == 1 ? "" : "s").str());
+ if (CalleeArgInfo) {
+ assert(CallPassType);
+ std::string Buffer;
+ llvm::raw_string_ostream OS(Buffer);
+ OS << "Passed ";
+ if (CallPassType->PassBy != HoverInfo::PassType::Value) {
+ OS << "by ";
+ if (CallPassType->PassBy == HoverInfo::PassType::ConstRef)
+ OS << "const ";
+ OS << "reference ";
+ }
+ if (CalleeArgInfo->Name)
+ OS << "as " << CalleeArgInfo->Name;
+ if (CallPassType->Converted && CalleeArgInfo->Type)
+ OS << " (converted to " << CalleeArgInfo->Type << ")";
+ Output.addParagraph().appendText(OS.str());
+ }
+
if (!Documentation.empty())
parseDocumentation(Documentation, Output);
@@ -844,6 +953,7 @@ markup::Document HoverInfo::present() const {
// non-c++ projects or projects that are not making use of namespaces.
Output.addCodeBlock(ScopeComment + DefinitionWithAccess);
}
+
return Output;
}
diff --git a/clang-tools-extra/clangd/Hover.h b/clang-tools-extra/clangd/Hover.h
index b712d844e33d..2f2afbf6723b 100644
--- a/clang-tools-extra/clangd/Hover.h
+++ b/clang-tools-extra/clangd/Hover.h
@@ -77,11 +77,31 @@ struct HoverInfo {
llvm::Optional<uint64_t> Size;
/// Contains the offset of fields within the enclosing class.
llvm::Optional<uint64_t> Offset;
+ // Set when symbol is inside function call. Contains information extracted
+ // from the callee definition about the argument this is passed as.
+ llvm::Optional<Param> CalleeArgInfo;
+ struct PassType {
+ // How the variable is passed to callee.
+ enum PassMode { Ref, ConstRef, Value };
+ PassMode PassBy = Ref;
+ // True if type conversion happened. This includes calls to implicit
+ // constructor, as well as built-in type conversions. Casting to base class
+ // is not considered conversion.
+ bool Converted = false;
+ };
+ // Set only if CalleeArgInfo is set.
+ llvm::Optional<PassType> CallPassType;
/// Produce a user-readable information.
markup::Document present() const;
};
+inline bool operator==(const HoverInfo::PassType &LHS,
+ const HoverInfo::PassType &RHS) {
+ return std::tie(LHS.PassBy, LHS.Converted) ==
+ std::tie(RHS.PassBy, RHS.Converted);
+}
+
// Try to infer structure of a documentation comment (e.g. line breaks).
// FIXME: move to another file so CodeComplete doesn't depend on Hover.
void parseDocumentation(llvm::StringRef Input, markup::Document &Output);
diff --git a/clang-tools-extra/clangd/unittests/HoverTests.cpp b/clang-tools-extra/clangd/unittests/HoverTests.cpp
index 78964ee7ab91..4059f8307c4c 100644
--- a/clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ b/clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -26,6 +26,8 @@ namespace clang {
namespace clangd {
namespace {
+using PassMode = HoverInfo::PassType::PassMode;
+
TEST(Hover, Structured) {
struct {
const char *const Code;
@@ -719,6 +721,57 @@ class Foo {})cpp";
// Bindings are in theory public members of an anonymous struct.
HI.AccessSpecifier = "public";
}},
+ {// Extra info for function call.
+ R"cpp(
+ void fun(int arg_a, int &arg_b) {};
+ void code() {
+ int a = 1, b = 2;
+ fun(a, [[^b]]);
+ }
+ )cpp",
+ [](HoverInfo &HI) {
+ HI.Name = "b";
+ HI.Kind = index::SymbolKind::Variable;
+ HI.NamespaceScope = "";
+ HI.Definition = "int b = 2";
+ HI.LocalScope = "code::";
+ HI.Value = "2";
+ HI.Type = "int";
+ HI.CalleeArgInfo.emplace();
+ HI.CalleeArgInfo->Name = "arg_b";
+ HI.CalleeArgInfo->Type = "int &";
+ HI.CallPassType.emplace();
+ HI.CallPassType->PassBy = PassMode::Ref;
+ HI.CallPassType->Converted = false;
+ }},
+ {// Extra info for method call.
+ R"cpp(
+ class C {
+ public:
+ void fun(int arg_a = 3, int arg_b = 4) {}
+ };
+ void code() {
+ int a = 1, b = 2;
+ C c;
+ c.fun([[^a]], b);
+ }
+ )cpp",
+ [](HoverInfo &HI) {
+ HI.Name = "a";
+ HI.Kind = index::SymbolKind::Variable;
+ HI.NamespaceScope = "";
+ HI.Definition = "int a = 1";
+ HI.LocalScope = "code::";
+ HI.Value = "1";
+ HI.Type = "int";
+ HI.CalleeArgInfo.emplace();
+ HI.CalleeArgInfo->Name = "arg_a";
+ HI.CalleeArgInfo->Type = "int";
+ HI.CalleeArgInfo->Default = "3";
+ HI.CallPassType.emplace();
+ HI.CallPassType->PassBy = PassMode::Value;
+ HI.CallPassType->Converted = false;
+ }},
};
for (const auto &Case : Cases) {
SCOPED_TRACE(Case.Code);
@@ -752,6 +805,85 @@ class Foo {})cpp";
EXPECT_EQ(H->Size, Expected.Size);
EXPECT_EQ(H->Offset, Expected.Offset);
EXPECT_EQ(H->AccessSpecifier, Expected.AccessSpecifier);
+ EXPECT_EQ(H->CalleeArgInfo, Expected.CalleeArgInfo);
+ EXPECT_EQ(H->CallPassType, Expected.CallPassType);
+ }
+}
+
+TEST(Hover, CallPassType) {
+ const llvm::StringRef CodePrefix = R"cpp(
+class Base {};
+class Derived : public Base {};
+class CustomClass {
+ public:
+ CustomClass() {}
+ CustomClass(const Base &x) {}
+ CustomClass(int &x) {}
+ CustomClass(float x) {}
+};
+
+void int_by_ref(int &x) {}
+void int_by_const_ref(const int &x) {}
+void int_by_value(int x) {}
+void base_by_ref(Base &x) {}
+void base_by_const_ref(const Base &x) {}
+void base_by_value(Base x) {}
+void float_by_value(float x) {}
+void custom_by_value(CustomClass x) {}
+
+void fun() {
+ int int_x;
+ int &int_ref = int_x;
+ const int &int_const_ref = int_x;
+ Base base;
+ const Base &base_const_ref = base;
+ Derived derived;
+ float float_x;
+)cpp";
+ const llvm::StringRef CodeSuffix = "}";
+
+ struct {
+ const char *const Code;
+ HoverInfo::PassType::PassMode PassBy;
+ bool Converted;
+ } Tests[] = {
+ // Integer tests
+ {"int_by_value([[^int_x]]);", PassMode::Value, false},
+ {"int_by_ref([[^int_x]]);", PassMode::Ref, false},
+ {"int_by_const_ref([[^int_x]]);", PassMode::ConstRef, false},
+ {"int_by_value([[^int_ref]]);", PassMode::Value, false},
+ {"int_by_const_ref([[^int_ref]]);", PassMode::ConstRef, false},
+ {"int_by_const_ref([[^int_ref]]);", PassMode::ConstRef, false},
+ {"int_by_const_ref([[^int_const_ref]]);", PassMode::ConstRef, false},
+ // Custom class tests
+ {"base_by_ref([[^base]]);", PassMode::Ref, false},
+ {"base_by_const_ref([[^base]]);", PassMode::ConstRef, false},
+ {"base_by_const_ref([[^base_const_ref]]);", PassMode::ConstRef, false},
+ {"base_by_value([[^base]]);", PassMode::Value, false},
+ {"base_by_value([[^base_const_ref]]);", PassMode::Value, false},
+ {"base_by_ref([[^derived]]);", PassMode::Ref, false},
+ {"base_by_const_ref([[^derived]]);", PassMode::ConstRef, false},
+ {"base_by_value([[^derived]]);", PassMode::Value, false},
+ // Converted tests
+ {"float_by_value([[^int_x]]);", PassMode::Value, true},
+ {"float_by_value([[^int_ref]]);", PassMode::Value, true},
+ {"float_by_value([[^int_const_ref]]);", PassMode::Value, true},
+ {"custom_by_value([[^int_x]]);", PassMode::Ref, true},
+ {"custom_by_value([[^float_x]]);", PassMode::Value, true},
+ {"custom_by_value([[^base]]);", PassMode::ConstRef, true},
+ };
+ for (const auto &Test : Tests) {
+ SCOPED_TRACE(Test.Code);
+
+ const auto Code = (CodePrefix + Test.Code + CodeSuffix).str();
+ Annotations T(Code);
+ TestTU TU = TestTU::withCode(T.code());
+ TU.ExtraArgs.push_back("-std=c++17");
+ auto AST = TU.build();
+ auto H = getHover(AST, T.point(), format::getLLVMStyle(), nullptr);
+ ASSERT_TRUE(H);
+ EXPECT_EQ(H->CallPassType->PassBy, Test.PassBy);
+ EXPECT_EQ(H->CallPassType->Converted, Test.Converted);
}
}
@@ -2043,6 +2175,106 @@ protected: int method())",
// In namespace ns1
private: union foo {})",
+ },
+ {
+ [](HoverInfo &HI) {
+ HI.Kind = index::SymbolKind::Variable;
+ HI.Name = "foo";
+ HI.Definition = "int foo = 3";
+ HI.LocalScope = "test::Bar::";
+ HI.Value = "3";
+ HI.Type = "int";
+ HI.CalleeArgInfo.emplace();
+ HI.CalleeArgInfo->Name = "arg_a";
+ HI.CalleeArgInfo->Type = "int";
+ HI.CalleeArgInfo->Default = "7";
+ HI.CallPassType.emplace();
+ HI.CallPassType->PassBy = PassMode::Value;
+ HI.CallPassType->Converted = false;
+ },
+ R"(variable foo
+
+Type: int
+Value = 3
+Passed as arg_a
+
+// In test::Bar
+int foo = 3)",
+ },
+ {
+ [](HoverInfo &HI) {
+ HI.Kind = index::SymbolKind::Variable;
+ HI.Name = "foo";
+ HI.Definition = "int foo = 3";
+ HI.LocalScope = "test::Bar::";
+ HI.Value = "3";
+ HI.Type = "int";
+ HI.CalleeArgInfo.emplace();
+ HI.CalleeArgInfo->Name = "arg_a";
+ HI.CalleeArgInfo->Type = "int";
+ HI.CalleeArgInfo->Default = "7";
+ HI.CallPassType.emplace();
+ HI.CallPassType->PassBy = PassMode::Ref;
+ HI.CallPassType->Converted = false;
+ },
+ R"(variable foo
+
+Type: int
+Value = 3
+Passed by reference as arg_a
+
+// In test::Bar
+int foo = 3)",
+ },
+ {
+ [](HoverInfo &HI) {
+ HI.Kind = index::SymbolKind::Variable;
+ HI.Name = "foo";
+ HI.Definition = "int foo = 3";
+ HI.LocalScope = "test::Bar::";
+ HI.Value = "3";
+ HI.Type = "int";
+ HI.CalleeArgInfo.emplace();
+ HI.CalleeArgInfo->Name = "arg_a";
+ HI.CalleeArgInfo->Type = "int";
+ HI.CalleeArgInfo->Default = "7";
+ HI.CallPassType.emplace();
+ HI.CallPassType->PassBy = PassMode::Value;
+ HI.CallPassType->Converted = true;
+ },
+ R"(variable foo
+
+Type: int
+Value = 3
+Passed as arg_a (converted to int)
+
+// In test::Bar
+int foo = 3)",
+ },
+ {
+ [](HoverInfo &HI) {
+ HI.Kind = index::SymbolKind::Variable;
+ HI.Name = "foo";
+ HI.Definition = "int foo = 3";
+ HI.LocalScope = "test::Bar::";
+ HI.Value = "3";
+ HI.Type = "int";
+ HI.CalleeArgInfo.emplace();
+ HI.CalleeArgInfo->Name = "arg_a";
+ HI.CalleeArgInfo->Type = "int";
+ HI.CalleeArgInfo->Default = "7";
+ HI.CallPassType.emplace();
+ HI.CallPassType->PassBy = PassMode::ConstRef;
+ HI.CallPassType->Converted = true;
+ },
+ R"(variable foo
+
+Type: int
+Value = 3
+Passed by const reference as arg_a (converted to int)
+
+// In test::Bar
+int foo = 3)",
}};
for (const auto &C : Cases) {
More information about the cfe-commits
mailing list