[C89/C2x] Diagnose calls to a function without a prototype but passes arguments

[aaron.ballman]

Summary

This catches places where a function without a prototype is accidentally used, potentially passing an incorrect number of arguments, and is a follow-up to the work done in https://reviews.llvm.org/D122895 and described in the RFC (https://discourse.llvm.org/t/rfc-enabling-wstrict-prototypes-by-default-in-c). The diagnostic is grouped under the new -Wdeprecated-non-prototypes warning group and is enabled by default.

The diagnostic is disabled if the function being called was implicitly declared (the user already gets an on-by-default warning about the creation of the implicit function declaration, so no need to warn them twice on the same line). Additionally, the diagnostic is disabled if the declaration of the function without a prototype was in a location where the user explicitly disabled deprecation warnings for functions without prototypes (this allows the provider of the API a way to disable the diagnostic at call sites because the lack of prototype is intentional).

[cor3ntin]

LGTM except a couple of nitpicks

clang/lib/Sema/SemaExpr.cpp

7052

I have a hard time parsing this sentence. I suspect words or punctuation are missing

7065

!= nullptr may be cleaner

[aaron.ballman]

Updated based on review comments.

clang/lib/Sema/SemaExpr.cpp

7052

I reworded to mention the warning group specifically; that hopefully clears up the confusion.

7065

Sure!

[cor3ntin]

clang/lib/Sema/SemaExpr.cpp

7052

Excellent !

[erichkeane]

clang/docs/ReleaseNotes.rst

146

This reads awkwardly to me... Maybe something like?

Additionally, it will diagnose calls with provided arguments to a function without a prototype. This warning is enabled only when the `-Wdeprecated-non-prototype` option is enabled at the function declaration, which allows a developer to disable the diagnostic for all callers at the point of declaration.

clang/include/clang/Basic/DiagnosticSemaKinds.td

5558

perhaps:

"function %select{|%1}0 without prototype called with arguments specified"

or something like that?

[aaron.ballman]

Updated based on review feedback (reworded diagnostic and release note).

clang/docs/ReleaseNotes.rst

146

Oh, this is much better, thank you!

clang/include/clang/Basic/DiagnosticSemaKinds.td

5558

After some off-list discussion, we landed on some new wording here that better describes the issue with the user's code instead of just describing what the user's code does and hoping they infer the issue from there. :-D

[aaron.ballman]

FYI -- I plan to land this tomorrow unless I hear more feedback from reviewers (but as always, I'll happily take post-commit feedback as well).

[aaron.ballman]

I've landed in 33d3fc4466479285121cbb1a62db249454da0bda

clang/docs/ReleaseNotes.rst

- ``-Wunused-variable`` no longer warn for references extending the lifetime
   <https://github.com/llvm/llvm-project/issues/54489>`_.
 - Modified the behavior of ``-Wstrict-prototypes`` and added a new, related
   diagnostic ``-Wdeprecated-non-prototype``. The strict prototypes warning will
   now only diagnose deprecated declarations and definitions of functions
   without a prototype where the behavior in C2x will remain correct. This
   diagnostic remains off by default but is now enabled via ``-pedantic`` due to
   it being a deprecation warning. ``-Wdeprecated-non-prototype`` will diagnose
   cases where the deprecated declarations or definitions of a function without
-  a prototype will change behavior in C2x. This diagnostic is grouped under the
-  ``-Wstrict-prototypes`` warning group, but is enabled by default.
+  a prototype will change behavior in C2x. Additionally, it will diagnose calls
+  which pass arguments to a function without a prototype. This warning is
+  enabled only when the ``-Wdeprecated-non-prototype`` option is enabled at the
+  function declaration site, which allows a developer to disable the diagnostic
+  for all callers at the point of declaration. This diagnostic is grouped under
+  the ``-Wstrict-prototypes`` warning group, but is enabled by default.
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 - Improve __builtin_dump_struct:
   - Support bitfields in struct and union.
   - Improve the dump format, dump both bitwidth(if its a bitfield) and field value.
   - Remove anonymous tag locations.
   - Beautify dump format, add indent for nested struct and struct members.

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def warn_not_enough_argument : Warning<
   "not enough variable arguments in %0 declaration to fit a sentinel">,
   InGroup<Sentinel>;
 def warn_missing_sentinel : Warning <
   "missing sentinel in %select{function call|method dispatch|block call}0">,
   InGroup<Sentinel>;
 def note_sentinel_here : Note<
   "%select{function|method|block}0 has been explicitly marked sentinel here">;
+def warn_strict_uses_without_prototype : Warning<
+  "passing arguments to %select{a function|%1}0 without a prototype is "
+  "deprecated in all versions of C and is not supported in C2x">,
+  InGroup<DeprecatedNonPrototype>;
 def warn_missing_prototype : Warning<
   "no previous prototype for function %0">,
   InGroup<DiagGroup<"missing-prototypes">>, DefaultIgnore;
 def note_declaration_not_a_prototype : Note<
   "this declaration is not a prototype; add %select{'void'|parameter declarations}0 "
   "to make it %select{a prototype for a zero-parameter function|one}0">;
 // This is not actually an extension, but we only want it to be enabled in
 // -pedantic mode and this is the most direct way of accomplishing that.

clang/lib/Sema/SemaExpr.cpp

       }
 
       // If the function we're calling isn't a function prototype, but we have
       // a function prototype from a prior declaratiom, use that prototype.
       if (!FDecl->hasPrototype())
         Proto = FDecl->getType()->getAs<FunctionProtoType>();
     }
 
+    // If we still haven't found a prototype to use but there are arguments to
+    // the call, diagnose this as calling a function without a prototype.
+    // However, if we found a function declaration, check to see if
+    // -Wdeprecated-non-prototype was disabled where the function was declared.
+    // If so, we will silence the diagnostic here on the assumption that this
+    // interface is intentional and the user knows what they're doing. We will
+    // also silence the diagnostic if there is a function declaration but it
+    // was implicitly defined (the user already gets diagnostics about the
+    // creation of the implicit function declaration, so the additional warning
+    // is not helpful).
+    if (!Proto && !Args.empty() &&
+        (!FDecl || (!FDecl->isImplicit() &&
+                    !Diags.isIgnored(diag::warn_strict_uses_without_prototype,
+                                     FDecl->getLocation()))))
+      Diag(LParenLoc, diag::warn_strict_uses_without_prototype)
+          << (FDecl != nullptr) << FDecl;
+
     // Promote the arguments (C99 6.5.2.2p6).
     for (unsigned i = 0, e = Args.size(); i != e; i++) {
       Expr *Arg = Args[i];
 
       if (Proto && i < Proto->getNumParams()) {
         InitializedEntity Entity = InitializedEntity::InitializeParameter(
             Context, Proto->getParamType(i), Proto->isParamConsumed(i));
         ExprResult ArgE =

clang/test/Analysis/nullability.c

-// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,nullability -verify %s
+// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,nullability -Wno-deprecated-non-prototype -verify %s
 
 void it_takes_two(int a, int b);
 void function_pointer_arity_mismatch() {
   void(*fptr)() = it_takes_two;
   fptr(1); // no-crash expected-warning {{Function taking 2 arguments is called with fewer (1)}}
 }
 
 void block_arity_mismatch() {
   void(^b)() = ^(int a, int b) { };
   b(1);  // no-crash expected-warning {{Block taking 2 arguments is called with fewer (1)}}
 }

clang/test/Analysis/svalbuilder-float-cast.c

-// RUN: %clang_analyze_cc1 -analyzer-checker debug.ExprInspection -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker debug.ExprInspection -Wno-deprecated-non-prototype -verify %s
 void clang_analyzer_denote(int, const char *);
 void clang_analyzer_express(int);
 
 void SymbolCast_of_float_type_aux(int *p) {
   *p += 0;
   // FIXME: Ideally, all unknown values should be symbolicated.
   clang_analyzer_denote(*p, "$x"); // expected-warning{{Not a symbol}}
 

clang/test/Sema/warn-deprecated-non-prototype.c

 void one_more(a, b) int a, b; {} // both-warning {{a function declaration without a prototype is deprecated in all versions of C and is not supported in C2x}}
 
 void sheesh(int a);
 void sheesh(a) int a; {} // both-warning {{a function declaration without a prototype is deprecated in all versions of C and is not supported in C2x}}
 
 void another(); // strict-warning {{a function declaration without a prototype is deprecated in all versions of C}}
 
 int main(void) {
-  another(1, 2); // OK for now
+  another(1, 2);  // both-warning {{passing arguments to 'another' without a prototype is deprecated in all versions of C and is not supported in C2x}}
 }
 
 void order1();        // expected-warning {{a function declaration without a prototype is deprecated in all versions of C and is not supported in C2x}} \
                          strict-warning {{a function declaration without a prototype is deprecated in all versions of C}} \
                          strict-note {{a function declaration without a prototype is not supported in C2x}}
 void order1(int i);   // both-warning {{a function declaration without a prototype is deprecated in all versions of C and is not supported in C2x}}
 
 void order2(int i);

 // comes from merging the function declarations together. The second is the
 // point at which we know the behavior has changed (because we can see the
 // previous declaration at that point), but we've already issued the type
 // warning by that point. It's not ideal to be this chatty, but this situation
 // should be pretty rare.
 void blapp(int);
 void blapp() { } // both-warning {{a function declaration without a prototype is deprecated in all versions of C and is not supported in C2x}} \
                  // strict-warning {{a function declaration without a prototype is deprecated in all versions of C}}
+
+// Disable -Wdeprecated-non-prototype
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-non-prototype"
+void depr_non_prot(); // strict-warning {{a function declaration without a prototype is deprecated in all versions of C}}
+#pragma GCC diagnostic pop
+// Reenable it.
+
+// Disable -Wstrict-prototypes
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wstrict-prototypes"
+void strict_prot(); // OK
+#pragma GCC diagnostic pop
+// Reenable it.
+
+void calls(void) {
+  // Ensure that we diagnose calls to functions without a prototype, but only
+  // if they pass arguments.
+  never_defined(); // OK
+  never_defined(1); // both-warning {{passing arguments to 'never_defined' without a prototype is deprecated in all versions of C and is not supported in C2x}}
+
+  // Ensure that calls to builtins without a traditional prototype are not
+  // diagnosed.
+  (void)__builtin_isless(1.0, 1.0); // OK
+
+  // Calling a function whose prototype was provided by a function with an
+  // identifier list is still fine.
+  func(1, 2); // OK
+
+  // Ensure that a call through a function pointer is still diagnosed properly.
+  fp f;
+  f(); // OK
+  f(1, 2); // both-warning {{passing arguments to a function without a prototype is deprecated in all versions of C and is not supported in C2x}}
+
+  // Ensure that we don't diagnose when the diagnostic group is disabled.
+  depr_non_prot(1); // OK
+  strict_prot(1); // OK
+
+  // Ensure we don't issue diagnostics if the function without a prototype was
+  // later given a prototype by a definintion. Also ensure we don't duplicate
+  // diagnostics if such a call is incorrect.
+  func(1, 2); // OK
+  func(1, 2, 3); // both-warning {{too many arguments in call to 'func'}}
+}