[clang] Fix covariant cv-qualification check to require the override function return type to have the same or less cv-qualification

[bricknerb]

This prevents changing cv-qualification from const to volatile or vice versa, for example.

https://eel.is/c++draft/class.virtual#8.3

Previously, we checked that the new type is the same or more qualified to return an error, but the standard requires the new type to be the same or less qualified and since the cv-qualification is only partially ordered, we cannot rely on a check on whether it is more qualified to return an error. Now, we reversed the condition to check whether the old is at least as qualified, and return an error if it is not.

Also, adjusted the error name and message to clarify the requirement and added a missing closing parenthesis.

Added tests to cover different use cases for classes with different qualifications and also refactored them to make them easier to follow:

1. Use override to make sure the function names actually match.
2. Named the function in a more descriptive way to clarify what each use case is checking.

Fixes: #111742

[llvmbot]

@llvm/pr-subscribers-clang

Author: Boaz Brickner (bricknerb)

Changes

This prevents changing cv-qualification from const to volatile or vice versa, for example.

https://eel.is/c++draft/class.virtual#8.3

Previously, we checked that the new type is the same or more qualified to return an error, but the standard requires the new type to be the same or less qualified and since the cv-qualification is only partially ordered, we cannot rely on a check on whether it is more qualified to return an error. Now, we reversed the condition to check whether the old is at least as qualified, and return an error if it is not.

Also, adjusted the error name and message to clarify the requirement and added a missing closing parenthesis.

Added tests to cover different use cases for classes with different qualifications and also refactored them to make them easier to follow:

1. Use override to make sure the function names actually match.
2. Named the function in a more descriptive way to clarify what each use case is checking.

Fixes: #111742

---

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

4 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+8-1)
• (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+3-3)
• (modified) clang/lib/Sema/SemaDeclCXX.cpp (+2-2)
• (modified) clang/test/SemaCXX/virtual-override.cpp (+44-8)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index dc5564b6db119f..1d630335f8d213 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -99,17 +99,24 @@ C++ Specific Potentially Breaking Changes
     // Was error, now evaluates to false.
     constexpr bool b = f() == g();
 
-- Clang will now correctly not consider pointers to non classes for covariance.
+- Clang will now correctly not consider pointers to non classes for covariance
+  and disallow changing return type to a type that doesn't have the same or less cv-qualifications.
 
   .. code-block:: c++
 
     struct A {
       virtual const int *f() const;
+      virtual const std::string *g() const;
     };
     struct B : A {
       // Return type has less cv-qualification but doesn't point to a class.
       // Error will be generated.
       int *f() const override;
+
+      // Return type doesn't have more cv-qualification also not the same or
+      // less cv-qualification.
+      // Error will be generated.
+      volatile std::string *g() const override;
     };
 
 - The warning ``-Wdeprecated-literal-operator`` is now on by default, as this is
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index c458a62d9be48c..487dd8990d88e9 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2182,10 +2182,10 @@ def err_covariant_return_incomplete : Error<
 def err_covariant_return_type_different_qualifications : Error<
   "return type of virtual function %0 is not covariant with the return type of "
   "the function it overrides (%1 has different qualifiers than %2)">;
-def err_covariant_return_type_class_type_more_qualified : Error<
+def err_covariant_return_type_class_type_not_same_or_less_qualified : Error<
   "return type of virtual function %0 is not covariant with the return type of "
-  "the function it overrides (class type %1 is more qualified than class "
-  "type %2">;
+  "the function it overrides (class type %1 does not have the same "
+  "cv-qualification as or less cv-qualification than class type %2)">;
 
 // C++ implicit special member functions
 def note_in_declaration_of_implicit_special_member : Note<
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 38f808a470aa87..43ec25b23d972a 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -18338,9 +18338,9 @@ bool Sema::CheckOverridingFunctionReturnType(const CXXMethodDecl *New,
 
 
   // The new class type must have the same or less qualifiers as the old type.
-  if (NewClassTy.isMoreQualifiedThan(OldClassTy)) {
+  if (!OldClassTy.isAtLeastAsQualifiedAs(NewClassTy)) {
     Diag(New->getLocation(),
-         diag::err_covariant_return_type_class_type_more_qualified)
+         diag::err_covariant_return_type_class_type_not_same_or_less_qualified)
         << New->getDeclName() << NewTy << OldTy
         << New->getReturnTypeSourceRange();
     Diag(Old->getLocation(), diag::note_overridden_virtual_function)
diff --git a/clang/test/SemaCXX/virtual-override.cpp b/clang/test/SemaCXX/virtual-override.cpp
index d37c275d46baeb..ce6dd35e0b56fa 100644
--- a/clang/test/SemaCXX/virtual-override.cpp
+++ b/clang/test/SemaCXX/virtual-override.cpp
@@ -83,17 +83,53 @@ namespace T6 {
 struct a { };
 
 class A {
-  virtual const a* f(); 
-  virtual a* g(); // expected-note{{overridden virtual function is here}}
-  virtual const int* h(); // expected-note{{overridden virtual function is here}}
-  virtual int* i(); // expected-note{{overridden virtual function is here}}
+  // Classes.
+  virtual const a* const_vs_unqualified_class();
+  virtual a* unqualified_vs_const_class(); // expected-note{{overridden virtual function is here}}
+
+  virtual volatile a* volatile_vs_unqualified_class();
+  virtual a* unqualified_vs_volatile_class(); // expected-note{{overridden virtual function is here}}
+
+  virtual const a* const_vs_volatile_class(); // expected-note{{overridden virtual function is here}}
+  virtual volatile a* volatile_vs_const_class(); // expected-note{{overridden virtual function is here}}
+
+  virtual const volatile a* const_volatile_vs_const_class();
+  virtual const a* const_vs_const_volatile_class(); // expected-note{{overridden virtual function is here}}
+
+  virtual const volatile a* const_volatile_vs_volatile_class();
+  virtual volatile a* volatile_vs_const_volatile_class(); // expected-note{{overridden virtual function is here}}
+
+  virtual const volatile a* const_volatile_vs_unualified_class();
+  virtual a* unqualified_vs_const_volatile_class(); // expected-note{{overridden virtual function is here}}
+
+  // Non Classes.
+  virtual const int* const_vs_unqualified_non_class(); // expected-note{{overridden virtual function is here}}
+  virtual int* unqualified_vs_const_non_class(); // expected-note{{overridden virtual function is here}}
 };
 
 class B : A {
-  virtual a* f(); 
-  virtual const a* g(); // expected-error{{return type of virtual function 'g' is not covariant with the return type of the function it overrides (class type 'const a *' is more qualified than class type 'a *'}}
-  virtual int* h();  // expected-error{{virtual function 'h' has a different return type ('int *') than the function it overrides (which has return type 'const int *')}}
-  virtual const int* i(); // expected-error{{virtual function 'i' has a different return type ('const int *') than the function it overrides (which has return type 'int *')}}
+  // Classes.
+  a* const_vs_unqualified_class() override;
+  const a* unqualified_vs_const_class() override; // expected-error{{return type of virtual function 'unqualified_vs_const_class' is not covariant with the return type of the function it overrides (class type 'const a *' does not have the same cv-qualification as or less cv-qualification than class type 'a *')}}
+
+  a* volatile_vs_unqualified_class() override;
+  volatile a* unqualified_vs_volatile_class() override; // expected-error{{return type of virtual function 'unqualified_vs_volatile_class' is not covariant with the return type of the function it overrides (class type 'volatile a *' does not have the same cv-qualification as or less cv-qualification than class type 'a *')}}
+
+  volatile a* const_vs_volatile_class() override; // expected-error{{return type of virtual function 'const_vs_volatile_class' is not covariant with the return type of the function it overrides (class type 'volatile a *' does not have the same cv-qualification as or less cv-qualification than class type 'const a *')}}
+  const a* volatile_vs_const_class() override; // expected-error{{return type of virtual function 'volatile_vs_const_class' is not covariant with the return type of the function it overrides (class type 'const a *' does not have the same cv-qualification as or less cv-qualification than class type 'volatile a *')}}
+
+  const a* const_volatile_vs_const_class() override;
+  const volatile a* const_vs_const_volatile_class() override; // expected-error{{return type of virtual function 'const_vs_const_volatile_class' is not covariant with the return type of the function it overrides (class type 'const volatile a *' does not have the same cv-qualification as or less cv-qualification than class type 'const a *')}}
+
+  volatile a* const_volatile_vs_volatile_class() override;
+  const volatile a* volatile_vs_const_volatile_class() override; // expected-error{{return type of virtual function 'volatile_vs_const_volatile_class' is not covariant with the return type of the function it overrides (class type 'const volatile a *' does not have the same cv-qualification as or less cv-qualification than class type 'volatile a *')}}
+
+  a* const_volatile_vs_unualified_class() override;
+  const volatile a* unqualified_vs_const_volatile_class() override; // expected-error{{return type of virtual function 'unqualified_vs_const_volatile_class' is not covariant with the return type of the function it overrides (class type 'const volatile a *' does not have the same cv-qualification as or less cv-qualification than class type 'a *')}}
+
+  // Non Classes.
+  int* const_vs_unqualified_non_class() override; // expected-error{{virtual function 'const_vs_unqualified_non_class' has a different return type ('int *') than the function it overrides (which has return type 'const int *')}}
+  const int* unqualified_vs_const_non_class() override; // expected-error{{virtual function 'unqualified_vs_const_non_class' has a different return type ('const int *') than the function it overrides (which has return type 'int *')}}
 };
 
 }

[cor3ntin]

LGTM (I'm surprised we never caught that before)

[cor3ntin]

Would you be willing to write additional tests for https://cplusplus.github.io/CWG/issues/960.html in a separate PR?

[bricknerb]

Would you be willing to write additional tests for https://cplusplus.github.io/CWG/issues/960.html in a separate PR?

Sure, I'll take a look.

[bricknerb]

Would you be willing to write additional tests for https://cplusplus.github.io/CWG/issues/960.html in a separate PR?

Sure, I'll take a look.

See #112853.