[Clang] Improve diagnostics for 'placement new' with const storage argument

[vbvictor]

Before this patch, the following code gave misleading diagnostics about absence of #include <new>:

#include <new>

struct X { int n; };
int foo() {
  const X cx = {5};
  // error: no matching 'operator new' function for non-allocating placement new expression; include <new>
  (void)new(&cx) X{10};
};

Now it gives correct diagnostics about constness of passed argument:

#include <new>

struct X { int n; };
int foo() {
  const X cx = {5};
  // error: placement new expression with a const-qualified argument of type 'const X *' is not allowed
  (void)new(&cx) X{10};
};

Fixes #143708.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Baranov Victor (vbvictor)

Changes

Before this patch, the following code gave misleading diagnostics about absence of #include <new>:

struct X { int n; };
int foo() {
  const X cx = {5};
  // error: no matching 'operator new' function for non-allocating placement new expression; include <new>
  (void)new(&cx) X{10};
};

Now it gives correct diagnostics about constness of passed argument:

struct X { int n; };
int foo() {
  const X cx = {5};
  // error: placement new expression with a const-qualified argument of type 'const X *' is not allowed
  (void)new(&cx) X{10};
};

Fixes #143708.

---

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

4 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+2)
• (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+3)
• (modified) clang/lib/Sema/SemaExprCXX.cpp (+9-1)
• (modified) clang/test/SemaCXX/new-delete.cpp (+7)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 33ee8a53b5f37..9fdc8660a981e 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -629,6 +629,8 @@ Improvements to Clang's diagnostics
   #GH69470, #GH59391, #GH58172, #GH46215, #GH45915, #GH45891, #GH44490,
   #GH36703, #GH32903, #GH23312, #GH69874.
 
+- Improve the diagnostics for placement new expression when const-qualified
+  object was passed as the storage argument.
 
 Improvements to Clang's time-trace
 ----------------------------------
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 8fe7ad6138aa0..0bc1e7deff5ab 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -8285,6 +8285,9 @@ def err_need_header_before_typeid : Error<
 def err_need_header_before_placement_new : Error<
   "no matching %0 function for non-allocating placement new expression; "
   "include <new>">;
+def err_placement_new_into_const_qualified_storage : Error<
+  "placement new expression with a const-qualified argument of type %0 "
+  "is not allowed">;
 def err_ms___leave_not_in___try : Error<
   "'__leave' statement not in __try block">;
 def err_uuidof_without_guid : Error<
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index ba52e8f8932d3..e9cd40b9061d0 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -2753,10 +2753,18 @@ static bool resolveAllocationOverloadInterior(
     if (Diagnose) {
       // If this is an allocation of the form 'new (p) X' for some object
       // pointer p (or an expression that will decay to such a pointer),
-      // diagnose the missing inclusion of <new>.
+      // diagnose potential error.
       if (!R.isClassLookup() && Args.size() == 2 &&
           (Args[1]->getType()->isObjectPointerType() ||
            Args[1]->getType()->isArrayType())) {
+        if (Args[1]->getType()->isPointerType()) {
+          if (Args[1]->getType()->getPointeeType().isConstQualified()) {
+            S.Diag(Args[1]->getExprLoc(),
+                   diag::err_placement_new_into_const_qualified_storage)
+                << Args[1]->getType() << Args[1]->getSourceRange();
+            return true;
+          }
+        }
         S.Diag(R.getNameLoc(), diag::err_need_header_before_placement_new)
             << R.getLookupName() << Range;
         // Listing the candidates is unlikely to be useful; skip it.
diff --git a/clang/test/SemaCXX/new-delete.cpp b/clang/test/SemaCXX/new-delete.cpp
index 9bbee32c58c36..fae9487cbe2c1 100644
--- a/clang/test/SemaCXX/new-delete.cpp
+++ b/clang/test/SemaCXX/new-delete.cpp
@@ -160,6 +160,13 @@ void bad_news(int *ip)
 #if __cplusplus < 201103L
   (void)new int[]{}; // expected-error {{array size must be specified in new expression with no initializer}}
 #endif
+  struct X { int n; };
+  const X cx = {5};
+  (void)new(&cx) X{10}; // expected-error {{placement new expression with a const-qualified argument of type 'const X *' is not allowed}}
+  const X* const cx2 = 0;
+  (void)new(cx2) X{10}; // expected-error {{placement new expression with a const-qualified argument of type 'const X *const' is not allowed}}
+  const int arr[1] = {1};
+  (void)new(&arr[0]) int(10); // expected-error {{placement new expression with a const-qualified argument of type 'const int *' is not allowed}}
 }
 
 void no_matching_placement_new() {

[vbvictor]

Failed "Test documentation build" also fails without my commit (tested locally), so it is unrelated

[ojhunt]

pedantic: "diagnose the reason for the error."

[ojhunt]

I think this would be improved by the addition of something to the effect of

QualType Arg1Type = Args[1]->getType();

And then just reusing that.

But I think this check is in the wrong place, and should probably be earlier - this is inside an array of object check so I suspect the existing bad error message would likely occur with something like

void f(const void* ptr) {
  new (ptr) int;
}

you probably want something to the effect of

if (Context.getBaseElementType(Arg1Type).isConstQualified()) {
   emit the error
   return true;
}