Repository navigation
Please don't remove unused parameters from function definitions on Save #4655
Description
Describe the bug
Create this class:
class _MyClass {
int? param;
_MyClass({
this.param,
});
}Now hit save. You get:
class _MyClass {
int? param;
_MyClass();
}this.param is removed automatically, because it is (as yet) unused.
This is not good default behavior. (Or maybe I accidentally enabled it in my settings, but it's not good behavior in general.)
Please complete the following information:
- Operating System and version: Linux
- VS Code version: code-1.80.1-1689183644.el7.x86_64
- Dart extension version:
- Dart/Flutter SDK version: 3.0.6
- Target device (if the issue relates to Flutter debugging):
Activity
DanTup commentedon Jul 22, 2023
@lukehutch
Have you enabled
source.fixAllineditor.codeActionsOnSave? (it definitely shouldn't be default behaviour right now*)@bwilkerson @jacob314 we currently only exclude "remove unused imports" when running code actions on-save. It seemed like the only paint point (and we wanted to minimise differences between running manually/automatically), but perhaps I didn't realise that some of these "dead code" removal fixes were flagged to run in bulk.
I wonder there should be another flag in addition to
canBeAppliedToFile(used to produce a fix-all-in-file code action that would be invoked manually) andcanBeAppliedInBulk(used by "dart fix"), such ascanBeAppliedAutomatically(which will be a slightly smaller subset considered save to run on "incomplete" code).(+ @srawlins - maybe also applicable to a fix you were describing recently?)
* yet... I think we'd like to apply some fixes automatically on save by default in future
lukehutch commentedon Jul 23, 2023
Yes, I have that enabled.
Sounds reasonable to me.
lukehutch commentedon Jul 27, 2023
An observation: even with
source.fixAlladded to the options, unused local variables are not removed on save -- only unused params are removed.jacob314 commentedon Jul 27, 2023
Adding
canBeAppliedAutomaticallyto handle this case consistently with the unused imports case SGTM.bwilkerson commentedon Jul 31, 2023
I wasn't thrilled when we added
canBeAppliedToFilebecause it was moving us in the direction of having a flag for each affordance from which a fix could potentially be applied, and that seems like the wrong way to control the product. (It can easily lead to arbitrary choices that are hard for the user to predict.) The original intent ofcanBeAppliedInBulkwas to capture whether or not the code was written in such a way as to make it safe to apply it repeatedly, which I think is essential information, butcanBeAppliedToFileandcanBeAppliedAutomaticallyare designed to answer the question of whether we want to use the fix from a given affordance, and I'd rather we answer the question of why some fixes are appropriate for one affordance or another.I've only thought about it briefly, so I'm probably missing something, but, for example, it seems to me that there are two conditions that matter when deciding whether to apply a fix from a given affordance (assuming that the affordance will attempt to apply the fix everywhere and doesn't given the user the option to explicitly select which fixes to run):
dart fix) or whether the file might be in an incomplete state (such as fix-on-save).I'm fine with adding the proposed flag temporarily because I believe that there's a problem that needs to be fixed (though I'd prefer to call it
canBeAppliedOnSaveto make it clear which affordance it's controlling), but longer term I'd like to explore using more basic properties of the affordance to control when fixes are applied.DanTup commentedon Jul 31, 2023
Yep, this seems right to me (I'm interpreting "incomplete" to mean code that is still being worked on and not necessarily in an invalid state). There are some kinds of "cleanup" you might want to do only at the end (eg. just before committing).
Seems reasonable to me. I used Automatically because I thought it was a little less specific in case in future we had other ways of prompting to fix (does anyone want an annoying paperclip while they're typing? 😁)
I'm curious if you have ideas for how this might eventually work though. I had thought about exposing both "groups" of these fixes via commands (eg. a "Fix All" and a "Fix All inc. dead code removal" and you could trigger either on-save), although that still requires defining these "groups" (either in the server, or by exposing some configuration). I'm not sure if that's the kind of thing you had in mind, or something else.
bwilkerson commentedon Jul 31, 2023
Correct. Thanks for the clarification.
It's likely that users will want both affordances in the IDE, but I haven't thought much about the UX associated with them. But yes, I would consider each of those commands to be a separate affordance.
lukehutch commentedon Jul 31, 2023
Whatever the configuration options that are provided, I don't think unused code should ever be removed automatically. Unused code can be removed manually by quick-fix actions (Ctrl+.), sure. But not on save.
[analysis_server] Don't remove unused parameters when running fix-all…
DanTup commentedon Aug 9, 2023
With dart-lang/sdk@6229694, removing unused parameters will only happen if you manually run the Fix All command and not when run on-save.
The change ships in the SDK so will show up in a future SDK update (not a Dart-Code update).
lukehutch commentedon Aug 10, 2023
Thank you @DanTup!