[Clang] disallow the use of asterisks preceding constructor and destructor names

[a-tarasyuk]

Fixes #121706

[llvmbot]

@llvm/pr-subscribers-clang

Author: Oleksandr T. (a-tarasyuk)

Changes

Fixes #121706

---

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

5 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+1)
• (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+4)
• (modified) clang/lib/Sema/SemaDeclCXX.cpp (+13)
• (modified) clang/test/SemaCXX/constructor.cpp (+10)
• (modified) clang/test/SemaCXX/destructor.cpp (+7)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 197b3692b8a181..9e5bbbf6dc055a 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -764,6 +764,7 @@ Improvements to Clang's diagnostics
       scope.Unlock();
       require(scope); // Warning!  Requires mu1.
     }
+- Clang now disallows the use of asterisks preceding constructor and destructor names (#GH121706).
 
 Improvements to Clang's time-trace
 ----------------------------------
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index d4e897868f1a9a..ec0be4ea8b6ce0 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2202,6 +2202,8 @@ def err_invalid_qualified_constructor : Error<
   "'%0' qualifier is not allowed on a constructor">;
 def err_ref_qualifier_constructor : Error<
   "ref-qualifier '%select{&&|&}0' is not allowed on a constructor">;
+def err_invalid_constructor_decl : Error<
+  "invalid constructor declaration">;
 
 def err_constructor_return_type : Error<
   "constructor cannot have a return type">;
@@ -2223,6 +2225,8 @@ def err_destructor_not_member : Error<
 def err_destructor_cannot_be : Error<"destructor cannot be declared '%0'">;
 def err_invalid_qualified_destructor : Error<
   "'%0' qualifier is not allowed on a destructor">;
+def err_invalid_destructor_decl : Error<
+  "invalid destructor declaration">;
 def err_ref_qualifier_destructor : Error<
   "ref-qualifier '%select{&&|&}0' is not allowed on a destructor">;
 def err_destructor_return_type : Error<"destructor cannot have a return type">;
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index c4bee44f5ec048..bb6f6e14713d9b 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -10757,6 +10757,17 @@ static void checkMethodTypeQualifiers(Sema &S, Declarator &D, unsigned DiagID) {
   }
 }
 
+static void checkMethodPointerType(Sema &S, Declarator &D, unsigned DiagID) {
+  if (D.getNumTypeObjects() > 0) {
+    DeclaratorChunk &Chunk = D.getTypeObject(D.getNumTypeObjects() - 1);
+    if (Chunk.Kind == DeclaratorChunk::Pointer) {
+      SourceLocation PointerLoc = Chunk.getSourceRange().getBegin();
+      S.Diag(PointerLoc, DiagID) << Chunk.getSourceRange();
+      D.setInvalidType();
+    }
+  }
+}
+
 QualType Sema::CheckConstructorDeclarator(Declarator &D, QualType R,
                                           StorageClass &SC) {
   bool isVirtual = D.getDeclSpec().isVirtualSpecified();
@@ -10792,6 +10803,7 @@ QualType Sema::CheckConstructorDeclarator(Declarator &D, QualType R,
   }
 
   checkMethodTypeQualifiers(*this, D, diag::err_invalid_qualified_constructor);
+  checkMethodPointerType(*this, D, diag::err_invalid_constructor_decl);
 
   // C++0x [class.ctor]p4:
   //   A constructor shall not be declared with a ref-qualifier.
@@ -10958,6 +10970,7 @@ QualType Sema::CheckDestructorDeclarator(Declarator &D, QualType R,
   }
 
   checkMethodTypeQualifiers(*this, D, diag::err_invalid_qualified_destructor);
+  checkMethodPointerType(*this, D, diag::err_invalid_destructor_decl);
 
   // C++0x [class.dtor]p2:
   //   A destructor shall not be declared with a ref-qualifier.
diff --git a/clang/test/SemaCXX/constructor.cpp b/clang/test/SemaCXX/constructor.cpp
index abd7dbe18a0e6a..c6df7be25c1fac 100644
--- a/clang/test/SemaCXX/constructor.cpp
+++ b/clang/test/SemaCXX/constructor.cpp
@@ -96,3 +96,13 @@ namespace PR38286 {
   template<typename> struct C; // expected-note {{non-type declaration found}}
   template<typename T> C<T>::~C() {} // expected-error {{identifier 'C' after '~' in destructor name does not name a type}}
 }
+
+namespace GH121706 {
+struct S {
+  *S();  // expected-error {{invalid constructor declaration}}
+  **S(); // expected-error {{invalid constructor declaration}}
+
+  **S(const S &); // expected-error {{invalid constructor declaration}}
+  *S(S &&);       // expected-error {{invalid constructor declaration}}
+};
+}
diff --git a/clang/test/SemaCXX/destructor.cpp b/clang/test/SemaCXX/destructor.cpp
index dfcd1b033af5a2..f188e95c25d174 100644
--- a/clang/test/SemaCXX/destructor.cpp
+++ b/clang/test/SemaCXX/destructor.cpp
@@ -586,4 +586,11 @@ struct Y : X {} y1{ }; // expected-error {{call to implicitly-deleted default co
                        // expected-note {{default constructor of 'Y' is implicitly deleted because base class 'X' has no destructor}}
 }
 
+namespace GH121706 {
+struct S {
+  *~S();  // expected-error {{invalid destructor declaration}}
+  **~S(); // expected-error {{invalid destructor declaration}}
+};
+}
+
 #endif // BE_THE_HEADER

[cor3ntin]

Can we simplify that by checking that there is only one chunk? (which is a function?)

[a-tarasyuk]

In this case, the declarator consists of two chunks: [DeclaratorChunk::Function, DeclaratorChunk::Pointer]

[cor3ntin]

I know, my question is whether there is any case where having more than one chunk would be valid?

[a-tarasyuk]

Do you mean that having more than one chunk should, by default, be considered an error? I thought about that, however, I opted to use the appropriate kind check to ensure cases like this are handled correctly (the helper is used for constructors and destructors).

struct S {
  (~S)();
};

[cor3ntin]

You are right, parentheses are valid. Good catch. But everything else is not
https://eel.is/c++draft/class.ctor#general-1

So if any of the chuck is not either Function or Paren it should be an error (if pointer was the only case we missed, maybe an assert instead)

[cor3ntin]

More test cases https://compiler-explorer.com/z/1v5MvxKfh

[MagentaTreehouse]

See also #121706 (comment)

[shafik]

Thank you for the fix.

Please add a more detailed summary, the title feels sufficient to describe the problem but you should outline the solution in the summary, at least briefly. e.g. adding checkMethodPointerType which will called during .... to verify ...

[shafik]

As pointed out above, we need a lot more tests. The bug report shows several other cases this applies to and they should all be enumerated in the tests.