Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Please don't remove unused parameters from function definitions on Save #4655

Closed
lukehutch opened this issue Jul 21, 2023 · 10 comments
Closed
Labels
in editor Relates to code editing or language features in lsp/analysis server Something to be fixed in the Dart analysis server is bug relies on sdk changes Something that requires changes in the Dart/Flutter SDK to ship before it will become available
Milestone

Comments

@lukehutch
Copy link

lukehutch commented Jul 21, 2023

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):
@DanTup
Copy link
Member

DanTup commented Jul 22, 2023

@lukehutch

This is not good default behavior. (Or maybe I accidentally enabled it in my settings, but it's not good behavior in general.)

Have you enabled source.fixAll in editor.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) and canBeAppliedInBulk (used by "dart fix"), such as canBeAppliedAutomatically (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
Copy link
Author

Have you enabled source.fixAll in editor.codeActionsOnSave? (it definitely shouldn't be default behaviour right now*)

Yes, I have that enabled.

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) and canBeAppliedInBulk (used by "dart fix"), such as canBeAppliedAutomatically (which will be a slightly smaller subset considered save to run on "incomplete" code).

Sounds reasonable to me.

@DanTup DanTup added this to the v3.72.0 milestone Jul 27, 2023
@DanTup DanTup added in editor Relates to code editing or language features in lsp/analysis server Something to be fixed in the Dart analysis server labels Jul 27, 2023
@lukehutch
Copy link
Author

An observation: even with source.fixAll added to the options, unused local variables are not removed on save -- only unused params are removed.

@jacob314
Copy link

Adding canBeAppliedAutomatically to handle this case consistently with the unused imports case SGTM.

@bwilkerson
Copy link

I wasn't thrilled when we added canBeAppliedToFile because 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 of canBeAppliedInBulk was 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, but canBeAppliedToFile and canBeAppliedAutomatically are 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):

  • whether the fix is written to allow it to be applied at multiple locations, and
  • whether the file is likely in a stable state (such as when running 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 canBeAppliedOnSave to 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
Copy link
Member

DanTup commented Jul 31, 2023

it seems to me that there are two conditions that matter when deciding whether to apply a fix from a given affordance

  1. ...
  2. whether the file is likely in a stable state (such as when running dart fix) or whether the file might be in an incomplete state (such as fix-on-save).

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).

though I'd prefer to call it canBeAppliedOnSave to make it clear which affordance it's controlling

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
Copy link

I'm interpreting "incomplete" to mean code that is still being worked on ...

Correct. Thanks for the clarification.

I had thought about exposing both "groups" of these fixes via commands ...

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
Copy link
Author

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.

copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Aug 9, 2023
…-on-save

Fixes Dart-Code/Dart-Code#4655

Change-Id: I09269766124f7b77fde7c499c6f69a09989d1766
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/317684
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
@DanTup
Copy link
Member

DanTup commented 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).

@DanTup DanTup closed this as completed Aug 9, 2023
@lukehutch
Copy link
Author

Thank you @DanTup!

@DanTup DanTup added the relies on sdk changes Something that requires changes in the Dart/Flutter SDK to ship before it will become available label Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in editor Relates to code editing or language features in lsp/analysis server Something to be fixed in the Dart analysis server is bug relies on sdk changes Something that requires changes in the Dart/Flutter SDK to ship before it will become available
Projects
None yet
Development

No branches or pull requests

4 participants