[clangd] Avoid unexpected desugaring in isSugaredTemplateParameter

[zyounan]

Summary

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.

[zyounan]

Format

[zyounan]

Ping~

[nridge]

Your patience is appreciated. I have a number of patches in my review queue, and 3 days is often not a realistic turnaround time for me.

[zyounan]

Oops, my apologies for bothering you. Thanks again for the explanation and your dedication!

[nridge]

clang-tools-extra/clangd/InlayHints.cpp

408

nit: since it's only doing one level of peeling, let's call it PeelWrapper (no s)

410–411

Now that the function is not looping, we can simplify the body a bit:

QualType Peeled = QT->getPointeeType();
return Peeled.isNull() ? QT : Peeled;

415

Nice find, this is indeed pretty tricky. I remember running into a similar issue before in https://reviews.llvm.org/D124690#inline-1205484.

[zyounan]

Update

[zyounan]

Fix the chaos in the previous update

[zyounan]

Sorry, looks like I messed up with the commits before. Fixed it now.

clang-tools-extra/clangd/InlayHints.cpp

415

Cool. That looks far more intricate and I should take some time to understand the context. Thanks for the reference :)

clang-tools-extra/clangd/unittests/InlayHintTests.cpp

1444–1445

I feel like this test is becoming clunky: The validations here require an invented class template vector which I think should be better placed in a header; however, in the remaining snippet, it seems that I'm testing too many hints in one assertHint call. Should I split these cases into different snippets, or we just leave them as-is at the moment?

[nridge]

Thanks!

clang-tools-extra/clangd/unittests/InlayHintTests.cpp

1444–1445

I think this is fine as-is for now

[zyounan]

.

[zyounan]

Final update

clang-tools-extra/clangd/InlayHints.cpp

struct Namer : ConstStmtVisitor<Namer, std::string> {
     }
   };
   return Namer{}.Visit(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;
+    QualType Desugared = QT->getLocallyUnqualifiedSingleStepDesugaredType();
+    if (Desugared != QT)
       QT = Desugared;
+    else if (auto Peeled = PeelWrapper(Desugared); Peeled != QT)
+      QT = Peeled;
+    else
+      break;
   }
   return false;
 }
 
 // A simple wrapper for `clang::desugarForDiagnostic` that provides optional
 // semantic.
 std::optional<QualType> desugar(ASTContext &AST, QualType QT) {
   bool ShouldAKA = false;

clang-tools-extra/clangd/unittests/InlayHintTests.cpp

Config noHintsConfig() {
   C.InlayHints.Parameters = false;
   C.InlayHints.DeducedTypes = false;
   C.InlayHints.Designators = false;
   C.InlayHints.BlockEnd = false;
   return C;
 }
 
 template <typename... ExpectedHints>
-void assertHints(InlayHintKind Kind, llvm::StringRef AnnotatedSource,
+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),
               ElementsAre(HintMatcher(Expected, Source)...));
   // Sneak in a cross-cutting check that hints are disabled by config.
   // We'll hit an assertion failure if addInlayHint still gets called.
   WithContextValue WithCfg(Config::Key, noHintsConfig());
   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 &&...) {}
 
 template <typename... ExpectedHints>
 void assertParameterHints(llvm::StringRef AnnotatedSource,
                           ExpectedHints... Expected) {
   ignore(Expected.Side = Left...);
   assertHints(InlayHintKind::Parameter, AnnotatedSource, Expected...);
assertTypeHints(R"cpp(
     auto $j[[s]];
   )cpp",
                   ExpectedHint{": int", "a"}, ExpectedHint{": int", "b"},
                   ExpectedHint{": int", "c"}, ExpectedHint{": int", "e"},
                   ExpectedHint{": int", "f"}, ExpectedHint{": int", "g"},
                   ExpectedHint{": int", "h"}, ExpectedHint{": int", "i"});
 }
 
 TEST(TypeHints, SubstTemplateParameterAliases) {
-  assertTypeHints(
-      R"cpp(
+  llvm::StringRef Header = R"cpp(
   template <class T> struct allocator {};
 
   template <class T, class A>
   struct vector_base {
     using pointer = T*;
   };
 
   template <class T, class A>
struct vector : vector_base<T, A> {
     pointer data() { return &elements[0]; }
     allocator_type get_allocator() { return A(); }
     size_type size() const { return 10; }
     iterator begin() { return iterator(); }
     non_template_iterator end() { return non_template_iterator(); }
 
     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];
   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, 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;
   using Short = VeryLongLongTypeName;
 
   auto $short_name[[my_value]] = Short();
 
   // Same applies with templates.
   template <typename T, typename A>
   using basic_static_vector = vector<T, A>;
   template <typename T>
   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"},
+  )cpp";
+
+  assertHintsWithHeader(InlayHintKind::Type, TypeAlias, Header,
                         ExpectedHint{": Short", "short_name"},
                         ExpectedHint{": static_vector<int>", "vector_name"});
 }
 
 TEST(DesignatorHints, Basic) {
   assertDesignatorHints(R"cpp(
     struct S { int x, y, z; };
     S s {$x[[1]], $y[[2+2]]};
 
     int x[] = {$0[[0]], $1[[1]]};