[Clang] [NFC] Fix more `-Wreturn-type` warnings in tests everywhere #123470

Sirraide · 2025-01-18T18:38:32Z

~~This makes -Wreturn-type default to an error in all language modes as there is basically never a reason as to why one would want to fall off the end of a non-void function.~~

~~Most of the tedious work of migrating the tests was done in #123464 (except for one test that for some reason didn’t want to cooperate earlier, which is why that one is handled in this pr).~~

It turns out that most of the tedious work is not done yet, so this fixes even more tests, mainly clang-tidy and clangd tests.

llvmbot · 2025-01-18T18:39:08Z

@llvm/pr-subscribers-clang-tools-extra
@llvm/pr-subscribers-clangd
@llvm/pr-subscribers-compiler-rt-sanitizer

@llvm/pr-subscribers-clang

Author: None (Sirraide)

Changes

This makes -Wreturn-type default to an error in all language modes as there is basically never a reason as to why one would want to fall off the end of a non-void function.

Most of the tedious work of migrating the tests was done in #123464 (except for one test that for some reason didn’t want to cooperate earlier, which is why that one is handled in this pr).

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

3 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+4)
(modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+6-6)
(modified) clang/test/Index/warning-flags.c (+1-1)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index aa1c02d04f7caa..a086c443984408 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -81,6 +81,10 @@ code bases.
   ``-fno-strict-overflow`` to opt-in to a language dialect where signed integer
   and pointer overflow are well-defined.
 
+- ``-Wreturn-type`` now defaults to an error in all language modes; like other
+  warnings, it can be reverted to just being a warning by specifying
+  ``-Wno-error=return-type``.
+
 C/C++ Language Potentially Breaking Changes
 -------------------------------------------
 
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index db54312ad965e8..b70d152781a2a8 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -709,10 +709,10 @@ def err_thread_unsupported : Error<
 // FIXME: Combine fallout warnings to just one warning.
 def warn_maybe_falloff_nonvoid_function : Warning<
   "non-void function does not return a value in all control paths">,
-  InGroup<ReturnType>;
+  InGroup<ReturnType>, DefaultError;
 def warn_falloff_nonvoid_function : Warning<
   "non-void function does not return a value">,
-  InGroup<ReturnType>;
+  InGroup<ReturnType>, DefaultError;
 def warn_const_attr_with_pure_attr : Warning<
   "'const' attribute imposes more restrictions; 'pure' attribute ignored">,
   InGroup<IgnoredAttributes>;
@@ -726,10 +726,10 @@ def err_falloff_nonvoid_block : Error<
   "non-void block does not return a value">;
 def warn_maybe_falloff_nonvoid_coroutine : Warning<
   "non-void coroutine does not return a value in all control paths">,
-  InGroup<ReturnType>;
+  InGroup<ReturnType>, DefaultError;
 def warn_falloff_nonvoid_coroutine : Warning<
   "non-void coroutine does not return a value">,
-  InGroup<ReturnType>;
+  InGroup<ReturnType>, DefaultError;
 def warn_suggest_noreturn_function : Warning<
   "%select{function|method}0 %1 could be declared with attribute 'noreturn'">,
   InGroup<MissingNoreturn>, DefaultIgnore;
@@ -8365,10 +8365,10 @@ let CategoryName = "Lambda Issue" in {
     "lambda declared 'noreturn' should not return">;
   def warn_maybe_falloff_nonvoid_lambda : Warning<
     "non-void lambda does not return a value in all control paths">,
-    InGroup<ReturnType>;
+    InGroup<ReturnType>, DefaultError;
   def warn_falloff_nonvoid_lambda : Warning<
     "non-void lambda does not return a value">,
-    InGroup<ReturnType>;
+    InGroup<ReturnType>, DefaultError;
   def err_access_lambda_capture : Error<
     // The ERRORs represent other special members that aren't constructors, in
     // hopes that someone will bother noticing and reporting if they appear
diff --git a/clang/test/Index/warning-flags.c b/clang/test/Index/warning-flags.c
index 1694c6abab5624..3229f000c4ae0c 100644
--- a/clang/test/Index/warning-flags.c
+++ b/clang/test/Index/warning-flags.c
@@ -9,7 +9,7 @@ int *bar(float *f) { return f; }
 // RUN: c-index-test -test-load-source-reparse 5 all -w %s 2>&1 | FileCheck -check-prefix=NOWARNINGS %s
 // RUN: c-index-test -test-load-source all -w -O4 %s 2>&1 | FileCheck -check-prefix=NOWARNINGS %s
 
-// CHECK-BOTH-WARNINGS: warning: non-void function does not return a value
+// CHECK-BOTH-WARNINGS: error: non-void function does not return a value
 // CHECK-BOTH-WARNINGS: warning: incompatible pointer types returning 'float *' from a function with result type 'int *'
 
 // CHECK-SECOND-WARNING-NOT:non-void function does not return a value

Sirraide · 2025-01-18T23:17:43Z

Looks like I might have to fix some compiler-rt tests first; that might well end up being another separate pr.

Sirraide · 2025-01-18T23:42:42Z

Ok, it ended up being only 3 compiler-rt tests that needed fixing.

cor3ntin · 2025-01-19T08:27:18Z

Thanks for working on that
Did you do a stage 2 build?
I think this is probably Clang 21 material (unless we become sufficiently confident before RC3)

jeremy-rifkin · 2025-01-19T15:27:39Z

There are implications with #123166, which might also be clang 21 material

Sirraide · 2025-01-19T20:18:26Z

Did you do a stage 2 build?

Not yet; that’s probably a good idea.

shafik

I see a lot of diagnostics were updated to DefaultError but I don't see matching tests that demonstrate the now error. We should be testing each diagnostic.

Sirraide · 2025-01-23T09:43:25Z

I see a lot of diagnostics were updated to DefaultError but I don't see matching tests that demonstrate the now error. We should be testing each diagnostic.

#123464 also added -Werror=return-type to some tests; we can remove those now, which should cover at least some of the diagonstics; I’ll add a test for whatever is left after that because I don’t think we have one for e.g. the coroutine ones.

Sirraide · 2025-01-23T09:44:29Z

I think this is probably Clang 21 material

Yeah, probably

AaronBallman · 2025-01-23T19:06:59Z

I think this is probably Clang 21 material

Yeah, probably

First off, I really appreciate this patch because (to me) it's not really defensible in 2025 that falling off the end of a function with a non-void return type is anything but an error. So thank you!

I do think this is Clang 21 material though because I suspect the fallout from this might be a challenge. Specifically, I wonder about autoconf; that tool often plays fast-and-loose with the generated code and it relies on the presence of diagnostics to say "nope, this is not supported". So I worry there will be autoconf scripts out there which do int test() { malloc(1); } to test for whether malloc exists and it now fails because it falls off the end of a non-void function and so it gives the wrong answer. However, I don't have evidence of this myself. Is that something you've looked into by any chance? If not, it would be worthwhile to know what the fallout is. CC @thesamesam who maybe has insights into what kind of fallout this will cause (or may be willing to do a test-run with the patch?)

Sirraide · 2025-01-23T19:59:08Z

(to me) it's not really defensible in 2025 that falling off the end of a function with a non-void return type is anything but an error.

Yeah, agreed.

Specifically, I wonder about autoconf

I hadn’t thought about that, that’s a good point.

Is that something you've looked into by any chance?

I’m not really familiar w/ autoconf, so no, I haven’t.

shafik · 2025-01-23T21:40:43Z

I see a lot of diagnostics were updated to DefaultError but I don't see matching tests that demonstrate the now error. We should be testing each diagnostic.

#123464 also added -Werror=return-type to some tests; we can remove those now, which should cover at least some of the diagonstics; I’ll add a test for whatever is left after that because I don’t think we have one for e.g. the coroutine ones.

I missed that but it was a large change.

In the future it would be helpful to mention that in the summary since we do expect all diagnostics we modify/add to have full coverage. Otherwise we leave ourselves open to future regressions.

thesamesam · 2025-01-27T00:23:30Z

Is that something you've looked into by any chance? If not, it would be worthwhile to know what the fallout is.

I'm worried about the fallout and I have vague recollections of some early work I did when I assumed we were making this change for the C99 enforcement work, but I don't have specifics. I can look into this but I'd need time -- I can't do that ready for Clang 20.

thesamesam · 2025-01-27T00:27:49Z

@AaronBallman On another note, we've had people request this many times in GCC for C++ at least, but it has FPs which I thought were unavoidable. I was under the impression Clang generally didn't do "middle-end"-style (or optimisation-dependent) warnings. Is it implemented differently in Clang, am I misremembering or misunderstanding something, or is Clang just accepting the FPs here?

(cc @pinskia)

Sirraide · 2025-01-27T14:03:38Z

Is it implemented differently in Clang,

The way -Wreturn-type is implemented in Clang is we build a CFG from the AST and then walk that to determine whether all paths return a value, if that’s what you’re asking.

Sirraide · 2025-01-27T14:05:57Z

I'm worried about the fallout and I have vague recollections of some early work I did when I assumed we were making this change for the C99 enforcement work

Another option would be to make this default to an error depending on the language mode (e.g. only in C23 or later, and either unconditionally in C++ or in e.g. C++17 or later etc.); I don’t have any actual data on this, but I would imagine that most programs that actually have code that triggers this warning are probably C99 or earlier.

thesamesam · 2025-01-27T14:13:50Z

The way -Wreturn-type is implemented in Clang is we build a CFG from the AST and then walk that to determine whether all paths return a value, if that’s what you’re asking.

Is -Wreturn-type more susceptible to FPs than other typical Clang warnings, and hence maybe shouldn't be an error? e.g. cases where some assert is made in the only caller so we know it can't happen, or it's a fixed value and that is wrongly not propagated. This kind of thing has plagued GCC and is unavoidable AFAICT. I'll look for bugs.

Sirraide · 2025-01-27T14:18:27Z

What I'm asking is: is -Wreturn-type more susceptible to FPs than other typical Clang warnings

There definitely are cases where we currently can’t figure out that a function always returns if the control flow is very convoluted, e.g. #111509 has this horrible example

int foo(int x) {
    bool matched = false;
    do {
      if (matched) {
        label: return 0;
      }

      if (matched || x == 1) {
        matched = true;
        return 123;
      }
      if (matched || x == 2) {
        matched = true;
        return 456;
      }

      if (matched) {
        break;
      }
      goto label;
    } while(false);
}

thesamesam · 2025-01-27T14:33:26Z

Thanks. I expect there'll be some fallout but you also get missed optimisation bugs out of that in some cases, so.

Another option would be to make this default to an error depending on the language mode (e.g. only in C23 or later, and either unconditionally in C++ or in e.g. C++17 or later etc.); I don’t have any actual data on this, but I would imagine that most programs that actually have code that triggers this warning are probably C99 or earlier.

This unfortunately won't work when we change the default to C23 and so on, but it could help a little bit for those who explicitly build with -std=c89 etc. Not sure if I'd advocate for such a thing but it's an option to keep in mind.

Sirraide · 2025-02-19T17:12:34Z

Existing code already uses -Wreturn-type though, and this patch is enabling it in all language modes, not just C23 and above.

At least that’s what I hope we’ll be able to do. A compromise would be to only enable it in e.g. C23/C++26/whatever, but that doesn’t quite seem satisfactory to me...

AaronBallman · 2025-02-19T17:17:48Z

Existing code already uses -Wreturn-type though, and this patch is enabling it in all language modes, not just C23 and above.

At least that’s what I hope we’ll be able to do. A compromise would be to only enable it in e.g. C23/C++26/whatever, but that doesn’t quite seem satisfactory to me...

Yeah, the goal is to improve safety and security, and so I think it makes sense to aim for enabling this in all language modes. Even if we enable it only in C23 or later as a fallback, it still means projects will struggle to upgrade to the latest C standard, so it's better for us to try to address as many pain points as we can so that migration path is easier for users.

github-actions · 2025-02-19T21:01:35Z

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

Sirraide · 2025-02-19T21:13:39Z

Ok, this is now another NFC pr that just fixes a bunch of tests.

AaronBallman

LGTM; I'm sure plenty of these could have changed the return type to void, but I think adding a return statement is fine as well (it means we don't have to question whether the return type mattered elsewhere in the tests).

Sirraide · 2025-02-20T18:46:36Z

(it means we don't have to question whether the return type mattered elsewhere in the tests).

Yeah, that’s the main reason I opted for adding a return statement in a lot of cases.

llvm-ci · 2025-02-20T19:00:30Z

LLVM Buildbot has detected a new failure on builder llvm-clang-x86_64-sie-ubuntu-fast running on sie-linux-worker while building clang-tools-extra,clang,compiler-rt at step 6 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/18525

Here is the relevant piece of the build log for the reference

Step 6 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'Clang Tools :: clang-tidy/checkers/readability/named-parameter.cpp' FAILED ********************
Exit Code: 1

Command Output (stdout):
--
Running ['clang-tidy', '/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/named-parameter.cpp.tmp.cpp', '-fix', '--checks=-*,readability-named-parameter', '--config={}', '--', '-std=c++11', '-nostdinc++']...
clang-tidy /home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/named-parameter.cpp.tmp.cpp -fix --checks=-*,readability-named-parameter --config={} -- -std=c++11 -nostdinc++ failed:
18 warnings and 2 errors generated.
Error while processing /home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/named-parameter.cpp.tmp.cpp.
/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/named-parameter.cpp.tmp.cpp:3:19: warning: all parameters should be named in a function [readability-named-parameter]
    3 | void Method(char *) { /* */ }
      |                   ^
      |                    /*unused*/
/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/named-parameter.cpp.tmp.cpp:6:20: warning: all parameters should be named in a function [readability-named-parameter]
    6 | void Method2(char *) {}
      |                    ^
      |                     /*unused*/
/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/named-parameter.cpp.tmp.cpp:9:20: warning: all parameters should be named in a function [readability-named-parameter]
    9 | void Method3(char *, void *) {}
      |                    ^       
      |                     /*unused*/  /*unused*/
/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/named-parameter.cpp.tmp.cpp:12:20: warning: all parameters should be named in a function [readability-named-parameter]
   12 | void Method4(char *, int /*unused*/) {}
      |                    ^
      |                     /*unused*/
/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/named-parameter.cpp.tmp.cpp:15:30: warning: all parameters should be named in a function [readability-named-parameter]
   15 | void operator delete[](void *) throw() {}
      |                              ^
      |                               /*unused*/
/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/named-parameter.cpp.tmp.cpp:18:16: warning: all parameters should be named in a function [readability-named-parameter]
   18 | int Method5(int) { return 0; }
      |                ^
      |                 /*unused*/
/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/named-parameter.cpp.tmp.cpp:21:21: warning: all parameters should be named in a function [readability-named-parameter]
   21 | void Method6(void (*)(void *)) {}
      |                     ^
      |                      /*unused*/
/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/named-parameter.cpp.tmp.cpp:24:37: warning: all parameters should be named in a function [readability-named-parameter]
   24 | template <typename T> void Method7(T) {}
      |                                     ^
      |                                      /*unused*/
/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/named-parameter.cpp.tmp.cpp:40:23: error: cannot use 'throw' with exceptions disabled [clang-diagnostic-error]
   40 |   X operator++(int) { throw 0; }
      |                       ^
/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/named-parameter.cpp.tmp.cpp:41:23: error: cannot use 'throw' with exceptions disabled [clang-diagnostic-error]
   41 |   X operator--(int) { throw 0; }
      |                       ^
/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/tools/clang/tools/extra/test/clang-tidy/checkers/readability/Output/named-parameter.cpp.tmp.cpp:55:13: warning: all parameters should be named in a function [readability-named-parameter]
   55 |   void foo(T) {}
      |             ^
...

Sirraide · 2025-02-20T19:02:01Z

That seems related; I’ll investigate this in a bit.

#123470 broke one of the clang-tidy tests; this fixes that.

Sirraide · 2025-02-20T19:07:08Z

Should be fixed by #128051.

[Clang] Make -Wreturn-type default to an error in all language modes

441385e

Sirraide added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Jan 18, 2025

Sirraide requested review from cor3ntin and AaronBallman January 18, 2025 18:38

llvmbot added the clang Clang issues not falling into any other category label Jan 18, 2025

Fix two tests

5200de4

Fix three compiler-rt tests

e8fcfc0

llvmbot added compiler-rt compiler-rt:ubsan Undefined behavior sanitizer compiler-rt:hwasan Hardware-assisted address sanitizer compiler-rt:sanitizer labels Jan 18, 2025

shafik reviewed Jan 23, 2025

View reviewed changes

Sirraide added 4 commits February 19, 2025 19:09

Merge branch 'main' into wreturn-type-error

c5dd899

Fix clang-tidy tests

9b80977

Fix clangd tests

091a731

Fix compiler-rt test

e42e46a

llvmbot added clang-tools-extra clangd clang-tidy labels Feb 19, 2025

Remove non-test changes for now

7fefe94

Sirraide added 2 commits February 19, 2025 22:07

Revert this test for now

c719809

clang-format

160adad

Sirraide changed the title ~~[Clang] Make -Wreturn-type default to an error in all language modes~~ [Clang] [NFC] Fix more -Wreturn-type warnings in tests everywhere Feb 19, 2025

AaronBallman approved these changes Feb 20, 2025

View reviewed changes

Sirraide merged commit b0210fe into llvm:main Feb 20, 2025
8 checks passed

Sirraide mentioned this pull request Feb 20, 2025

[Clang] Fix failing clang-tidy test #128051

Merged

Sirraide added a commit that referenced this pull request Feb 20, 2025

[Clang] Fix failing clang-tidy test (#128051)

29361b3

#123470 broke one of the clang-tidy tests; this fixes that.

Sirraide deleted the wreturn-type-error branch March 13, 2025 19:50

[Clang] [NFC] Fix more -Wreturn-type warnings in tests everywhere #123470

[Clang] [NFC] Fix more -Wreturn-type warnings in tests everywhere #123470

Uh oh!

Conversation

Sirraide commented Jan 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jan 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sirraide commented Jan 18, 2025

Uh oh!

Sirraide commented Jan 18, 2025

Uh oh!

cor3ntin commented Jan 19, 2025

Uh oh!

jeremy-rifkin commented Jan 19, 2025

Uh oh!

Sirraide commented Jan 19, 2025

Uh oh!

shafik left a comment

Choose a reason for hiding this comment

Uh oh!

Sirraide commented Jan 23, 2025

Uh oh!

Sirraide commented Jan 23, 2025

Uh oh!

AaronBallman commented Jan 23, 2025

Uh oh!

Sirraide commented Jan 23, 2025

Uh oh!

shafik commented Jan 23, 2025

Uh oh!

thesamesam commented Jan 27, 2025

Uh oh!

thesamesam commented Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sirraide commented Jan 27, 2025

Uh oh!

Sirraide commented Jan 27, 2025

Uh oh!

thesamesam commented Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sirraide commented Jan 27, 2025

Uh oh!

thesamesam commented Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sirraide commented Feb 19, 2025

Uh oh!

AaronBallman commented Feb 19, 2025

Uh oh!

github-actions bot commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sirraide commented Feb 19, 2025

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Sirraide commented Feb 20, 2025

Uh oh!

Uh oh!

llvm-ci commented Feb 20, 2025

Uh oh!

Sirraide commented Feb 20, 2025

Uh oh!

Sirraide commented Feb 20, 2025

Uh oh!

Uh oh!

[Clang] [NFC] Fix more `-Wreturn-type` warnings in tests everywhere #123470

[Clang] [NFC] Fix more `-Wreturn-type` warnings in tests everywhere #123470

Sirraide commented Jan 18, 2025 •

edited

Loading

llvmbot commented Jan 18, 2025 •

edited

Loading

thesamesam commented Jan 27, 2025 •

edited

Loading

thesamesam commented Jan 27, 2025 •

edited

Loading

thesamesam commented Jan 27, 2025 •

edited

Loading

github-actions bot commented Feb 19, 2025 •

edited

Loading