Check for mutability better (#127843)

This PR adds a function to determine if a type "looks" mutable. Since
it's impossible to be totally sure if something can or can't be modified
in C++, this makes a best-effort attempt to determine if variables of
that type can be modified or not.

The motivation for this change is the -Wunique-object-duplication
warning, which had a false positive due to a missed case while checking
for mutability. Pulling the logic into a separate function allows it to
be written much more cleanly. There should be no behavior change, except
that we no longer report function references to be mutable.

Author: DKLoehr

clang/lib/Sema/SemaDecl.cpp

@@ -13441,6 +13441,23 @@ bool Sema::GloballyUniqueObjectMightBeAccidentallyDuplicated(
   return true;
 }
 
+// Determine whether the object seems mutable for the purpose of diagnosing
+// possible unique object duplication, i.e. non-const-qualified, and
+// not an always-constant type like a function.
+// Not perfect: doesn't account for mutable members, for example, or
+// elements of container types.
+// For nested pointers, any individual level being non-const is sufficient.
+static bool looksMutable(QualType T, const ASTContext &Ctx) {
+  T = T.getNonReferenceType();
+  if (T->isFunctionType())
+    return false;
+  if (!T.isConstant(Ctx))
+    return true;
+  if (T->isPointerType())
+    return looksMutable(T->getPointeeType(), Ctx);
+  return false;
+}
+
 void Sema::DiagnoseUniqueObjectDuplication(const VarDecl *VD) {
   // If this object has external linkage and hidden visibility, it might be
   // duplicated when built into a shared library, which causes problems if it's
@@ -13455,24 +13472,10 @@ void Sema::DiagnoseUniqueObjectDuplication(const VarDecl *VD) {
       !VD->isTemplated() &&
       GloballyUniqueObjectMightBeAccidentallyDuplicated(VD)) {
 
-    // Check mutability. For pointers, ensure that both the pointer and the
-    // pointee are (recursively) const.
-    QualType Type = VD->getType().getNonReferenceType();
-    if (!Type.isConstant(VD->getASTContext())) {
+    QualType Type = VD->getType();
+    if (looksMutable(Type, VD->getASTContext())) {
       Diag(VD->getLocation(), diag::warn_possible_object_duplication_mutable)
           << VD;
-    } else {
-      while (Type->isPointerType()) {
-        Type = Type->getPointeeType();
-        if (Type->isFunctionType())
-          break;
-        if (!Type.isConstant(VD->getASTContext())) {
-          Diag(VD->getLocation(),
-               diag::warn_possible_object_duplication_mutable)
-              << VD;
-          break;
-        }
-      }
     }
 
     // To keep false positives low, only warn if we're certain that the

clang/test/SemaCXX/unique_object_duplication.h

@@ -99,6 +99,9 @@ inline void has_thread_local() {
   thread_local int disallowedThreadLocal = 0; // hidden-warning {{'disallowedThreadLocal' may be duplicated when built into a shared library: it is mutable, has hidden visibility, and external linkage}}
 }
 
+// Functions themselves are always immutable, so referencing them is okay
+inline auto& allowedFunctionReference = has_static_locals_external;
+
 } // namespace StaticLocalTest
 
 /******************************************************************************