[Patch] Diagnose unused declaration

Clang Frontend
1

Hello

Please find attached a patch in order to diagnose unused declaration.
This sould solve 2739 – Unused function parameters

As I'm new to clang, I'm not sure if I did it the right way.
Comments are welcome.

unused.patch (6.29 KB)

2

EdB wrote:

Hello

Please find attached a patch in order to diagnose unused declaration.
This sould solve 2739 – Unused function parameters

As I'm new to clang, I'm not sure if I did it the right way.
Comments are welcome.
  

Looks good. There seems to be a whitespace issue in
ActOnEndOfTranslationUnit.

Sebastian

3

Looks good. There seems to be a whitespace issue in
ActOnEndOfTranslationUnit.

Pleased find attached a new version:
It fixed some crash.
Unused warning is no longer actived by default. You should enable it
with -Wunused_declaration

unused.patch (7.33 KB)

4

I'm not certain that the approach here will work. Since an IdentifierInfo is not unique to a declaration using a bit in IdentifierInfo to perform this check will be ineffective in many cases. For example:

void f() {
   int x = ...
   x++;
}

void g() {
    int x; // no warning will be issued
}

The other thing that bothers me about this is putting this bit in IdentifierInfo in the first place. It seems like a very invasive thing to do for a simple check like this, especially one that isn't always used. Generally we like to keep such information "on the side" unless there is a good reason not to. Why not use a DenseSet to keep track (on the side) of what declarations are used? You can then do this on a declaration based (i.e., for a particular NamedDecl) rather than using IdentifierInfo's in this conflated way.

5

Ted Kremenek wrote:-

>> Looks good. There seems to be a whitespace issue in
>> ActOnEndOfTranslationUnit.
>
> Pleased find attached a new version:
> It fixed some crash.
> Unused warning is no longer actived by default. You should enable it
> with -Wunused_declaration

I'm not certain that the approach here will work. Since an
IdentifierInfo is not unique to a declaration using a bit in
IdentifierInfo to perform this check will be ineffective in many
cases. For example:

void f() {
   int x = ...
   x++;
}

void g() {
    int x; // no warning will be issued
}

The other thing that bothers me about this is putting this bit in
IdentifierInfo in the first place. It seems like a very invasive
thing to do for a simple check like this, especially one that isn't
always used. Generally we like to keep such information "on the side"
unless there is a good reason not to. Why not use a DenseSet to keep
track (on the side) of what declarations are used? You can then do
this on a declaration based (i.e., for a particular NamedDecl) rather
than using IdentifierInfo's in this conflated way.

Further, in standard C90

  int z; sizeof z;

z is generally not considered "used". C99 adds more complexity.
In C++ the rules are vastly more complicated.

Neil.

6

Hello

This time "used" informations are keep "on the side".
There is place for improvement but I want to be sure that information are at
the right place before.

unused.patch (7.23 KB)

7

Hello,

This time "used" informations are keep "on the side".

Thanks; that's much better.

There is place for improvement but I want to be sure that information are at
the right place before.

A few comments on this iteration:

  static llvm::cl::opt<bool>
+WarnUnusedDeclaration("Wunused_declaration",
+ llvm::cl::desc("Warn for unused declaration"));

I suggest that we use the same command-line arguments as GCC, unless there is some specific reason to deviate from them:

  Warning Options - Using the GNU Compiler Collection (GCC)

In Sema::PopDeclContext(), the warning-generation code needs to be much more picky about which declarations it complains about. For example, I tried this C++ example:

namespace N {
   int x;
}

namespace N {
   int f() {
     return x;
   }
}

and Clang produced 4 warnings: one each for x, f, __builtin_va_list, and N:

This option should not produce warnings about any of these, because:

  - x is the definition of a variable that can be referred to by other translation units, so even if it isn't used within this translation unit, we should not warn about it. Plus, "x" is used later on in the program (in a different DeclContext for the namespace N). Note that GCC's -Wunused-variable documentation explicitly says that it only warns about local variables and non-constant static variables; we should do something similar.

  - f is the definition of a function, which could also be referenced by other translation units. Note that GCC's -Wunused-function only complains about static functions (and, presumably, functions in anonymous namespaces) because those aren't visible outside of the translation unit.

  - __builtin_va_list wasn't written by the user, so we shouldn't complain about it. Besides, do we even want to complain about unused typedefs? Especially in C++, there are certainly reasons to write typedefs even if they aren't used in the translation unit.

  - N is a namespace; do we ever want to warn about those?

I see that UsedDeclaration grows throughout the processing of the translation unit, because nothing is ever removed from the set (even when it is no longer visible through name lookup). We'd like to avoid that.

Does this warning work properly in C? In Sema::LookupDecl, it looks like you need to change

     for (; I != IdResolver.end(); ++I)
       if ((*I)->getIdentifierNamespace() & NS)
  return *I;

to

     for (; I != IdResolver.end(); ++I)
       if ((*I)->getIdentifierNamespace() & NS) {
  UsedDeclaration.insert(*I);
  return *I;
       }

?

Also in Sema::LookupDecl:

- return MaybeConstructOverloadSet(Context, I, E);
+ Decl *D = MaybeConstructOverloadSet(Context, I, E);
+ UsedDeclaration.insert(static_cast<NamedDecl *>(D));
+ return D;

The result of MaybeConstructOverloadSet could be an OverloadedFunctionDecl, which is transient and therefore should not be inserted into the UsedDeclaration set (since we'd never know to remove it).

Do we need the new code in ActOnEndOfTranslationUnit? It's nearly identical to the new code in Sema::PopDeclContext, and translation units are DeclContexts. Perhaps this code could be factored out?

Personally, I think that any patch introducing warnings for unused variables/parameters/functions/etc. also needs to support GCC's __attribute__((unused)) extension to turn off the warning for specific declarations. We'll get a lot of complaints from GCC users if we produce this warning when they've explicitly asked to suppress it for a declaration.

You'll need to include a regression test that works with -verify along with your patch, so that we can make sure this functionality doesn't break in the future. You'll probably need at least two regression tests, one each for C and C++. Try to exercise a bunch of different language features in them, e.g., overloading in C++, uses of "extern", "inline", etc. Getting this warning right is actually far harder than it looks at first blush, but you've got a good start on it.

  - Doug

8

Douglas Gregor wrote:

Hello,

There is place for improvement but I want to be sure that
information are at
the right place before.
    
A few comments on this iteration:

In Sema::PopDeclContext(), the warning-generation code needs to be
much more picky about which declarations it complains about. For
example, I tried this C++ example:

namespace N {
   int x;
}

namespace N {
   int f() {
     return x;
   }
}

and Clang produced 4 warnings: one each for x, f, __builtin_va_list,
and N:

This option should not produce warnings about any of these, because:

  - x is the definition of a variable that can be referred to by other
translation units, so even if it isn't used within this translation
unit, we should not warn about it. Plus, "x" is used later on in the
program (in a different DeclContext for the namespace N). Note that
GCC's -Wunused-variable documentation explicitly says that it only
warns about local variables and non-constant static variables; we
should do something similar.

  - f is the definition of a function, which could also be referenced
by other translation units. Note that GCC's -Wunused-function only
complains about static functions (and, presumably, functions in
anonymous namespaces) because those aren't visible outside of the
translation unit.

  - __builtin_va_list wasn't written by the user, so we shouldn't
complain about it. Besides, do we even want to complain about unused
typedefs? Especially in C++, there are certainly reasons to write
typedefs even if they aren't used in the translation unit.

  - N is a namespace; do we ever want to warn about those?

I see that UsedDeclaration grows throughout the processing of the
translation unit, because nothing is ever removed from the set (even
when it is no longer visible through name lookup). We'd like to avoid
that.
  

I think both problems could be solved together by, instead of keeping
track of declarations that are used, keeping track of those that are unused.
In other words, the UsedDeclarations becomes UnusedDeclarations. When a
new declaration is encountered, it is added to the set if
1) it is function-local or TU-local and
2) it does not have the unused attribute.
When a declaration is used, it is removed from the set. When a scope is
left, we check which of the declarations now going out of scope are
still in the set and warn about those.

Sebastian

9

Great idea! This set should also be smaller than the UsedDeclarations set would be.

  - Doug

10

I agree, that is a very nice approach.

-Chris