Suppress noreturn warning if last statement in a function is a throw

[snarang181]

Fixes #144952

[llvmbot]

@llvm/pr-subscribers-clang

Author: Samarth Narang (snarang181)

Changes

---

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

2 Files Affected:

• (modified) clang/lib/Sema/AnalysisBasedWarnings.cpp (+45)
• (added) clang/test/SemaCXX/wreturn-always-throws.cpp (+22)

diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index 2a107a36e24b4..85a5d99c710d5 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -626,6 +626,31 @@ struct CheckFallThroughDiagnostics {
 
 } // anonymous namespace
 
+static bool isKnownToAlwaysThrow(const FunctionDecl *FD) {
+  if (!FD->hasBody())
+    return false;
+  const Stmt *Body = FD->getBody();
+  const Stmt *OnlyStmt = nullptr;
+
+  if (const auto *Compound = dyn_cast<CompoundStmt>(Body)) {
+    if (Compound->size() != 1)
+      return false; // More than one statement, can't be known to always throw.
+    OnlyStmt = *Compound->body_begin();
+  } else {
+    OnlyStmt = Body;
+  }
+
+  // Unwrap ExprWithCleanups if necessary.
+  if (const auto *EWC = dyn_cast<ExprWithCleanups>(OnlyStmt)) {
+    OnlyStmt = EWC->getSubExpr();
+  }
+  // Check if the only statement is a throw expression.
+  if (isa<CXXThrowExpr>(OnlyStmt)) {
+    return true; // Known to always throw.
+  }
+  return false; // Not known to always throw.
+}
+
 /// CheckFallThroughForBody - Check that we don't fall off the end of a
 /// function that should return a value.  Check that we don't fall off the end
 /// of a noreturn function.  We assume that functions and blocks not marked
@@ -681,6 +706,26 @@ static void CheckFallThroughForBody(Sema &S, const Decl *D, const Stmt *Body,
       if (CD.diag_FallThrough_HasNoReturn)
         S.Diag(RBrace, CD.diag_FallThrough_HasNoReturn) << CD.FunKind;
     } else if (!ReturnsVoid && CD.diag_FallThrough_ReturnsNonVoid) {
+      // If the final statement is a call to an always-throwing function,
+      // don't warn about the fall-through.
+      if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
+        if (const auto *CS = dyn_cast<CompoundStmt>(Body)) {
+          if (!CS->body_empty()) {
+            const Stmt *LastStmt = CS->body_back();
+            // Unwrap ExprWithCleanups if necessary.
+            if (const auto *EWC = dyn_cast<ExprWithCleanups>(LastStmt)) {
+              LastStmt = EWC->getSubExpr();
+            }
+            if (const auto *CE = dyn_cast<CallExpr>(LastStmt)) {
+              if (const FunctionDecl *Callee = CE->getDirectCallee()) {
+                if (isKnownToAlwaysThrow(Callee)) {
+                  return; // Don't warn about fall-through.
+                }
+              }
+            }
+          }
+        }
+      }
       bool NotInAllControlPaths = FallThroughType == MaybeFallThrough;
       S.Diag(RBrace, CD.diag_FallThrough_ReturnsNonVoid)
           << CD.FunKind << NotInAllControlPaths;
diff --git a/clang/test/SemaCXX/wreturn-always-throws.cpp b/clang/test/SemaCXX/wreturn-always-throws.cpp
new file mode 100644
index 0000000000000..0bbce0f5c1871
--- /dev/null
+++ b/clang/test/SemaCXX/wreturn-always-throws.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -fsyntax-only -fcxx-exceptions -fexceptions -Wreturn-type -verify %s
+// expected-no-diagnostics
+
+namespace std {
+  class string {
+  public:
+    string(const char*); // constructor for runtime_error
+  };
+  class runtime_error {
+  public:
+    runtime_error(const string &); 
+  };
+}
+
+void throwError(const std::string& msg) {
+  throw std::runtime_error(msg);
+}
+
+int ensureZero(const int i) {
+  if (i == 0) return 0;
+  throwError("ERROR"); // no-warning
+}

[Copilot]

Pull Request Overview

This PR adds support to suppress “fall-through” return-type warnings when a function’s last statement always throws, along with a verifying test.

• Introduce isKnownToAlwaysThrow helper to detect single-statement throw functions
• Update CheckFallThroughForBody to skip warnings for calls to those functions
• Add a new test (wreturn-always-throws.cpp) to verify suppression

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
clang/test/SemaCXX/wreturn-always-throws.cpp	New test covering a non-void function that ends in a throw
clang/lib/Sema/AnalysisBasedWarnings.cpp	Helper and logic to suppress return-type warnings on throwers

[Copilot]

[nitpick] The unwrap of ExprWithCleanups is duplicated in two places. Consider extracting a helper function to reduce duplication and centralize cleanup unwrapping logic.

Suggested change

	if (const auto *EWC = dyn_cast<ExprWithCleanups>(LastStmt)) {
	LastStmt = EWC->getSubExpr();
	}
	LastStmt = unwrapExprWithCleanups(LastStmt);

[github-actions]

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

[zwuis]

Thank you for the patch! Please add a release note in clang/docs/ReleaseNotes.rst.

I'm not sure if this case should be handled:

if (condition) {
  throw 1;
} else {
  throw 2;
}

Maybe we need to open an issue for it.

[snarang181]

Thank you for the patch! Please add a release note in clang/docs/ReleaseNotes.rst.

@zwuis, added a description of the patch in the release notes and addressed your review comments.

[philnik777]

I feel like this is a solution that can grow out of hand very easily. IMO the better solution would be to have -Wmissing-noreturn place a [[noreturn]] into the AST to avoid incorrect follow-up diagnostics.

[TiborGY]

I feel like this is a solution that can grow out of hand very easily. IMO the better solution would be to have -Wmissing-noreturn place a [[noreturn]] into the AST to avoid incorrect follow-up diagnostics.

I agree, in principle I would expect Clang to be able to prove (in simple cases) that a function cannot return, and then treat it as-if they were marked as [[noreturn]] in the source code, not just for purposes of diagnostics but also optimization. Suppressing the -Wreturn-type warning does not achieve that.

[erichkeane]

This seems reasonable to me, though I'd like to give other reviewers a chance. I think we need to be less optimistic in the release notes (clarifying that we are doing a very mild pattern match), else I think this is sensible.

[snarang181]

@erichkeane, thank you for your review. I have adjusted the diff to reflect your suggestions.

[erichkeane]

I feel like this is a solution that can grow out of hand very easily. IMO the better solution would be to have -Wmissing-noreturn place a [[noreturn]] into the AST to avoid incorrect follow-up diagnostics.

I agree, in principle I would expect Clang to be able to prove (in simple cases) that a function cannot return, and then treat it as-if they were marked as [[noreturn]] in the source code, not just for purposes of diagnostics but also optimization. Suppressing the -Wreturn-type warning does not achieve that.

Actually, I just saw this and think it is a reasonably good idea. Rather than trying to check the same functions over and over, we can 'cache' the answers via an implicit function attribute (only when one of the relevant diagnostics is enabled). I'm not sure I would spell it [[noreturn]] even when implicit (as that could result in cascading diagnostics, and I'm not sure I want the optimization outcomes), but it would be generally more accurate.

Sorry for not seeing this sooner.

[snarang181]

I feel like this is a solution that can grow out of hand very easily. IMO the better solution would be to have -Wmissing-noreturn place a [[noreturn]] into the AST to avoid incorrect follow-up diagnostics.

I agree, in principle I would expect Clang to be able to prove (in simple cases) that a function cannot return, and then treat it as-if they were marked as [[noreturn]] in the source code, not just for purposes of diagnostics but also optimization. Suppressing the -Wreturn-type warning does not achieve that.

Actually, I just saw this and think it is a reasonably good idea. Rather than trying to check the same functions over and over, we can 'cache' the answers via an implicit function attribute (only when one of the relevant diagnostics is enabled). I'm not sure I would spell it [[noreturn]] even when implicit (as that could result in cascading diagnostics, and I'm not sure I want the optimization outcomes), but it would be generally more accurate.

Sorry for not seeing this sooner.

No issues. I will work on it further, thanks.

[erichkeane]

Want 1 more test and a preference on interface, else I'm pretty good here.