Skip to content

Commit

Permalink
[clangd] Unify printing policy for type hints
Browse files Browse the repository at this point in the history
(This patch addresses the comment from https://reviews.llvm.org/D151785#4402460.)

Previously, we used a special printing policy that enabled `PrintCanonicalTypes`
to print type hints for structure bindings. This was intended to
eliminate type aliases like `tuple_element::type`. However, this also
caused TypePrinter to print default template arguments, which could
result in losing the ability to see types like `std::basic_string<char>`
if the fully expanded template-id exceeded the default inlay hint threshold.

Simply getting the canonical type at the call site could help us get rid of
the side effect.

This also merges overloaded `addTypeHint` into one function without
`PrintingPolicy`.

Reviewed By: nridge

Differential Revision: https://reviews.llvm.org/D152520
  • Loading branch information
zyn0217 committed Jun 13, 2023
1 parent 4cbedae commit 5cdb906
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 25 deletions.
33 changes: 9 additions & 24 deletions clang-tools-extra/clangd/InlayHints.cpp
Expand Up @@ -258,8 +258,7 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
Cfg(Cfg), RestrictRange(std::move(RestrictRange)),
MainFileID(AST.getSourceManager().getMainFileID()),
Resolver(AST.getHeuristicResolver()),
TypeHintPolicy(this->AST.getPrintingPolicy()),
StructuredBindingPolicy(this->AST.getPrintingPolicy()) {
TypeHintPolicy(this->AST.getPrintingPolicy()) {
bool Invalid = false;
llvm::StringRef Buf =
AST.getSourceManager().getBufferData(MainFileID, &Invalid);
Expand All @@ -269,14 +268,8 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
TypeHintPolicy.AnonymousTagLocations =
false; // do not print lambda locations

// For structured bindings, print canonical types. This is important because
// for bindings that use the tuple_element protocol, the non-canonical types
// would be "tuple_element<I, A>::type".
// For "auto", we often prefer sugared types.
// Not setting PrintCanonicalTypes for "auto" allows
// SuppressDefaultTemplateArgs (set by default) to have an effect.
StructuredBindingPolicy = TypeHintPolicy;
StructuredBindingPolicy.PrintCanonicalTypes = true;
}

bool VisitTypeLoc(TypeLoc TL) {
Expand Down Expand Up @@ -358,8 +351,12 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
// but show hints for the individual bindings.
if (auto *DD = dyn_cast<DecompositionDecl>(D)) {
for (auto *Binding : DD->bindings()) {
addTypeHint(Binding->getLocation(), Binding->getType(), /*Prefix=*/": ",
StructuredBindingPolicy);
// For structured bindings, print canonical types. This is important
// because for bindings that use the tuple_element protocol, the
// non-canonical types would be "tuple_element<I, A>::type".
if (auto Type = Binding->getType(); !Type.isNull())
addTypeHint(Binding->getLocation(), Type.getCanonicalType(),
/*Prefix=*/": ");
}
return true;
}
Expand Down Expand Up @@ -724,22 +721,17 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
}

void addTypeHint(SourceRange R, QualType T, llvm::StringRef Prefix) {
addTypeHint(R, T, Prefix, TypeHintPolicy);
}

void addTypeHint(SourceRange R, QualType T, llvm::StringRef Prefix,
const PrintingPolicy &Policy) {
if (!Cfg.InlayHints.DeducedTypes || T.isNull())
return;

// The sugared type is more useful in some cases, and the canonical
// type in other cases.
auto Desugared = maybeDesugar(AST, T);
std::string TypeName = Desugared.getAsString(Policy);
std::string TypeName = Desugared.getAsString(TypeHintPolicy);
if (T != Desugared && !shouldPrintTypeHint(TypeName)) {
// If the desugared type is too long to display, fallback to the sugared
// type.
TypeName = T.getAsString(Policy);
TypeName = T.getAsString(TypeHintPolicy);
}
if (shouldPrintTypeHint(TypeName))
addInlayHint(R, HintSide::Right, InlayHintKind::Type, Prefix, TypeName,
Expand All @@ -764,14 +756,7 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
FileID MainFileID;
StringRef MainFileBuf;
const HeuristicResolver *Resolver;
// 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
// important, whereas printing canonical types is crucial for structured
// bindings, so we use two separate policies. (See the constructor where
// the policies are initialized for more details.)
PrintingPolicy TypeHintPolicy;
PrintingPolicy StructuredBindingPolicy;
};

} // namespace
Expand Down
5 changes: 4 additions & 1 deletion clang-tools-extra/clangd/unittests/InlayHintTests.cpp
Expand Up @@ -1347,8 +1347,11 @@ TEST(TypeHints, DefaultTemplateArgs) {
struct A {};
A<float> foo();
auto $var[[var]] = foo();
A<float> bar[1];
auto [$binding[[value]]] = bar;
)cpp",
ExpectedHint{": A<float>", "var"});
ExpectedHint{": A<float>", "var"},
ExpectedHint{": A<float>", "binding"});
}

TEST(TypeHints, Deduplication) {
Expand Down

0 comments on commit 5cdb906

Please sign in to comment.