DR330: when determining whether a cast casts away constness, consider qualifiers from all levels matching a multidimensional array

[rsmith]

Summary

DR330: when determining whether a cast casts away constness, consider
qualifiers from all levels matching a multidimensional array.

For example, this allows casting from

pointer to       array of            array of   const volatile int

to

pointer to const pointer to volatile pointer to                int

because the multidimensional array part of the source type corresponds
to a part of the destination type that contains both 'const' and
'volatile'.

Fixes the bug reported here: https://github.com/apple/swift/pull/17882#discussion_r203196368

[rjmccall]

lib/Sema/SemaCast.cpp

535

Hmm. Just CVR? I understand that we can have problems here with the enumerated qualifiers, so maybe we shouldn't blindly merge them, but is there some way to propagate them if not conflicting?

[rsmith]

lib/Sema/SemaCast.cpp

535

When diagnosing casting away constness, we intentionally only look at CVR qualifiers rather than the full gamut of qualifiers, so that's all I propagated here. (The restriction on casting away qualifiers in reinterpret_cast is arbitrary nannying, and I don't think we should extend its scope. In any case, a const_cast rightly can't change non-CVR qualifiers, so if we diagnosed anything else there'd be no way to perform certain casts.)

That said, the caller (below) can also check for ObjC lifetime qualifier mismatches, so we should figure out what the right rule is for that case. Fortunately, if I understand correctly a lifetime qualifier can only appear at the innermost level in a cv-decomposition (we either have a pointer to a lifetime type, which can't be further decomposed, or a block pointer whose pointee must be a function type and therefore can't be further decomposed), so I don't think we need any special handling for that case.

If I'm wrong about that and we do actually support lifetime qualifiers anywhere other than the lowest level in a cv-decomposition, we'll need to figure out what to do when we have multiple inconsistent qualifiers. (But given that's our extension anyway, perhaps we could pick a rule that's more sane than the C++ standard's rule, such as only decomposing similar levels of types, and skipping all "array of" qualifiers on both sides regardless of how many there are on each, so the question doesn't arise.)

[rjmccall]

lib/Sema/SemaCast.cpp

535

No, that all makes sense to me, and you're right about the ARC ownership qualifiers. This seems reasonable to me.

...well, I do wonder if we're really getting much from UnwrapSimilarTypes here, and especially the way it pays attention to array sizes makes me anxious. But we don't need to change that now.

[davezarzycki]

Hi @rsmith – I've verified this patch fixes the original issue. Thanks for the quick fix!

include/clang/AST/ASTContext.h

bool hasSameNullabilityTypeQualifier(QualType SubT, QualType SuperT,
     }
     return true;
   }
 
   bool ObjCMethodsAreEqual(const ObjCMethodDecl *MethodDecl,
                            const ObjCMethodDecl *MethodImp);
 
   bool UnwrapSimilarTypes(QualType &T1, QualType &T2);
+  bool UnwrapSimilarArrayTypes(QualType &T1, QualType &T2);
 
   /// Determine if two types are similar, according to the C++ rules. That is,
   /// determine if they are the same other than qualifiers on the initial
   /// sequence of pointer / pointer-to-member / array (and in Clang, object
   /// pointer) types and their element types.
   ///
   /// Clang offers a number of qualifiers in addition to the C++ qualifiers;
   /// those qualifiers are also ignored in the 'similarity' check.

lib/AST/ASTContext.cpp

QualType ASTContext::getUnqualifiedArrayType(QualType type,
   return getDependentSizedArrayType(unqualElementType, DSAT->getSizeExpr(),
                                     DSAT->getSizeModifier(), 0,
                                     SourceRange());
 }
 
 /// Attempt to unwrap two types that may both be array types with the same bound
 /// (or both be array types of unknown bound) for the purpose of comparing the
 /// cv-decomposition of two types per C++ [conv.qual].
-static void unwrapSimilarArrayTypes(ASTContext &Ctx, QualType &T1,
-                                    QualType &T2) {
+bool ASTContext::UnwrapSimilarArrayTypes(QualType &T1, QualType &T2) {
+  bool UnwrappedAny = false;
   while (true) {
-    auto *AT1 = Ctx.getAsArrayType(T1);
-    if (!AT1) return;
+    auto *AT1 = getAsArrayType(T1);
+    if (!AT1) return UnwrappedAny;
 
-    auto *AT2 = Ctx.getAsArrayType(T2);
-    if (!AT2) return;
+    auto *AT2 = getAsArrayType(T2);
+    if (!AT2) return UnwrappedAny;
 
     // If we don't have two array types with the same constant bound nor two
     // incomplete array types, we've unwrapped everything we can.
     if (auto *CAT1 = dyn_cast<ConstantArrayType>(AT1)) {
       auto *CAT2 = dyn_cast<ConstantArrayType>(AT2);
       if (!CAT2 || CAT1->getSize() != CAT2->getSize())
-        return;
+        return UnwrappedAny;
     } else if (!isa<IncompleteArrayType>(AT1) ||
                !isa<IncompleteArrayType>(AT2)) {
-      return;
+      return UnwrappedAny;
     }
 
     T1 = AT1->getElementType();
     T2 = AT2->getElementType();
+    UnwrappedAny = true;
   }
 }
 
 /// Attempt to unwrap two types that may be similar (C++ [conv.qual]).
 ///
 /// If T1 and T2 are both pointer types of the same kind, or both array types
 /// with the same bound, unwraps layers from T1 and T2 until a pointer type is
 /// unwrapped. Top-level qualifiers on T1 and T2 are ignored.
 ///
 /// This function will typically be called in a loop that successively
 /// "unwraps" pointer and pointer-to-member types to compare them at each
 /// level.
 ///
 /// \return \c true if a pointer type was unwrapped, \c false if we reached a
 /// pair of types that can't be unwrapped further.
 bool ASTContext::UnwrapSimilarTypes(QualType &T1, QualType &T2) {
-  unwrapSimilarArrayTypes(*this, T1, T2);
+  UnwrapSimilarArrayTypes(T1, T2);
 
   const auto *T1PtrType = T1->getAs<PointerType>();
   const auto *T2PtrType = T2->getAs<PointerType>();
   if (T1PtrType && T2PtrType) {
     T1 = T1PtrType->getPointeeType();
     T2 = T2PtrType->getPointeeType();
     return true;
   }
-  
   const auto *T1MPType = T1->getAs<MemberPointerType>();
   const auto *T2MPType = T2->getAs<MemberPointerType>();
   if (T1MPType && T2MPType && 
       hasSameUnqualifiedType(QualType(T1MPType->getClass(), 0), 
                              QualType(T2MPType->getClass(), 0))) {
     T1 = T1MPType->getPointeeType();
     T2 = T2MPType->getPointeeType();
     return true;

lib/Sema/SemaCast.cpp

 };
 }
 
 /// Unwrap one level of types for CastsAwayConstness.
 ///
 /// Like Sema::UnwrapSimilarTypes, this removes one level of indirection from
 /// both types, provided that they're both pointer-like or array-like. Unlike
 /// the Sema function, doesn't care if the unwrapped pieces are related.
+///
+/// This function may remove additional levels as necessary for correctness:
+/// the resulting T1 is unwrapped sufficiently that it is never an array type,
+/// so that its qualifiers can be directly compared to those of T2 (which will
+/// have the combined set of qualifiers from all indermediate levels of T2),
+/// as (effectively) required by [expr.const.cast]p7 replacing T1's qualifiers
+/// with those from T2.
 static CastAwayConstnessKind
 unwrapCastAwayConstnessLevel(ASTContext &Context, QualType &T1, QualType &T2) {
-  // Note, even if this returns false, it may have unwrapped some number of
-  // matching "array of" pieces. That's OK, we don't need to check their
-  // cv-qualifiers (that check is covered by checking the qualifiers on the
-  // array types themselves).
-  if (Context.UnwrapSimilarTypes(T1, T2))
-    return CastAwayConstnessKind::CACK_Similar;
-
-  // Special case: if the destination type is a reference type, unwrap it as
-  // the first level.
-  if (T2->isReferenceType()) {
-    T2 = T2->getPointeeType();
-    return CastAwayConstnessKind::CACK_Similar;
-  }
-
+  enum { None, Ptr, MemPtr, BlockPtr, Array };
   auto Classify = [](QualType T) {
-    if (T->isAnyPointerType()) return 1;
-    if (T->isMemberPointerType()) return 2;
-    if (T->isBlockPointerType()) return 3;
+    if (T->isAnyPointerType()) return Ptr;
+    if (T->isMemberPointerType()) return MemPtr;
+    if (T->isBlockPointerType()) return BlockPtr;
     // We somewhat-arbitrarily don't look through VLA types here. This is at
     // least consistent with the behavior of UnwrapSimilarTypes.
-    if (T->isConstantArrayType() || T->isIncompleteArrayType()) return 4;
-    return 0;
+    if (T->isConstantArrayType() || T->isIncompleteArrayType()) return Array;
+    return None;
   };
 
-  int T1Class = Classify(T1);
-  if (!T1Class)
-    return CastAwayConstnessKind::CACK_None;
-
-  int T2Class = Classify(T2);
-  if (!T2Class)
-    return CastAwayConstnessKind::CACK_None;
-
   auto Unwrap = [&](QualType T) {
     if (auto *AT = Context.getAsArrayType(T))
       return AT->getElementType();
     return T->getPointeeType();
   };
 
+  CastAwayConstnessKind Kind;
+
+  if (T2->isReferenceType()) {
+    // Special case: if the destination type is a reference type, unwrap it as
+    // the first level. (The source will have been an lvalue expression in this
+    // case, so there is no corresponding "reference to" in T1 to remove.) This
+    // simulates removing a "pointer to" from both sides.
+    T2 = T2->getPointeeType();
+    Kind = CastAwayConstnessKind::CACK_Similar;
+  } else if (Context.UnwrapSimilarTypes(T1, T2)) {
+    Kind = CastAwayConstnessKind::CACK_Similar;
+  } else {
+    // Try unwrapping mismatching levels.
+    int T1Class = Classify(T1);
+    if (T1Class == None)
+      return CastAwayConstnessKind::CACK_None;
+
+    int T2Class = Classify(T2);
+    if (T2Class == None)
+      return CastAwayConstnessKind::CACK_None;
+
     T1 = Unwrap(T1);
     T2 = Unwrap(T2);
-  return T1Class == T2Class ? CastAwayConstnessKind::CACK_SimilarKind
+    Kind = T1Class == T2Class ? CastAwayConstnessKind::CACK_SimilarKind
                               : CastAwayConstnessKind::CACK_Incoherent;
   }
 
+  // We've unwrapped at least one level. If the resulting T1 is a (possibly
+  // multidimensional) array type, any qualifier on any matching layer of
+  // T2 is considered to correspond to T1. Decompose down to the element
+  // type of T1 so that we can compare properly.
+  while (true) {
+    Context.UnwrapSimilarArrayTypes(T1, T2);
+
+    if (Classify(T1) != Array)
+      break;
+
+    auto T2Class = Classify(T2);
+    if (T2Class == None)
+      break;
+
+    if (T2Class != Array)
+      Kind = CastAwayConstnessKind::CACK_Incoherent;
+    else if (Kind != CastAwayConstnessKind::CACK_Incoherent)
+      Kind = CastAwayConstnessKind::CACK_SimilarKind;
+
+    T1 = Unwrap(T1);
+    T2 = Unwrap(T2).withCVRQualifiers(T2.getCVRQualifiers());
+  }
+
+  return Kind;
+}
+
 /// Check if the pointer conversion from SrcType to DestType casts away
 /// constness as defined in C++ [expr.const.cast]. This is used by the cast
 /// checkers. Both arguments must denote pointer (possibly to member) types.
 ///
 /// \param CheckCVR Whether to check for const/volatile/restrict qualifiers.
 /// \param CheckObjCLifetime Whether to check Objective-C lifetime qualifiers.
 static CastAwayConstnessKind
 CastsAwayConstness(Sema &Self, QualType SrcType, QualType DestType,

test/CXX/drs/dr3xx.cpp

void f(P p, Q q, Q2 q2, R r, S s, T t) {
     (void) reinterpret_cast<S>(q2);
     (void) reinterpret_cast<Q>(s);
     (void) reinterpret_cast<Q2>(s);
     (void) reinterpret_cast<T>(s); // expected-error {{casts away qualifiers}}
     (void) reinterpret_cast<S>(t);
     (void) reinterpret_cast<T>(q); // expected-error {{casts away qualifiers}}
     (void) reinterpret_cast<Q>(t);
   }
+
+  namespace swift_17882 {
+    typedef const char P[72];
+    typedef int *Q;
+    void f(P &pr, P *pp) {
+      (void) reinterpret_cast<const Q&>(pr);
+      (void) reinterpret_cast<const Q*>(pp);
+    }
+
+    struct X {};
+    typedef const volatile int A[1][2][3];
+    typedef int *const X::*volatile *B1;
+    typedef int *const X::*         *B2;
+    typedef int *X::*      volatile *B3;
+    typedef volatile int *(*const B4)[4];
+    void f(A *a) {
+      (void) reinterpret_cast<B1*>(a);
+      (void) reinterpret_cast<B2*>(a); // expected-error {{casts away qualifiers}}
+      (void) reinterpret_cast<B3*>(a); // expected-error {{casts away qualifiers}}
+      (void) reinterpret_cast<B4*>(a);
+    }
+  }
 }
 
 namespace dr331 { // dr331: yes
   struct A {
     A(volatile A&); // expected-note {{candidate}}
   } const a, b(a); // expected-error {{no matching constructor}}
 }