Skip to content

Commit

Permalink
[clangd] Avoid unexpected desugaring in isSugaredTemplateParameter
Browse files Browse the repository at this point in the history 
This is a follow-up to D151785, addressing clangd/clangd#1703.

The previous approach of peeling pointer types during a traversal
using getPointeeType might have produced unexpected results; since
the method would implicitly desugar the type if the type being passed
in could not be cast to a Pointer-like Type.

Reviewed By: nridge

Differential Revision: https://reviews.llvm.org/D156300
  • Loading branch information
@zyn0217 @doru1004
zyn0217 authored and doru1004 committed Aug 3, 2023
1 parent f3706f3 commit 2e527dd
Show file tree
Hide file tree
Showing 2 changed files with 90 additions and 26 deletions.
47 changes: 36 additions & 11 deletions clang-tools-extra/clangd/InlayHints.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -405,22 +405,47 @@ std::string summarizeExpr(const Expr *E) {
// Determines if any intermediate type in desugaring QualType QT is of
// substituted template parameter type. Ignore pointer or reference wrappers.
bool isSugaredTemplateParameter(QualType QT) {
static auto PeelWrappers = [](QualType QT) {
static auto PeelWrapper = [](QualType QT) {
// Neither `PointerType` nor `ReferenceType` is considered as sugared
// type. Peel it.
QualType Next;
while (!(Next = QT->getPointeeType()).isNull())
QT = Next;
return QT;
QualType Peeled = QT->getPointeeType();
return Peeled.isNull() ? QT : Peeled;
};

// This is a bit tricky: we traverse the type structure and find whether or
// not a type in the desugaring process is of SubstTemplateTypeParmType.
// During the process, we may encounter pointer or reference types that are
// not marked as sugared; therefore, the desugar function won't apply. To
// move forward the traversal, we retrieve the pointees using
// QualType::getPointeeType().
//
// However, getPointeeType could leap over our interests: The QT::getAs<T>()
// invoked would implicitly desugar the type. Consequently, if the
// SubstTemplateTypeParmType is encompassed within a TypedefType, we may lose
// the chance to visit it.
// For example, given a QT that represents `std::vector<int *>::value_type`:
// `-ElaboratedType 'value_type' sugar
// `-TypedefType 'vector<int *>::value_type' sugar
// |-Typedef 'value_type'
// `-SubstTemplateTypeParmType 'int *' sugar class depth 0 index 0 T
// |-ClassTemplateSpecialization 'vector'
// `-PointerType 'int *'
// `-BuiltinType 'int'
// Applying `getPointeeType` to QT results in 'int', a child of our target
// node SubstTemplateTypeParmType.
//
// As such, we always prefer the desugared over the pointee for next type
// in the iteration. It could avoid the getPointeeType's implicit desugaring.
while (true) {
QualType Desugared =
PeelWrappers(QT->getLocallyUnqualifiedSingleStepDesugaredType());
if (Desugared == QT)
break;
if (Desugared->getAs<SubstTemplateTypeParmType>())
if (QT->getAs<SubstTemplateTypeParmType>())
return true;
QT = Desugared;
QualType Desugared = QT->getLocallyUnqualifiedSingleStepDesugaredType();
if (Desugared != QT)
QT = Desugared;
else if (auto Peeled = PeelWrapper(Desugared); Peeled != QT)
QT = Peeled;
else
break;
}
return false;
}
Expand Down
69 changes: 54 additions & 15 deletions clang-tools-extra/clangd/unittests/InlayHintTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,13 @@ Config noHintsConfig() {
}

template <typename... ExpectedHints>
void assertHints(InlayHintKind Kind, llvm::StringRef AnnotatedSource,
ExpectedHints... Expected) {
void assertHintsWithHeader(InlayHintKind Kind, llvm::StringRef AnnotatedSource,
llvm::StringRef HeaderContent,
ExpectedHints... Expected) {
Annotations Source(AnnotatedSource);
TestTU TU = TestTU::withCode(Source.code());
TU.ExtraArgs.push_back("-std=c++20");
TU.HeaderCode = HeaderContent;
auto AST = TU.build();

EXPECT_THAT(hintsOfKind(AST, Kind),
Expand All @@ -99,6 +101,13 @@ void assertHints(InlayHintKind Kind, llvm::StringRef AnnotatedSource,
EXPECT_THAT(inlayHints(AST, std::nullopt), IsEmpty());
}

template <typename... ExpectedHints>
void assertHints(InlayHintKind Kind, llvm::StringRef AnnotatedSource,
ExpectedHints... Expected) {
return assertHintsWithHeader(Kind, AnnotatedSource, "",
std::move(Expected)...);
}

// Hack to allow expression-statements operating on parameter packs in C++14.
template <typename... T> void ignore(T &&...) {}

Expand Down Expand Up @@ -1433,8 +1442,7 @@ TEST(TypeHints, Decltype) {
}

TEST(TypeHints, SubstTemplateParameterAliases) {
assertTypeHints(
R"cpp(
llvm::StringRef Header = R"cpp(
template <class T> struct allocator {};
template <class T, class A>
Expand Down Expand Up @@ -1467,9 +1475,34 @@ TEST(TypeHints, SubstTemplateParameterAliases) {
T elements[10];
};
)cpp";

vector<int> array;
llvm::StringRef VectorIntPtr = R"cpp(
vector<int *> array;
auto $no_modifier[[x]] = array[3];
auto* $ptr_modifier[[ptr]] = &array[3];
auto& $ref_modifier[[ref]] = array[3];
auto& $at[[immutable]] = array.at(3);
auto $data[[data]] = array.data();
auto $allocator[[alloc]] = array.get_allocator();
auto $size[[size]] = array.size();
auto $begin[[begin]] = array.begin();
auto $end[[end]] = array.end();
)cpp";

assertHintsWithHeader(
InlayHintKind::Type, VectorIntPtr, Header,
ExpectedHint{": int *", "no_modifier"},
ExpectedHint{": int **", "ptr_modifier"},
ExpectedHint{": int *&", "ref_modifier"},
ExpectedHint{": int *const &", "at"}, ExpectedHint{": int **", "data"},
ExpectedHint{": allocator<int *>", "allocator"},
ExpectedHint{": size_type", "size"}, ExpectedHint{": iterator", "begin"},
ExpectedHint{": non_template_iterator", "end"});

llvm::StringRef VectorInt = R"cpp(
vector<int> array;
auto $no_modifier[[by_value]] = array[3];
auto* $ptr_modifier[[ptr]] = &array[3];
auto& $ref_modifier[[ref]] = array[3];
Expand All @@ -1480,8 +1513,19 @@ TEST(TypeHints, SubstTemplateParameterAliases) {
auto $size[[size]] = array.size();
auto $begin[[begin]] = array.begin();
auto $end[[end]] = array.end();
)cpp";

assertHintsWithHeader(
InlayHintKind::Type, VectorInt, Header,
ExpectedHint{": int", "no_modifier"},
ExpectedHint{": int *", "ptr_modifier"},
ExpectedHint{": int &", "ref_modifier"},
ExpectedHint{": const int &", "at"}, ExpectedHint{": int *", "data"},
ExpectedHint{": allocator<int>", "allocator"},
ExpectedHint{": size_type", "size"}, ExpectedHint{": iterator", "begin"},
ExpectedHint{": non_template_iterator", "end"});

llvm::StringRef TypeAlias = R"cpp(
// If the type alias is not of substituted template parameter type,
// do not show desugared type.
using VeryLongLongTypeName = my_iterator;
Expand All @@ -1496,16 +1540,11 @@ TEST(TypeHints, SubstTemplateParameterAliases) {
using static_vector = basic_static_vector<T, allocator<T>>;
auto $vector_name[[vec]] = static_vector<int>();
)cpp",
ExpectedHint{": int", "no_modifier"},
ExpectedHint{": int *", "ptr_modifier"},
ExpectedHint{": int &", "ref_modifier"},
ExpectedHint{": const int &", "at"}, ExpectedHint{": int *", "data"},
ExpectedHint{": allocator<int>", "allocator"},
ExpectedHint{": size_type", "size"}, ExpectedHint{": iterator", "begin"},
ExpectedHint{": non_template_iterator", "end"},
ExpectedHint{": Short", "short_name"},
ExpectedHint{": static_vector<int>", "vector_name"});
)cpp";

assertHintsWithHeader(InlayHintKind::Type, TypeAlias, Header,
ExpectedHint{": Short", "short_name"},
ExpectedHint{": static_vector<int>", "vector_name"});
}

TEST(DesignatorHints, Basic) {
Expand Down

0 comments on commit 2e527dd

Please sign in to comment.