Turning on linting for unused JavaScript function arguments

[Dave Townsend]

We’re going to start throwing lint errors for unused function arguments. This matches our existing behaviour of throwing errors for unused variables within functions. Specifically we will be setting the arguments option for eslint’s no-unused-vars rule to after-used.

Unused arguments can be simply leftover after previous refactoring or can be a sign of bugs where a function is expected to do something with the argument and doesn’t. Either is a problem as it makes the function signature misleading.

This lint error won’t be automatically fixable. Automatically fixing is problematic during development since if you have your IDE configured to auto-fix whenever saving it removes arguments from functions you are in the middle of writing. Removing the argument is not always the correct solution either, it is better for the developer to choose the best solution.

However we will be automatically fixing the over 9,000 existing occurrences in the tree before turning the rule on. While it is possible we may miss real bugs by doing this, those bugs are already present and it is simply not feasible to turn this rule on otherwise.

What we cannot do is warn about or fix callers that are passing too many arguments to functions. That would require more advanced JavaScript tooling than we have available in Mozilla code currently.

This work will be tracked in bug 1864896.

[Marco Bonardo]

Apart from the possibility of catching bugs, are there performance advantages in doing this?

I sometimes specify arguments even if they are not used, especially when refactoring code and adding new arguments that avoid the consumer fetching things in more exotic ways. Unless I update all consumers with the new argument, people are used to the previous signature and may not notice the change, then new exotic fetches are added.

It's surely something that the review process should catch, but it's not a given.

In the bug Gijs suggested supporting a _ prefix to keep arguments, will that be implemented?

Are the 9000 cases covering all the cases of unused arguments, before and after used arguments? What will it replace unused arguments with, if they are before used ones?

Thank you.

--
You received this message because you are subscribed to the Google Groups "firef...@mozilla.org" group.
To unsubscribe from this group and stop receiving emails from it, send an email to firefox-dev...@mozilla.org.
To view this discussion on the web visit https://groups.google.com/a/mozilla.org/d/msgid/firefox-dev/CAPMxTNrS%2BnhxhGuR7sxm6-_Ve_ofS%3D1GZvp_fmPGY3K7u6wcWQ%40mail.gmail.com.

[Dave Townsend]

On Wed, 29 Nov 2023 at 13:00, Marco Bonardo <mbon...@mozilla.com> wrote:

Apart from the possibility of catching bugs, are there performance advantages in doing this?

Not that I'm aware of. There will be a small reduction in the physical size of the files of course but probably nothing that really impacts anything.

 

I sometimes specify arguments even if they are not used, especially when refactoring code and adding new arguments that avoid the consumer fetching things in more exotic ways. Unless I update all consumers with the new argument, people are used to the previous signature and may not notice the change, then new exotic fetches are added.

It's surely something that the review process should catch, but it's not a given.

Can you explain this further, I'm not sure I understand what you're saying.

 

In the bug Gijs suggested supporting a _ prefix to keep arguments, will that be implemented?

We did not plan to, do you think we should?

 

Are the 9000 cases covering all the cases of unused arguments, before and after used arguments? What will it replace unused arguments with, if they are before used ones?

Those are all cases of unused arguments after any used arguments. Unused arguments before used arguments won't be touched.

On Wed, Nov 29, 2023 at 12:15 PM Dave Townsend <dtow...@mozilla.com> wrote:

We’re going to start throwing lint errors for unused function arguments. This matches our existing behaviour of throwing errors for unused variables within functions. Specifically we will be setting the arguments option for eslint’s no-unused-vars rule to after-used.

Unused arguments can be simply leftover after previous refactoring or can be a sign of bugs where a function is expected to do something with the argument and doesn’t. Either is a problem as it makes the function signature misleading.

This lint error won’t be automatically fixable. Automatically fixing is problematic during development since if you have your IDE configured to auto-fix whenever saving it removes arguments from functions you are in the middle of writing. Removing the argument is not always the correct solution either, it is better for the developer to choose the best solution.

However we will be automatically fixing the over 9,000 existing occurrences in the tree before turning the rule on. While it is possible we may miss real bugs by doing this, those bugs are already present and it is simply not feasible to turn this rule on otherwise.

What we cannot do is warn about or fix callers that are passing too many arguments to functions. That would require more advanced JavaScript tooling than we have available in Mozilla code currently.

This work will be tracked in bug 1864896.

--
You received this message because you are subscribed to the Google Groups "firef...@mozilla.org" group.
To unsubscribe from this group and stop receiving emails from it, send an email to firefox-dev...@mozilla.org.
To view this discussion on the web visit https://groups.google.com/a/mozilla.org/d/msgid/firefox-dev/CAPMxTNrS%2BnhxhGuR7sxm6-_Ve_ofS%3D1GZvp_fmPGY3K7u6wcWQ%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "firef...@mozilla.org" group.
To unsubscribe from this group and stop receiving emails from it, send an email to firefox-dev...@mozilla.org.

To view this discussion on the web visit https://groups.google.com/a/mozilla.org/d/msgid/firefox-dev/CAPDqYT2HO8rC7%3DyRXvqamc1RGndaxMrNRmcZJu%2B-_pkO%2Bek%2BbA%40mail.gmail.com.

[Marco Bonardo]

On Wed, Nov 29, 2023 at 2:11 PM Dave Townsend <dtow...@mozilla.com> wrote:

I sometimes specify arguments even if they are not used, especially when refactoring code and adding new arguments that avoid the consumer fetching things in more exotic ways. Unless I update all consumers with the new argument, people are used to the previous signature and may not notice the change, then new exotic fetches are added.

It's surely something that the review process should catch, but it's not a given.

Can you explain this further, I'm not sure I understand what you're saying.

Before refactoring:

=> file1.js

interfaceImpl(arg1, arg2) {

  let something = exotic_way_to_get_something();

  let other = something.do();

  return arg1 + arg2 + other;

}

=> file2.js

interfaceImpl(arg1, arg2) {

  return arg1 + arg2;

}

After refactoring, with the new no-unused-args:

=> file1.js

interfaceImpl(arg1, arg2, something) {

  let other = something.do();

  return arg1 + arg2 + other;

} 

=> file2.js

interfaceImpl(arg1, arg2) {

  return arg1 + arg2;

}

Now DeveloperB needs `something` in file2, they know the Interface, but they don't know `something` is now available as argument, so they end up writing:

=> file.js

interfaceImpl(arg1, arg2) {

  let something = exotic_way_to_get_something();

  let other = something.do();

  return arg1 + arg2 + other;

} 

In this case I prefer to add the new unused argument to all the interfaceImpl instances during the refactoring, so when DeveloperB arrives, they will find:

=> file.js

interfaceImpl(arg1, arg2, something) {

  return arg1 + arg2;

}

that makes immediately obvious `something` can now be used directly.

I admit this may be considered an edge case, and most people are unlikely to do it (for which it's a good idea to check the interface definition every time), but I found it handy.

In all honesty, I was mostly wondering if unused arguments come with a performance cost.

I don't have too strong feelings overall, as while I'd appreciate a way out (like the _ prefix), I also see how it could be easily abused.

[Mark Banner]

On 29/11/2023 14:37, Marco Bonardo wrote:

In this case I prefer to add the new unused argument to all the interfaceImpl instances during the refactoring, so when DeveloperB arrives, they will find:

=> file.js

interfaceImpl(arg1, arg2, something) {

  return arg1 + arg2;

}

that makes immediately obvious `something` can now be used directly.

I admit this may be considered an edge case, and most people are unlikely to do it (for which it's a good idea to check the interface definition every time), but I found it handy.

In all honesty, I was mostly wondering if unused arguments come with a performance cost.

I don't have too strong feelings overall, as while I'd appreciate a way out (like the _ prefix), I also see how it could be easily abused.

I think there are two "styles" of function definitions - the observer/interface functions vs the more general function.

The biggest benefits here would be for the latter type. Highlighting arguments which may now be unused will remind developers to remove those and cleaning up the call tree - sometimes we could be doing now unnecessary work to pass the unused arguments, or it might signify that the architecture needs reworking.

Unfortunately ESLint can't distinguish between what's an observer/interface implementation and what isn't. The underscore prefix might be a way for observers/interface functions to still keep the argument names. We could potentially have a code convention that it only gets used for those style functions. Though I do think that's going to be difficult to handle on the initial roll-out as it would require a lot of manual intervention.

Mark

[Dave Townsend]

Thanks. I think the example you're giving is certainly a reasonable reason to be able to still give unused arguments so we'll configure the rule so that arguments prefixed with an underscore (`_`) are allowed. However we're still going to remove all the existing unused arguments for the roll-out as we have no reliable way of telling which is which and believe that in general removing the arguments is a better cleanup than applying a prefix to everything.

[Daniel Veditz]

On Wed, Nov 29, 2023 at 6:38 AM Marco Bonardo <mbon...@mozilla.com> wrote:

In this case I prefer to add the new unused argument to all the interfaceImpl instances during the refactoring, so when DeveloperB arrives, they will find:

=> file.js

interfaceImpl(arg1, arg2, something) {

  return arg1 + arg2;

}

How do you feel about doing this instead?

interfaceImpl(arg1, arg2 /*, something */) {

  return arg1 + arg2;

}

 

I'd probably add a comment explaining why I didn't need 'something', but that's a side issue.

-Dan Veditz

[Dave Townsend]

This change has now (finally!) landed. Thanks to everyone who was involved in a fairly epic amount of reviews.

Just to add, we chose to turn on errors for unused function arguments, unless the argument is prefixed with `_`, so if it makes sense to keep an argument for describing a functions signature then that is the preferred way to do it rather than disabling the rule entirely.