Fix an assertion failure in unwrapSugar

[ahatanak]

Summary

An assertion in Qualifiers::addObjCLifetime fails when the ObjC lifetime bits are already set.

Instead of calling operator+=, call addConsistentQualifiers, which allows the lifetime bits to be set again as long the new value doesn't conflict with the old value.

This fixes https://github.com/llvm/llvm-project/issues/61419.

[ahatanak]

ping

[mizvekov]

clang/test/SemaObjCXX/arc-objc-lifetime.mm

69–79 ↗

(On Diff #509793)

I think the fix looks good, although this test may be out of place in this file.

Ideally, if I had thought of this test case, it would have gone into clang/test/SemaCXX/sugared-auto.cpp.

The existing tests there would try to trigger this bug through return type deduction, and not through the ternary operator as in your reduction.

A second good option would be to add it to clang/test/SemaCXX/sugar-common-types.cpp, as that tests ternary operator cases, but you would have to extend the compile flags to accept the ObjC++ stuff.

Also, if you want to add an isolated test for a bug in a file where it otherwise doesn't follow the style of the existing tests, we would isolate it in a namespace named after the PR.

Something like:

namespace PR61419 {
....
} // namespace PR61419

This makes it easier to extend and not accidentally break existing tests.

Also, it looks like this test could be a bit more minimal, as suggested in the edit.

[mizvekov]

LGTM

[ahatanak]

Simplify test case and move it to clang/test/SemaCXX/sugar-common-types.cpp.

clang/lib/AST/ASTContext.cpp

#undef UNEXPECTED_TYPE
   }
   }
   llvm_unreachable("Unhandled Type Class");
 }
 
 static auto unwrapSugar(SplitQualType &T, Qualifiers &QTotal) {
   SmallVector<SplitQualType, 8> R;
   while (true) {
-    QTotal += T.Quals;
+    QTotal.addConsistentQualifiers(T.Quals);
     QualType NT = T.Ty->getLocallyUnqualifiedSingleStepDesugaredType();
     if (NT == QualType(T.Ty, 0))
       break;
     R.push_back(T);
     T = NT.split();
   }
   return R;
 }

clang/test/SemaCXX/sugar-common-types.cpp

-// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++20 -fenable-matrix -triple i686-pc-win32
+// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++20 -x objective-c++ -fobjc-arc -fenable-matrix -triple i686-pc-win32
 
 enum class N {};
 
 using B1 = int;
 using X1 = B1;
 using Y1 = B1;
 
 using B2 = void;

 namespace C { using A::type1; };
 using UPX1 = C::type1;
 namespace A { using type1 = Y1*; };  // redeclared
 namespace C { using A::type1; };     // redeclared
 using UPY1 = C::type1;
 auto t32 = 0 ? (UPX1){} : (UPY1){};
 N t33 = t32;  // expected-error {{lvalue of type 'C::type1' (aka 'int *')}}
 N t34 = *t32; // expected-error {{lvalue of type 'B1' (aka 'int')}}
+
+// See https://github.com/llvm/llvm-project/issues/61419
+namespace PR61419 {
+  template <class T0, class T1> struct pair {
+    T0 first;
+    T1 second;
+  };
+
+  extern const pair<id, id> p;
+  id t = false ? p.first : p.second;
+} // namespace PR61419