1232547 - (-Wunused-parameter) Consider enabling -Wunused-parameter

Status: REOPENED

(Firefox Build System :: General, defect)

Description

[Nicholas Nethercote [inactive]]

-Wunused-parameter finds lots of unused parameters in real code (see the blocking bugs), which is good. But it also requires modifying lots of code to make it usable, which is bad. But the modifications are very simple which is good.

Specifically, lots of arguments are legitimately unused, usually because the function is implementing an interface. In that case you just comment out the parameter name, viz:

> MobileViewportManager::Observe(nsISupports* aSubject, const char* aTopic,
>                                const char16_t* /*aData*/)
> {
>   [function body that doesn't use |aData|]
> }

Sometimes a parameter is used in one configuration but unused in another, e.g. a parameter might be only used in an assertion, i.e. in a debug build. In that case a compiler attribute is better, viz:

> bool isMagic(JSWhyMagic why) const {
> bool isMagic(MOZ_MAYBE_UNUSED JSWhyMagic why) const {
>     MOZ_ASSERT_IF(isMagic(), data.s.payload.why == why);
>     return JSVAL_IS_MAGIC_IMPL(data);
> }

(MOZ_MAYBE_UNUSED isn't in the repository yet; I will attach a patch defining it shortly.)

I'm not 100% convinced that we should turn -Wunused-parameters on for good, because so many modifications are required. I'm currently experimenting, turning it on for a few directories, in order to see just how many parameter removals it finds. (And I'm landing those removals as I do so.)

The first directories are the hardest, because they require the modification of many header files. Each additional directory is easier as more and more header files become compliant.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Add the MOZ_MAYBE_UNUSED attribute

It's necessary to use -Wunused-parameter usefully.

The underlying attribute is called "unused", but I chose MAYBE_MOZ_UNUSED
instead of MOZ_UNUSED for two reasons.

1. MOZ_UNUSED was already taken (see mfbt/unused.h).

2. MOZ_MAYBE_UNUSED is an arguably better name. You typically want to use it in
   cases where a variable is used in some configurations and not used in
   others; without the "MAYBE" it looks like you're claiming the variable is
   never used.

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8698308 [details] [diff] [review]
Add the MOZ_MAYBE_UNUSED attribute

Review of attachment 8698308 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the change below.

::: mfbt/Attributes.h
@@ +216,5 @@
> +// use.
> +//
> +// This attribute is most often used in situations where conditional
> +// compilation causes a variable to be used in some configurations but not in
> +// others, such as the following:

I think we should change this to MOZ_MAYBE_UNUSED_PARAMETER and change the documentation (and commit message) accordingly, so we don't have people attempting to use this for locals that they ought to be using DebugOnly for.

Comment 3

[Nicholas Nethercote [inactive]]

> I think we should change this to MOZ_MAYBE_UNUSED_PARAMETER and change the
> documentation (and commit message) accordingly, so we don't have people
> attempting to use this for locals that they ought to be using DebugOnly for.

I considered that, but (a) the extra length pushes it from "ok" into "annoying" territory, and (b) it  can be used reasonably for functions and (less likely) local variables and even types.

https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#Common-Variable-Attributes
https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes
https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html#Common-Type-Attributes

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Nicholas Nethercote [:njn] from comment #3)
> > I think we should change this to MOZ_MAYBE_UNUSED_PARAMETER and change the
> > documentation (and commit message) accordingly, so we don't have people
> > attempting to use this for locals that they ought to be using DebugOnly for.
> 
> I considered that, but (a) the extra length pushes it from "ok" into
> "annoying" territory, and (b) it  can be used reasonably for functions and
> (less likely) local variables and even types.

Well, "annoying" territory isn't necessarily a bad thing; this annotation shouldn't get used that much, right?

Functions are easier to #ifdef DEBUG out (which will cause them to get picked up by -Wunused-functions in debug builds if their caller ever goes away), local variables have DebugOnly, and types...well, I suspect types wouldn't get this treatment very often (if ever).

Do you have a sense of how often we would use this annotation?

Comment 5

[Chris Peterson [:cpeterson]]

MSVC has an optional warning (C4100) for unused parameters:

https://msdn.microsoft.com/en-us/library/26kb9fy0.aspx

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: Enable -Wunused-parameter for C++ code in layout/xul/

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: Enable -Wunused-parameter for C++ code in layout/base/

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: Enable -Wunused-parameter for C++ code in xpcom/base/

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: Enable -Wunused-parameter for C++ code in xpcom/build/

Comment 10

[Nicholas Nethercote [inactive]]

I'm bullish about -Wunused-parameter: I've now run it in earnest on
layout/xul/, layout/base/, xpcom/base/ and xpcom/build and it's found quite a
lot of removable parameters. (See the dependent bugs; note that there were no
unused parameters found in xpcom/build/.)

But the process is a bit tedious and I don't have the time to put in a large
effort.

cpeterson, is this something you'd be interested in pushing along?

If so, here's how I've been proceeding.

- Add -Wunused-parameter to CXXFLAGS in a moz.build of your choice.

- Inspect all the errors that arise. For each one, determine if the parameter
  can be removed.
  
  * Most of the time the parameter can't be removed because because the
    function is implementing an interface. (Determining this can be tedious,
    but you get a knack for it after a while.)
    
    In that case, just comment out the parameter name; I created a Vim macro to
    add the comment symbols, which saved a lot of keystrokes. Sometimes you
    have to use MOZ_MAYBE_UNUSED instead, e.g. if the parameter is used in some
    but not all configurations.

  * Sometimes you can remove the parameter. (And note that it's possible that
    all functions implementing an interface don't use a parameter.)
    
    In that case, don't remove it yet but mark it with a special comment.

- Once you've fixed all the compile errors, save your patch and then pop it
  from your patch stack. Then write a new patch that just removes the removable
  parameters. (Find them by searching for your special comment.)

- Then re-push the original patch. You'll get conflicts for the params you've
  removed; fix those up as necessary.

After doing that you end up with two patches: a first one that removes the
unused parameters, and a second one that enables -Wunused-parameter and adds
all the necessary comments/MOZ_MAYBE_UNUSED annotations. (If a lot of
parameters are removed, it might be worth splitting that first patch up into
multiple.)

I've attached the second patches for each of the four directories that I've
done. You can see they're quite large. They should get easier as you go, as
more of the header files get converted. If -Wunused-parameter ever gets
switched on you'll need to convince people that these changes are acceptable,
which should probably happen via a dev-platform discussion, and may be
challenging :)

One thing to note: commenting out unused parameter names is legal in C++, but
not legal in C. So you always have to use MOZ_MAYBE_UNUSED in C. I've only
modified CXXFLAGS so far, just because we have more C++ code.

Comment 11

[Chris Peterson [:cpeterson]]

I can take a look, but I don't know how soon. If any other interested parties are reading this bug, please feel free to work on this. :)

Comment 12

[Chris Peterson [:cpeterson]]

I see about 33,000 -Wunused-parameter warnings when building with clang on OS X. About 19,000 of these are in generated code for IPDL and protobufs, so we can fix all those by fixing the code generators. But there are still 14,000 other warnings in be fixed by hand.

In comparison, there are "only" about 4,500 -Wshadow warnings, which I think are more likely to find real bugs.

Comment 13

[Nicholas Nethercote [inactive]]

(In reply to Chris Peterson [:cpeterson] from comment #12)
> I see about 33,000 -Wunused-parameter warnings when building with clang on
> OS X. About 19,000 of these are in generated code for IPDL and protobufs, so
> we can fix all those by fixing the code generators. But there are still
> 14,000 other warnings in be fixed by hand.

A lot of the -Wunused-parameter warnings are in header files, and so there may be lots of duplicates. I imagine that's less likely to be true for -Wshadow.

You could try applying the patches I attached and see if that makes much difference.
 
> In comparison, there are "only" about 4,500 -Wshadow warnings, which I think
> are more likely to find real bugs.

It's true that addressomg -Wunused-parameter warnings is unlikely to find bugs. It's more just a code cleanliness/conciseness thing.

Comment 14

[jbonnafo]

Hello,

I would like to help on this Bug but don't really now where to start. Any idea?

I have tried to launch a compilation with -Wshadow and it generates quite a lot of warning messages indeed.

Thanks,

Comment 15

[Chris Peterson [:cpeterson]]

Hi Jean-Luc! Thanks for your interest in helping with this bug.

The first step, which you are already doing, is the build and run your own local version of Firefox:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build

The -Wunused-parameter warnings will likely be very noisy. I recommend compiling with -Wunused-parameter enabled and then fixing the warnings in one directory at a time. Open a new bug (that blocks this bug #) for each directory because different directories will have different code reviewers. Fixing one directory at a time will also allow you to land your fixes sooner instead of waiting to fix *all* of the warnings. :)

You can also check out "Codetribute" to find other bugs that might be smaller:

https://codetribute.mozilla.org/

Comment 16

[Chris Peterson [:cpeterson]]

(In reply to Chris Peterson [:cpeterson] from comment #15)
> I recommend compiling with -Wunused-parameter enabled and then fixing the warnings in one directory at a time.

I also recommend starting with a directory that has very few -Wunused-parameter warnings. That will allow you to complete a patch for code review sooner so you can get experience using Mozilla's code review system (Phabricator).

Comment 17

[Nicholas Nethercote [inactive]]

Repeating some points from comment 10:

> After doing that you end up with two patches: a first one that removes the
> unused parameters, and a second one that enables -Wunused-parameter and adds
> all the necessary comments/MOZ_MAYBE_UNUSED annotations. (If a lot of
> parameters are removed, it might be worth splitting that first patch up into
> multiple.)
> 
> I've attached the second patches for each of the four directories that I've
> done. You can see they're quite large. They should get easier as you go, as
> more of the header files get converted. If -Wunused-parameter ever gets
> switched on you'll need to convince people that these changes are acceptable,
> which should probably happen via a dev-platform discussion, and may be
> challenging :)

If you want to do this and just land the first patch for one or more directories, that is fine -- you'll just be removing unused arguments which is unambiguously good.

If you want to actually enable -Wunused-parameter by default and land the second patch for one or more directories, that may be controversial and merits a discussion on the dev-platform mailing list.

It's probably worth noting that Rust is aggressive about giving warnings/errors on unused parameters, so that's another piece of evidence that warning about this kind of thing is reasonable. (The fix in Rust is to add a leading '_' to the parameter name. A leading '_' doesn't work in C++ so you have to comment out or omit the parameter name instead.)

Comment 18

[jbonnafo]

Hello,

Thanks a lot for these detailed comments.

As a start, i have compiled with the CXXFLAG "-Wshadow" and have submitted a first bug report: Bug 1492570
This one is about function variable names shadowing class members. Fix looks simple but not sure the team in charge of that part of the code would accept the patch since there's no "real" bug, it is just about the compiler complaining that function variable names can be "misleading".
I'll try to do compilation per folder and submit bug for those warnings (-Wunused-parameter / -Wshadow).

What is the best way to list the warnings generated by a FF compilation? I am under the impression that "mach warnings-list" does not list them all.

Thanks for your help,

Comment 19

[Nicholas Nethercote [inactive]]

I just redirect them to a file and search for "warning: ". (If you use vim, you can use quickfix to quickly jump from one warning to the next.)

Comment 20

[jbonnafo]

Thanks, i will store the compilation results in a text file as you described.
The number of warnings generated looks quite big.

Should i try to submit a first patch attached to this bug in order to solve -Wunused-parameter on a given code directory or should i file one new bug for each directory?

Another question is the method to solve the warning: is MOZ_MAYBE_UNUSED directive the way to go?

Thanks again for your help,

Comment 21

[Nicholas Nethercote [inactive]]

> Should i try to submit a first patch attached to this bug in order to solve
> -Wunused-parameter on a given code directory or should i file one new bug
> for each directory?

I used separate bugs for the few directories that I did, so it's probably worth continuing like that. This bug can be the tracking bug -- mark any bugs you file as blocking this one.

> Another question is the method to solve the warning: is MOZ_MAYBE_UNUSED
> directive the way to go?

Comment 10 says what to do. In short:

- Remove the unused argument, if you can.

- Otherwise, comment out it's name, if you can.

- Otherwise, use MOZ_MAYBE_UNUSED, e.g. if the parameter is used in some but not all configurations. Note that MOZ_MAYBE_UNUSED is not in the repo, so you'll need to apply the first patch in this bug to be able to use it.

Comment 22

[Emilio Cobos Álvarez (:emilio)]

I think this is till worth doing. It would've caught bug 1719328 for example.

Comment 23

[Chris Peterson [:cpeterson]]

clang currently reports over 38,000 -Wunused-parameter warnings in mozilla-central. About half of them are in generated code for ipdl and DOM bindings.

Fixing all of them seems untenable, but some incremental options might be to opt-in (or opt-out) some individual directories or to add a Phabricator lint that just checks new code.