[clang-tools-extra] b1193c1 - [clangd] Avoid unexpected desugaring in isSugaredTemplateParameter
Younan Zhang via cfe-commits
cfe-commits at lists.llvm.org
Tue Aug 1 05:12:43 PDT 2023
Author: Younan Zhang
Date: 2023-08-01T20:12:35+08:00
New Revision: b1193c13a5f9e9a2dca6ae0dd8b4e911f54c09ce
URL: https://github.com/llvm/llvm-project/commit/b1193c13a5f9e9a2dca6ae0dd8b4e911f54c09ce
DIFF: https://github.com/llvm/llvm-project/commit/b1193c13a5f9e9a2dca6ae0dd8b4e911f54c09ce.diff
LOG: [clangd] Avoid unexpected desugaring in isSugaredTemplateParameter
This is a follow-up to D151785, addressing https://github.com/clangd/clangd/issues/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
Added:
Modified:
clang-tools-extra/clangd/InlayHints.cpp
clang-tools-extra/clangd/unittests/InlayHintTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/InlayHints.cpp b/clang-tools-extra/clangd/InlayHints.cpp
index 9247d212f73c45..1962aa51aef419 100644
--- a/clang-tools-extra/clangd/InlayHints.cpp
+++ b/clang-tools-extra/clangd/InlayHints.cpp
@@ -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;
}
diff --git a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
index 73aec4cdf2ff61..d38b875994e63c 100644
--- a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -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),
@@ -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 &&...) {}
@@ -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>
@@ -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];
@@ -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;
@@ -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) {
More information about the cfe-commits
mailing list