Check for mutability better

[DKLoehr]

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.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Devon Loehr (DKLoehr)

Changes

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.

---

Full diff: https://github.com/llvm/llvm-project/pull/127843.diff

4 Files Affected:

• (modified) clang/include/clang/AST/Type.h (+11)
• (modified) clang/lib/AST/Type.cpp (+11)
• (modified) clang/lib/Sema/SemaDecl.cpp (+2-16)
• (modified) clang/test/SemaCXX/unique_object_duplication.h (+5)

diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index 1d9743520654e..c844c58d134e9 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -1086,10 +1086,20 @@ class QualType {
   /// applied to this type.
   unsigned getCVRQualifiers() const;
 
+  // Determine whether the object seems constant (i.e., const-qualified)
   bool isConstant(const ASTContext& Ctx) const {
     return QualType::isConstant(*this, Ctx);
   }
 
+  // Determine whether the object seems mutable, (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.
+  bool isMutable(const ASTContext &Ctx) const {
+    return QualType::isMutable(*this, Ctx);
+  }
+
   /// Determine whether this is a Plain Old Data (POD) type (C++ 3.9p10).
   bool isPODType(const ASTContext &Context) const;
 
@@ -1617,6 +1627,7 @@ class QualType {
   // "static"-ize them to avoid creating temporary QualTypes in the
   // caller.
   static bool isConstant(QualType T, const ASTContext& Ctx);
+  static bool isMutable(QualType T, const ASTContext &Ctx);
   static QualType getDesugaredType(QualType T, const ASTContext &Context);
   static SplitQualType getSplitDesugaredType(QualType T);
   static SplitQualType getSplitUnqualifiedTypeImpl(QualType type);
diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp
index 8c11ec2e1fe24..28ecbdefb9422 100644
--- a/clang/lib/AST/Type.cpp
+++ b/clang/lib/AST/Type.cpp
@@ -139,6 +139,17 @@ bool QualType::isConstant(QualType T, const ASTContext &Ctx) {
   return T.getAddressSpace() == LangAS::opencl_constant;
 }
 
+bool QualType::isMutable(QualType T, const ASTContext &Ctx) {
+  T = T.getNonReferenceType();
+  if (T->isFunctionType())
+    return false;
+  if (!T.isConstant(Ctx))
+    return true;
+  if (T->isPointerType())
+    return QualType::isMutable(T->getPointeeType(), Ctx);
+  return false;
+}
+
 std::optional<QualType::NonConstantStorageReason>
 QualType::isNonConstantStorage(const ASTContext &Ctx, bool ExcludeCtor,
                             bool ExcludeDtor) {
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 362df485a025c..c712f4f53bdf5 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -13454,24 +13454,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 (Type.isMutable(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
diff --git a/clang/test/SemaCXX/unique_object_duplication.h b/clang/test/SemaCXX/unique_object_duplication.h
index a59a8f91da8b8..87303c75d7515 100644
--- a/clang/test/SemaCXX/unique_object_duplication.h
+++ b/clang/test/SemaCXX/unique_object_duplication.h
@@ -99,6 +99,10 @@ 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
+auto& allowedFunctionReference = has_static_locals_external;
+
 } // namespace StaticLocalTest
 
 /******************************************************************************
@@ -154,6 +158,7 @@ namespace GlobalTest {
   };
 
   inline float Test::disallowedStaticMember2 = 2.3; // hidden-warning {{'disallowedStaticMember2' may be duplicated when built into a shared library: it is mutable, has hidden visibility, and external linkage}}
+
 } // namespace GlobalTest
 
 /******************************************************************************

[zmodem]

I don't think we should make changes to the AST code for this. That just represents what's written in the code. For example, QualType::isConstant() doesn't "determine whether the object seems constant", it specifically checks if the type is const qualified. What that means is then a question for code in Sema and CodeGen and such.

QualType::isMutable() also runs the risk of being confused with the mutable specifier.

Instead, I think the right place for this logic is in Sema, close to the warning code that uses it.

[DKLoehr]

Makes sense, I'm happy to keep the scope small.

[zmodem]

LGTM with a nit.

I'll wait a day before merging in case anyone else wants to review as well.

[zmodem]

nit: make it static since it's local to this file

[DKLoehr]

Good call, changed.