Suggestions to fix -Wmissing-{prototypes,variable-declarations}

[aaronpuchert]

Summary

I've found that most often the proper way to fix this warning is to add
static, because if the code otherwise compiles and links, the function
or variable is apparently not needed outside of the TU.

We can't provide a fix-it hint for variable declarations, because
multiple VarDecls can share the same type, and if we put static in front
of that we affect all declared variables, some if which might have
previous declarations.

We also provide no fix-it hint for the rare case of an extern function
definition, because that would require removing extern and I have no
idea how to get the source location of the storage class specifier from
a FunctionDecl. I believe this information is only available earlier in
the AST construction from DeclSpec::getStorageClassSpecLoc(), but we
don't have that here.

[aaronpuchert]

test/Sema/warn-missing-prototypes.c

7 ↗

(On Diff #190770)

Maybe there shouldn't be a fix-it here, as there is a declaration, which is just not a prototype.

17–18 ↗

(On Diff #190770)

As I wrote in the commit message: this could be fixed by replacing extern with static, but I don't think we have the SourceLocation for the storage class specifier available where the warning is emitted. Maybe I'm overlooking something though.

[aaronpuchert]

Don't suggest adding static if there is a non-prototype declaration. This required a minor refactoring: we let ShouldWarnAboutMissingPrototype return any kind of declaration it finds and check for the number of parameters at the call site.

[aaronpuchert]

Ping?

[aaronpuchert]

Another ping. I've tried to add the original authors as reviewers, but both warnings are several years old and you might not remember.

[ed]

The patch looks pretty good as far as I can see. That said, I've been out of the loop for quite a while, so do take my approval with a grain of salt.

Thanks for working on this! \o/

[aaron.ballman]

My only real concern about this is that sometimes the fix-it will be wrong because the issue is a typo in the definition. However, it seems like that could be handled by preferring typo correction when the edit distance is below a certain threshold or something along those lines, and could be done later.

lib/Sema/SemaDecl.cpp

13216 ↗

(On Diff #190966)

You can drop the parens around the != operator subexpressions.

13220 ↗

(On Diff #190966)

You can drop the parens here.

[aaronpuchert]

In D59402#1515703, @aaron.ballman wrote:

My only real concern about this is that sometimes the fix-it will be wrong because the issue is a typo in the definition.

You're right about that, there are multiple reasons why one would see the warning. But if your program compiles and links fine, then this seems to be the only reason left.

• If the function is in a header and should be declared inline, you're most likely getting linker errors with duplicate symbols.
• If there is a typo, and the definition doesn't match the declaration, there should also be a linker error about a missing symbol.

We had a discussion in our team about activating the warning, and a colleague of mine remarked "to fix this, you would have to declare every function before defining it, even when its only declared & defined in a source file". That's when I realized the warning might be a bit misleading, and suggesting to add static could fix that. It's also how the warning should be fixed in our code in almost all cases. (There are a few cases where it fires in a header that is only included once.)

[thakis]

Please see https://clang.llvm.org/docs/InternalsManual.html#fix-it-hints , especially the "need to obey these rules" bit. I'm not sure that's fulfilled here. Maybe the fixit should be on a note instead?

[aaronpuchert]

I guess you're referring to "[fix-it hints] should only be used when it’s very likely they match the user’s intent".

When turning on the warning on an existing code base, I think that static is almost always right. But when writing new code with the warning active, it might indeed not be the right thing. It could be that the declaration has been forgotten, or it has a typo. We wouldn't want users to apply static blindly, so a note explaining when it is appropriate does actually make a lot of sense. Perhaps I can also detect if this is in a header and not emit the note then. (Or emit a note suggesting inline.)

@aaron.ballman Would moving the fix-it to a note alleviate your concerns?

[aaron.ballman]

In D59402#1516421, @aaronpuchert wrote:

I guess you're referring to "[fix-it hints] should only be used when it’s very likely they match the user’s intent".

Also, we're not attempting to recover from the error, which is a good point that @thakis raised. aka, if you apply the fix-it, you should also treat the declaration as though it were declared static.

When turning on the warning on an existing code base, I think that static is almost always right. But when writing new code with the warning active, it might indeed not be the right thing. It could be that the declaration has been forgotten, or it has a typo. We wouldn't want users to apply static blindly, so a note explaining when it is appropriate does actually make a lot of sense. Perhaps I can also detect if this is in a header and not emit the note then. (Or emit a note suggesting inline.)

@aaron.ballman Would moving the fix-it to a note alleviate your concerns?

Yes, it would -- you also wouldn't have to attempt the recovery in this case.

[aaronpuchert]

In D59402#1516432, @aaron.ballman wrote:

Also, we're not attempting to recover from the error, which is a good point that @thakis raised. aka, if you apply the fix-it, you should also treat the declaration as though it were declared static.

I think the recovery rule only applies to errors, there is no need to recover from a warning. If we attempted recovery from a warning, we might generate different object code depending on the warning level.

But I think your original comment nails it: we can't be really sure that this is the right fix, and my empirical data doesn't (necessarily) translate well to writing new code with the warning on.

[rsmith]

lib/Sema/SemaDecl.cpp

11810 ↗

(On Diff #190966)

It would be more appropriate to suppress the fixit if any storage class specifier is present, since a declaration can have only one such specifier.

13225–13230 ↗

(On Diff #190966)

If we produce this note, we should not also produce the "add static" suggestion.

[rsmith]

In D59402#1516479, @aaronpuchert wrote:

In D59402#1516432, @aaron.ballman wrote:

Also, we're not attempting to recover from the error, which is a good point that @thakis raised. aka, if you apply the fix-it, you should also treat the declaration as though it were declared static.

I think the recovery rule only applies to errors, there is no need to recover from a warning.

That is incorrect, because -Werror can be used to promote warnings to errors. Please see https://clang.llvm.org/docs/InternalsManual.html#fix-it-hints for the rule and rationale; if it's insufficiently clear on this, we should fix it.

[aaronpuchert]

In D59402#1516482, @rsmith wrote:

In D59402#1516479, @aaronpuchert wrote:

In D59402#1516432, @aaron.ballman wrote:

Also, we're not attempting to recover from the error, which is a good point that @thakis raised. aka, if you apply the fix-it, you should also treat the declaration as though it were declared static.

I think the recovery rule only applies to errors, there is no need to recover from a warning.

That is incorrect, because -Werror can be used to promote warnings to errors. Please see https://clang.llvm.org/docs/InternalsManual.html#fix-it-hints for the rule and rationale; if it's insufficiently clear on this, we should fix it.

Ok, but can't we generally recover from warnings with a no-op, because it's still valid C++? Neither parser nor the rest of Sema should be held up by this. Don't we in fact have to recover with a no-op to be compliant with the standard?

So my understanding of the rules is that the recovery rule only applies to "actual" errors, which would prevent continuing if we didn't recover. If that is wrong, then my reading is that fix-it hints for warnings must always be applied to a note, because the recovery must be a no-op. Otherwise the rules are pretty clear, except that perhaps "when it’s very likely they match the user’s intent" is open to interpretation. But I think we agreed here that it is not "very likely", so it should be a note.

lib/Sema/SemaDecl.cpp

13225–13230 ↗

(On Diff #190966)

They are exclusive because we suggest static only if PossiblePrototype is false, and this note only when it's true. But you're right that this isn't obvious, I'll make it an if/else.

[aaronpuchert]

In D59402#1516567, @aaronpuchert wrote:

If that is wrong, then my reading is that fix-it hints for warnings must always be applied to a note, because the recovery must be a no-op.

Not true, because some warnings provide fix-it hints that don't actually change anything except suppressing the warning, for example -Wshift-op-parentheses.

Perhaps we could clarify in the docs that fix-it hints on warnings are appropriate only if they don't change anything except suppressing the warning.

[rsmith]

In D59402#1516589, @aaronpuchert wrote:

Perhaps we could clarify in the docs that fix-it hints on warnings are appropriate only if they don't change anything except suppressing the warning.

That sounds like a great idea. Do you want to put together a quick documentation patch? :)

[aaronpuchert]

I discovered an issue with variables, because there can be multiple definitions with the same TypeLoc:

extern int a;
int a, b;

We emit the warning for b, but we can't just put static in front of the declaration, as that would affect a as well. I haven't found a way to determine whether a VarDecl shares its type with another declaration, especially since global declarations don't even belong to a DeclStmt. We just get

TranslationUnitDecl 0x1d0b598 <<invalid sloc>> <invalid sloc>
|-...
|-VarDecl 0x1d47b60 <<stdin>:1:1, col:12> col:12 a 'int' extern
|-VarDecl 0x1d47c18 prev 0x1d47b60 <line:2:1, col:5> col:5 a 'int'
`-VarDecl 0x1d47c90 <col:1, col:8> col:8 b 'int'

So I think I have to drop the fix-it hint there, but I can still emit a note.

[aaronpuchert]

I've factored out some changes into D62750 and will rebase this on top if it.

[aaronpuchert]

Show note suggesting to add static, move fix-it to note for functions, remove fix-it for variables. Show note even for extern function definitions when we can't build a proper fix-it.

[aaronpuchert]

Remove other change from this one.

[aaronpuchert]

Add missing CHECK-NOTs for -Wmissing-prototypes.

[aaron.ballman]

lib/Sema/SemaDecl.cpp

13340–13342 ↗

(On Diff #202483)

Formatting here is a bit off -- you should run through clang-format. Also, I'd appreciate curly braces even though the else clause is technically one line.

13345–13346 ↗

(On Diff #202483)

We may not want to produce the fixit if there's a macro involved in the declaration. Consider:

#ifdef SOMETHING
#define FROBBLE static
#else
#define FROBBLE
#endif

FROBBLE void foo(void);

We probably don't want the fixit in the case SOMETHING is not defined.

[aaronpuchert]

lib/Sema/SemaDecl.cpp

13345–13346 ↗

(On Diff #202483)

I think that's generally an issue with fix-its, there could always be a macro that turns the code into something entirely different. If we look at the other fix-it above, we can construct

#define VOID
int f(VOID);
int f() { return 0; }

Then we get:

<stdin>:3:5: warning: no previous prototype for function 'f' [-Wmissing-prototypes]
int f() { return 0; }
    ^
<stdin>:2:5: note: this declaration is not a prototype; add 'void' to make it a prototype for a zero-parameter function
int f(VOID);
    ^
          void

Then the fix-it doesn't even work in the original configuration, because it produces int f(VOIDvoid). If we make it work by adding a space, we still have the problem that you mentioned: if someone defines VOID as void, we then have int f(void void) after applying the fix-it in the original setting.

Trying to make fix-its work with macros is probably a hopeless endeavor.

[aaron.ballman]

lib/Sema/SemaDecl.cpp

13345–13346 ↗

(On Diff #202483)

Trying to make fix-its work with macros is probably a hopeless endeavor.

That's what I was getting at -- I think you need to see if there's a macro at the insertion location and not generate a fix-it in that case. The above suffers from the same issue (and can be corrected in a subsequent patch, if desired).

[aaronpuchert]

lib/Sema/SemaDecl.cpp

13345–13346 ↗

(On Diff #202483)

How can I know that? The AST seems to contain no such information, for the example I only see the following:

TranslationUnitDecl 0x12574e8 <<invalid sloc>> <invalid sloc>
|-[...]
|-FunctionDecl 0x12b5340 <<stdin>:2:1, col:11> col:5 f 'int ()'
`-FunctionDecl 0x12b5440 prev 0x12b5340 <line:3:1, col:21> col:5 f 'int ()'
  `-CompoundStmt 0x12b5508 <col:9, col:21>
    `-ReturnStmt 0x12b54f8 <col:11, col:18>
      `-IntegerLiteral 0x12b54d8 <col:18> 'int' 0

It seems fix-its are automatically suppressed if they would land in a macro definition, like

#define X int f()
X;
int f() { return 0; }

produces

<stdin>:3:5: warning: no previous prototype for function 'f' [-Wmissing-prototypes]
int f() { return 0; }
    ^
<stdin>:2:1: note: this declaration is not a prototype; add 'void' to make it a prototype for a zero-parameter function
X;
^
<stdin>:1:15: note: expanded from macro 'X'
#define X int f()
              ^

So there is no fix-it suggested, although we produce one. I guess some intermediate layer that knows about the connection to the original source detects that and throws it away.

But what you want is that we also suppress it if there is any macro (it wouldn't need to be at the beginning, it could also be void FROBBLE foo(void) with the same meaning). That would also mean I can't apply the fix-it even if the macros is unrelated, say if someone has

#define ATTR_PURE __attribute__((pure))
ATTR_PURE int f() { return 0; }

Other fix-its (that I have looked at) just seem to ignore macros, like we do, relying on fix-its to be discarded if they would land inside of a macro.

I think there is a case to be made for avoiding int f(VOIDvoid), but not suggesting the fix-it because a macro could have another meaning in another configuration seems unusual to me.

[aaron.ballman]

LGTM!

lib/Sema/SemaDecl.cpp

13345–13346 ↗

(On Diff #202483)

Okay, good point about void FOO blah(void); being just as problematic. I was thinking you would look at FD->getTypeSpecStartLoc().isMacroID(), but that would be insufficient and may be handled by an intermediate layer anyway.

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

def note_declaration_not_a_prototype : Note<
   "to make it %select{a prototype for a zero-parameter function|one}0">;
 def warn_strict_prototypes : Warning<
   "this %select{function declaration is not|block declaration is not|"
   "old-style function definition is not preceded by}0 a prototype">,
   InGroup<DiagGroup<"strict-prototypes">>, DefaultIgnore;
 def warn_missing_variable_declarations : Warning<
   "no previous extern declaration for non-static variable %0">,
   InGroup<DiagGroup<"missing-variable-declarations">>, DefaultIgnore;
+def note_static_for_internal_linkage : Note<
+  "declare 'static' if the %select{variable|function}0 is not intended to be "
+  "used outside of this translation unit">;
 def err_static_data_member_reinitialization :
   Error<"static data member %0 already has an initializer">;
 def err_redefinition : Error<"redefinition of %0">;
 def err_alias_after_tentative :
   Error<"alias definition of %0 after tentative definition">;
 def err_alias_is_definition :
   Error<"definition %0 cannot also be an %select{alias|ifunc}1">;
 def err_definition_of_implicitly_declared_member : Error<

cfe/trunk/lib/Sema/SemaDecl.cpp

if (var->isThisDeclarationADefinition() &&
       !isTemplateInstantiation(var->getTemplateSpecializationKind()) &&
       !getDiagnostics().isIgnored(diag::warn_missing_variable_declarations,
                                   var->getLocation())) {
     // Find a previous declaration that's not a definition.
     VarDecl *prev = var->getPreviousDecl();
     while (prev && prev->isThisDeclarationADefinition())
       prev = prev->getPreviousDecl();
 
-    if (!prev)
+    if (!prev) {
       Diag(var->getLocation(), diag::warn_missing_variable_declarations) << var;
+      Diag(var->getTypeSpecStartLoc(), diag::note_static_for_internal_linkage)
+          << /* variable */ 0;
+    }
   }
 
   // Cache the result of checking for constant initialization.
   Optional<bool> CacheHasConstInit;
   const Expr *CacheCulprit;
   auto checkConstInit = [&]() mutable {
     if (!CacheHasConstInit)
       CacheHasConstInit = var->getInit()->isConstantInitializer(
if (ShouldWarnAboutMissingPrototype(FD, PossiblePrototype)) {
           if (FunctionNoProtoTypeLoc FTL = TL.getAs<FunctionNoProtoTypeLoc>())
             Diag(PossiblePrototype->getLocation(),
                  diag::note_declaration_not_a_prototype)
                 << (FD->getNumParams() != 0)
                 << (FD->getNumParams() == 0
                         ? FixItHint::CreateInsertion(FTL.getRParenLoc(), "void")
                         : FixItHint{});
         }
+      } else {
+        Diag(FD->getTypeSpecStartLoc(), diag::note_static_for_internal_linkage)
+            << /* function */ 1
+            << (FD->getStorageClass() == SC_None
+                    ? FixItHint::CreateInsertion(FD->getTypeSpecStartLoc(),
+                                                 "static ")
+                    : FixItHint{});
       }
 
       // GNU warning -Wstrict-prototypes
       //   Warn if K&R function is defined without a previous declaration.
       //   This warning is issued only if the definition itself does not provide
       //   a prototype. Only K&R definitions do not provide a prototype.
       //   An empty list in a function declarator that is part of a definition
       //   of that function specifies that the function has no parameters

cfe/trunk/test/Sema/warn-missing-prototypes.c

 // RUN: %clang_cc1 -fsyntax-only -Wdocumentation -Wmissing-prototypes -verify %s
 // RUN: %clang_cc1 -fsyntax-only -Wdocumentation -Wmissing-prototypes -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
 
 int f(); // expected-note{{this declaration is not a prototype; add parameter declarations to make it one}}
 // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:{{.*}}-[[@LINE-1]]:{{.*}}}:"{{.*}}"
 
 int f(int x) { return x; } // expected-warning{{no previous prototype for function 'f'}}
 
 static int g(int x) { return x; }
 
 int h(int x) { return x; } // expected-warning{{no previous prototype for function 'h'}}
+// expected-note@-1{{declare 'static' if the function is not intended to be used outside of this translation unit}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:1-[[@LINE-2]]:1}:"static "
 
 static int g2();
 
 int g2(int x) { return x; }
 
+extern int g3(int x) { return x; } // expected-warning{{no previous prototype for function 'g3'}}
+// expected-note@-1{{declare 'static' if the function is not intended to be used outside of this translation unit}}
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:{{.*}}-[[@LINE-2]]:{{.*}}}:"{{.*}}"
+
 void test(void);
 
 int h3(); // expected-note{{this declaration is not a prototype; add parameter declarations to make it one}}
 // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:{{.*}}-[[@LINE-1]]:{{.*}}}:"{{.*}}"
 int h4(int);
 int h4();
 
 void test(void) {
   int h2(int x);
   int h3(int x);
   int h4();
 }
 
 int h2(int x) { return x; } // expected-warning{{no previous prototype for function 'h2'}}
+// expected-note@-1{{declare 'static' if the function is not intended to be used outside of this translation unit}}
 int h3(int x) { return x; } // expected-warning{{no previous prototype for function 'h3'}}
 int h4(int x) { return x; }
 
 int f2(int);
 int f2();
 
 int f2(int x) { return x; }
 
 // rdar://6759522
 int main(void) { return 0; }
 
 void not_a_prototype_test(); // expected-note{{this declaration is not a prototype; add 'void' to make it a prototype for a zero-parameter function}}
 // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:27-[[@LINE-1]]:27}:"void"
 void not_a_prototype_test() { } // expected-warning{{no previous prototype for function 'not_a_prototype_test'}}

cfe/trunk/test/Sema/warn-missing-variable-declarations.c

 // RUN: %clang_cc1 -Wmissing-variable-declarations -fsyntax-only -verify %s
 
 int vbad1; // expected-warning{{no previous extern declaration for non-static variable 'vbad1'}}
+// expected-note@-1{{declare 'static' if the variable is not intended to be used outside of this translation unit}}
 
 int vbad2;
 int vbad2 = 10; // expected-warning{{no previous extern declaration for non-static variable 'vbad2'}}
+// expected-note@-1{{declare 'static' if the variable is not intended to be used outside of this translation unit}}
 
-struct {
+struct { // expected-note{{declare 'static' if the variable is not intended to be used outside of this translation unit}}
   int mgood1;
 } vbad3; // expected-warning{{no previous extern declaration for non-static variable 'vbad3'}}
 
 int vbad4;
 int vbad4 = 10; // expected-warning{{no previous extern declaration for non-static variable 'vbad4'}}
+// expected-note@-1{{declare 'static' if the variable is not intended to be used outside of this translation unit}}
 extern int vbad4;
 
 extern int vgood1;
 int vgood1;
 int vgood1 = 10;

cfe/trunk/test/SemaCXX/warn-missing-prototypes.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify -Wmissing-prototypes -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -Wmissing-prototypes -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
 
 void f() { } // expected-warning {{no previous prototype for function 'f'}}
+// expected-note@-1{{declare 'static' if the function is not intended to be used outside of this translation unit}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:1-[[@LINE-2]]:1}:"static "
 
 namespace NS {
   void f() { } // expected-warning {{no previous prototype for function 'f'}}
+  // expected-note@-1{{declare 'static' if the function is not intended to be used outside of this translation unit}}
 }
 
 namespace {
   // Don't warn about functions in anonymous namespaces.
   void f() { }
 }
 
 struct A {

 
 // PR9519: don't warn about friend functions.
 class I {
   friend void I_friend() {}
 };
 
 // Don't warn on explicitly deleted functions.
 void j() = delete;
+
+extern void k() {} // expected-warning {{no previous prototype for function 'k'}}
+// expected-note@-1{{declare 'static' if the function is not intended to be used outside of this translation unit}}
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:{{.*}}-[[@LINE-2]]:{{.*}}}:"{{.*}}"

cfe/trunk/test/SemaCXX/warn-missing-variable-declarations.cpp

-// RUN: %clang -Wmissing-variable-declarations -fsyntax-only -Xclang -verify -std=c++17 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wmissing-variable-declarations -std=c++17 %s
 
 // Variable declarations that should trigger a warning.
 int vbad1; // expected-warning{{no previous extern declaration for non-static variable 'vbad1'}}
+// expected-note@-1{{declare 'static' if the variable is not intended to be used outside of this translation unit}}
+
 int vbad2 = 10; // expected-warning{{no previous extern declaration for non-static variable 'vbad2'}}
+// expected-note@-1{{declare 'static' if the variable is not intended to be used outside of this translation unit}}
 
 namespace x {
   int vbad3; // expected-warning{{no previous extern declaration for non-static variable 'vbad3'}}
+  // expected-note@-1{{declare 'static' if the variable is not intended to be used outside of this translation unit}}
 }
 
 // Variable declarations that should not trigger a warning.
 static int vgood1;
 extern int vgood2;
 int vgood2;
 static struct {
   int mgood1;
namespace {
   int vgood4;
 }
 
 inline int inline_var = 0;
 const int const_var = 0;
 constexpr int constexpr_var = 0;
 inline constexpr int inline_constexpr_var = 0;
 extern const int extern_const_var = 0; // expected-warning {{no previous extern declaration}}
+// expected-note@-1{{declare 'static' if the variable is not intended to be used outside of this translation unit}}
 extern constexpr int extern_constexpr_var = 0; // expected-warning {{no previous extern declaration}}
+// expected-note@-1{{declare 'static' if the variable is not intended to be used outside of this translation unit}}
 
 template<typename> int var_template = 0;
 template<typename> constexpr int const_var_template = 0;
 template<typename> static int static_var_template = 0;
 
 template int var_template<int[1]>;
 int use_var_template() { return var_template<int[2]>; }
 template int var_template<int[3]>;
 extern template int var_template<int[4]>;
 template<> int var_template<int[5]>; // expected-warning {{no previous extern declaration}}
+// expected-note@-1{{declare 'static' if the variable is not intended to be used outside of this translation unit}}
 
 // FIXME: We give this specialization internal linkage rather than inheriting
 // the linkage from the template! We should not warn here.
 template<> int static_var_template<int[5]>; // expected-warning {{no previous extern declaration}}
+// expected-note@-1{{declare 'static' if the variable is not intended to be used outside of this translation unit}}

cfe/trunk/test/SemaOpenCL/warn-missing-prototypes.cl

 // RUN: %clang_cc1 -fsyntax-only -verify -Wmissing-prototypes %s
 
 void f() { } // expected-warning {{no previous prototype for function 'f'}}
+// expected-note@-1{{declare 'static' if the function is not intended to be used outside of this translation unit}}
 
 // Don't warn about kernel functions.
 kernel void g() { }