[Clang] FunctionEffects: Make a separate diagnostic group for redeclarations/overrides where effects are implicit.

[dougsonos]

The current function effect diagnostics include these behaviors:

When you declare a function nonblocking (typically in a header) and then omit the attribute on the implementation (or any other redeclaration), Clang warns: attribute 'nonblocking' on function does not match previous declaration.

But if a nonblocking function is a C++ virtual method, then overrides are implicitly nonblocking; the attribute doesn't need to be explicitly stated.

These behaviors are arguably inconsistent -- and also, both, more pedantic than the rest of the function effect diagnostics.

This PR accomplishes two things:

• Separates the diagnostic on a redeclaration into a new group, -Wfunction-effect-redeclarations, so it can be disabled independently.

• Adds a second diagnostic to this new group, for the case of an override method missing the attribute. (This helps in a situation where I'm trying to add nonblocking via a macro that does other things and I want to know that the macro is missing on an override declaration.)

[llvmbot]

@llvm/pr-subscribers-clang

Author: Doug Wyatt (dougsonos)

Changes

The current function effect diagnostics include these behaviors:

When you declare a function nonblocking (typically in a header) and then omit the attribute on the implementation (or any other redeclaration), Clang warns: attribute 'nonblocking' on function does not match previous declaration.

But if a nonblocking function is a C++ virtual method, then overrides are implicitly nonblocking; the attribute doesn't need to be explicitly stated.

These behaviors are arguably inconsistent -- and also, both, more pedantic than the rest of the function effect diagnostics.

This PR accomplishes two things:

• Separates the diagnostic on a redeclaration into a new group, -Wfunction-effect-redeclarations, so it can be disabled independently.

• Adds a second diagnostic to this new group, for the case of an override method missing the attribute. (This helps in a situation where I'm trying to add nonblocking via a macro that does other things and I want to know that the macro is missing on an override declaration.)

---

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

4 Files Affected:

• (modified) clang/include/clang/Basic/DiagnosticGroups.td (+1)
• (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+10-5)
• (modified) clang/lib/Sema/SemaDeclCXX.cpp (+9-1)
• (modified) clang/test/Sema/attr-nonblocking-sema.cpp (+7-7)

diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 9a7a308600763..b59439d26a71a 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1291,6 +1291,7 @@ def ThreadSafetyBeta : DiagGroup<"thread-safety-beta">;
 // Warnings and notes related to the function effects system which underlies
 // the nonblocking and nonallocating attributes.
 def FunctionEffects : DiagGroup<"function-effects">;
+def FunctionEffectRedeclarations : DiagGroup<"function-effect-redeclarations">;
 def PerfConstraintImpliesNoexcept : DiagGroup<"perf-constraint-implies-noexcept">;
 
 // Uniqueness Analysis warnings
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 577adc30ba2fa..2b1fa4330d325 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -11529,17 +11529,22 @@ def note_in_evaluating_default_argument : Note<
 def warn_invalid_add_func_effects : Warning<
   "attribute '%0' should not be added via type conversion">,
   InGroup<FunctionEffects>, DefaultIgnore;
-def warn_mismatched_func_effect_override : Warning<
-  "attribute '%0' on overriding function does not match base declaration">,
-  InGroup<FunctionEffects>, DefaultIgnore;
-def warn_mismatched_func_effect_redeclaration : Warning<
-  "attribute '%0' on function does not match previous declaration">,
+def warn_conflicting_func_effect_override : Warning<
+  "attribute '%0' on overriding function conflicts with base declaration">,
   InGroup<FunctionEffects>, DefaultIgnore;
 def warn_conflicting_func_effects : Warning<
   "effects conflict when merging declarations; kept '%0', discarded '%1'">,
   InGroup<FunctionEffects>, DefaultIgnore;
 def err_func_with_effects_no_prototype : Error<
   "'%0' function must have a prototype">;
+// These are more pedantic: in redeclarations and virtual method overrides,
+// the effect attribute(s) should be restated.
+def warn_mismatched_func_effect_override : Warning<
+  "overriding function is missing '%0' attribute from base declaration">,
+  InGroup<FunctionEffectRedeclarations>, DefaultIgnore;
+def warn_mismatched_func_effect_redeclaration : Warning<
+  "redeclaration is missing '%0' attribute from previous declaration">,
+  InGroup<FunctionEffectRedeclarations>, DefaultIgnore;
 
 } // end of sema category
 
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 5cc92ebb0171f..090a1d9d290bb 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -18686,7 +18686,7 @@ bool Sema::CheckOverridingFunctionAttributes(CXXMethodDecl *New,
         case FunctionEffectDiff::OverrideResult::NoAction:
           break;
         case FunctionEffectDiff::OverrideResult::Warn:
-          Diag(New->getLocation(), diag::warn_mismatched_func_effect_override)
+          Diag(New->getLocation(), diag::warn_conflicting_func_effect_override)
               << Diff.effectName();
           Diag(Old->getLocation(), diag::note_overridden_virtual_function)
               << Old->getReturnTypeSourceRange();
@@ -18699,6 +18699,14 @@ bool Sema::CheckOverridingFunctionAttributes(CXXMethodDecl *New,
           QualType ModQT = Context.getFunctionType(NewFT->getReturnType(),
                                                    NewFT->getParamTypes(), EPI);
           New->setType(ModQT);
+          if (Errs.empty()) {
+            // A warning here is somewhat pedantic, in a different warning group.
+            // Skip this if there was already a merge conflict, which is more serious.
+            Diag(New->getLocation(), diag::warn_mismatched_func_effect_override)
+                << Diff.effectName();
+            Diag(Old->getLocation(), diag::note_overridden_virtual_function)
+                << Old->getReturnTypeSourceRange();
+          }
           break;
         }
         }
diff --git a/clang/test/Sema/attr-nonblocking-sema.cpp b/clang/test/Sema/attr-nonblocking-sema.cpp
index f13cc783dfc33..b8a9071aae50c 100644
--- a/clang/test/Sema/attr-nonblocking-sema.cpp
+++ b/clang/test/Sema/attr-nonblocking-sema.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -fblocks -fcxx-exceptions -verify -Wfunction-effects %s
-// RUN: %clang_cc1 -fsyntax-only -fblocks -verify -x c -std=c23 -Wfunction-effects %s
+// RUN: %clang_cc1 -fsyntax-only -fblocks -fcxx-exceptions -verify -Wfunction-effects -Wfunction-effect-redeclarations %s
+// RUN: %clang_cc1 -fsyntax-only -fblocks -verify -x c -std=c23 -Wfunction-effects -Wfunction-effect-redeclarations %s
 
 #if !__has_attribute(nonblocking)
 #error "the 'nonblocking' attribute is not available"
@@ -132,15 +132,15 @@ void type_conversions_2()
 #ifdef __cplusplus
 struct Base {
   virtual void f1();
-  virtual void nonblocking() noexcept [[clang::nonblocking]];
-  virtual void nonallocating() noexcept [[clang::nonallocating]];
+  virtual void nonblocking() noexcept [[clang::nonblocking]]; // expected-note {{overridden virtual function is here}}
+  virtual void nonallocating() noexcept [[clang::nonallocating]]; // expected-note {{overridden virtual function is here}}
   virtual void f2() [[clang::nonallocating]]; // expected-note {{previous declaration is here}}
 };
 
 struct Derived : public Base {
   void f1() [[clang::nonblocking]] override;
-  void nonblocking() noexcept override;
-  void nonallocating() noexcept override;
+  void nonblocking() noexcept override; // expected-warning {{overriding function is missing 'nonblocking' attribute from base declaration}}
+  void nonallocating() noexcept override; // expected-warning {{overriding function is missing 'nonallocating' attribute from base declaration}}
   void f2() [[clang::allocating]] override; // expected-warning {{effects conflict when merging declarations; kept 'allocating', discarded 'nonallocating'}}
 };
 #endif // __cplusplus
@@ -149,7 +149,7 @@ struct Derived : public Base {
 
 void f2();
 void f2() [[clang::nonblocking]]; // expected-note {{previous declaration is here}}
-void f2(); // expected-warning {{attribute 'nonblocking' on function does not match previous declaration}}
+void f2(); // expected-warning {{redeclaration is missing 'nonblocking' attribute from previous declaration}}
 // Note: we verify that the attribute is actually seen during the constraints tests.
 
 void f3() [[clang::blocking]]; // expected-note {{previous declaration is here}}

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[Sirraide]

I think that makes sense; make sure to add a release note for it though.