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.
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?
--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.
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 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.jsinterfaceImpl(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
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.jsinterfaceImpl(arg1, arg2, something) {return arg1 + arg2;}
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.jsinterfaceImpl(arg1, arg2, something) {return arg1 + arg2;
}
How do you feel about doing this instead?
interfaceImpl(arg1, arg2 /*, something */) {
return arg1 + arg2;
}
This is starting to get esoteric, but what if this is a function on a base-class?
class Foo {
bar(arg1, arg2, something) {
// TODO: use something, recently added in a refactor.
return arg1 + arg2;
}
}
class Impl extends Foo {
bar(arg1, arg2, something) {
// use "something"
super.bar(arg1, arg2, something);
}
}
I guess for JS reasons Foo.bar can technically still comment it out, but that seems ugly and also possibly will trigger other linters complaining about the args mismatch. A leading underscore on the name seems like it would be consistent with practices from other languages, but I understand that might be tricky...
Mark