[Sema] Fix assertion when `auto` parameter in lambda has an attribute.

[vsapsai]

Summary

Fixes the assertion

no Attr* for AttributedType*
UNREACHABLE executed at llvm-project/clang/lib/Sema/SemaType.cpp:298!

In TypeProcessingState::getAttributedType we put into AttrsForTypes
types with auto but later in
TypeProcessingState::takeAttrForAttributedType we use transformed
types and that's why cannot find Attr corresponding to
AttributedType.

Fix by keeping AttrsForTypes up to date after replacing AutoType.

rdar://problem/47689465

[vsapsai]

Have doubts that checking only the outermost type for being AttributedType after Sema::ReplaceAutoType is sufficient but haven't found a counterexample yet. Decided to post the proposed fix to see if others have any ideas.

[jkorous]

clang/lib/Sema/SemaType.cpp

266 ↗

(On Diff #188283)

Do you think it would make sense to terminate the loop early if AttrsForTypesSorted == true?

270 ↗

(On Diff #188283)

Do you think it would be reasonable to conditionally assign (or rather keep) AttrsForTypesSorted = true in case they were sorted previously and new key doesn't break the order?

[vsapsai]

clang/lib/Sema/SemaType.cpp

266 ↗

(On Diff #188283)

I agree with you it would be nice to reuse AttrsForTypesSorted property but in practice it doesn't matter. We sort AttrsForTypes only in takeAttrForAttributedType and we should do that after we've populated AttrsForTypes and after we've replaced auto types. It isn't strictly required but that's how it was working in debugger. Also I've tried to add assert(! AttrsForTypesSorted); and it wasn't triggered during ninja check-clang

Having AttrsForTypes sorted can be useful in the future but I don't think we should add code for that now.

Despite the linear iteration, we are not pessimizing the most common use case with no attributes for lambda parameters (expression "most common use case" is based on my experience and not substantiated by any data). So I think the performance shouldn't degrade noticeably.

[aaron.ballman]

clang/lib/Sema/SemaType.cpp

261 ↗

(On Diff #188283)

I think this work should be done within SubstituteDeducedTypeTransform rather than on the side. Any caller to Sema::ReplaceAutoType() should get this same behavior.

[vsapsai]

clang/lib/Sema/SemaType.cpp

261 ↗

(On Diff #188283)

Doing this work in SubstituteDeducedTypeTransform involves teaching SubstituteDeducedTypeTransform about TypeProcessingState and probably adding TypeProcessingState to Sema::ReplaceAutoType(). As for me, it exposes TypeProcessingState in more places than necessary. And it feels somewhat awkward that SubstituteDeducedTypeTransform is used in multiple places but TypeProcessingState is required only here.

I've modelled my approach after TypeProcessingState::getAttributedType where it forwards the call to Sema and keeps AttrsForTypes up to date. Do you still think SubstituteDeducedTypeTransform would be a better place for this code?

[aaron.ballman]

LGTM!

clang/lib/Sema/SemaType.cpp

261 ↗

(On Diff #188283)

Hmm, yeah, it doesn't seem like it would make sense there. This is keeping the TypeProcessingState up to date which is really only of interest during some stages of processing.

[vsapsai]

Thanks for the review, Aaron.

cfe/trunk/lib/Sema/SemaType.cpp

QualType getAttributedType(Attr *A, QualType ModifiedType,
                                QualType EquivType) {
       QualType T =
           sema.Context.getAttributedType(A->getKind(), ModifiedType, EquivType);
       AttrsForTypes.push_back({cast<AttributedType>(T.getTypePtr()), A});
       AttrsForTypesSorted = false;
       return T;
     }
 
+    /// Completely replace the \c auto in \p TypeWithAuto by
+    /// \p Replacement. Also replace \p TypeWithAuto in \c TypeAttrPair if
+    /// necessary.
+    QualType ReplaceAutoType(QualType TypeWithAuto, QualType Replacement) {
+      QualType T = sema.ReplaceAutoType(TypeWithAuto, Replacement);
+      if (auto *AttrTy = TypeWithAuto->getAs<AttributedType>()) {
+        // Attributed type still should be an attributed type after replacement.
+        auto *NewAttrTy = cast<AttributedType>(T.getTypePtr());
+        for (TypeAttrPair &A : AttrsForTypes) {
+          if (A.first == AttrTy)
+            A.first = NewAttrTy;
+        }
+        AttrsForTypesSorted = false;
+      }
+      return T;
+    }
+
     /// Extract and remove the Attr* for a given attributed type.
     const Attr *takeAttrForAttributedType(const AttributedType *AT) {
       if (!AttrsForTypesSorted) {
         std::stable_sort(AttrsForTypes.begin(), AttrsForTypes.end(),
                          [](const TypeAttrPair &A, const TypeAttrPair &B) {
                            return A.first < B.first;
                          });
         AttrsForTypesSorted = true;
case DeclaratorContext::LambdaExprParameterContext:
                 /*KeyLoc*/ SourceLocation(), /*NameLoc*/ D.getBeginLoc(),
                 TemplateParameterDepth, AutoParameterPosition,
                 /*Identifier*/ nullptr, false, IsParameterPack);
         LSI->AutoTemplateParams.push_back(CorrespondingTemplateParam);
         // Replace the 'auto' in the function parameter with this invented
         // template type parameter.
         // FIXME: Retain some type sugar to indicate that this was written
         // as 'auto'.
-        T = SemaRef.ReplaceAutoType(
+        T = state.ReplaceAutoType(
             T, QualType(CorrespondingTemplateParam->getTypeForDecl(), 0));
       }
       break;
     case DeclaratorContext::MemberContext: {
       if (D.getDeclSpec().getStorageClassSpec() == DeclSpec::SCS_static ||
           D.isFunctionDeclarator())
         break;
       bool Cxx = SemaRef.getLangOpts().CPlusPlus;

cfe/trunk/test/SemaCXX/auto-cxx0x.cpp

 thread_local auto x; // expected-error {{requires an initializer}}
 
 void g() {
   [](auto){}(0);
 #if __cplusplus == 201103L
   // expected-error@-2 {{'auto' not allowed in lambda parameter}}
 #endif
 }
+
+void rdar47689465() {
+  int x = 0;
+  [](auto __attribute__((noderef)) *){}(&x);
+#if __cplusplus == 201103L
+  // expected-error@-2 {{'auto' not allowed in lambda parameter}}
+#endif
+}