Page MenuHomePhabricator

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

Authored by aaronpuchert on Mar 14 2019, 7:13 PM.

Details

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.

Diff Detail

Event Timeline

aaronpuchert created this revision.Mar 14 2019, 7:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2019, 7:13 PM
aaronpuchert added inline comments.Mar 14 2019, 7:21 PM
test/Sema/warn-missing-prototypes.c
7

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

18–19

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.

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 marked an inline comment as done.Mar 16 2019, 9:44 AM
aaronpuchert edited reviewers, added: riccibruno; removed: efriedma.Apr 26 2019, 3:51 AM
aaronpuchert removed a subscriber: riccibruno.

Ping?

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 accepted this revision.Thu, May 23, 10:45 PM

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/

This revision is now accepted and ready to land.Thu, May 23, 10:45 PM
aaron.ballman accepted this revision.Fri, May 24, 5:30 AM
aaron.ballman added a subscriber: 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
13331

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

13335

You can drop the parens here.

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.)

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 planned changes to this revision.Fri, May 24, 11:12 AM

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?

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.

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 added inline comments.Fri, May 24, 11:37 AM
lib/Sema/SemaDecl.cpp
11893

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.

13332–13339

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

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 marked an inline comment as done.Fri, May 24, 12:25 PM

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
13332–13339

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.

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.

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? :)

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.

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

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.

This revision is now accepted and ready to land.Fri, May 31, 2:13 PM

Remove other change from this one.

aaronpuchert marked 4 inline comments as done.Fri, May 31, 2:17 PM

Add missing CHECK-NOTs for -Wmissing-prototypes.

aaronpuchert retitled this revision from Fix-it hints for -Wmissing-{prototypes,variable-declarations} to Suggestions to fix -Wmissing-{prototypes,variable-declarations}.Tue, Jun 4, 3:28 PM
aaronpuchert edited the summary of this revision. (Show Details)
aaron.ballman added inline comments.Mon, Jun 17, 8:09 AM
lib/Sema/SemaDecl.cpp
13340–13342

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

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 added inline comments.Mon, Jun 17, 12:18 PM
lib/Sema/SemaDecl.cpp
13345–13346

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 added inline comments.Mon, Jun 17, 12:24 PM
lib/Sema/SemaDecl.cpp
13345–13346

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 added inline comments.Mon, Jun 17, 1:21 PM
lib/Sema/SemaDecl.cpp
13345–13346

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.