Remove unused local variable can inadvertently remove code with side effects

[lukehutch]

Given the following code:

  void removeUserId(int userId) {
    final removed = profileUserIds.remove(userId);
  }

the analyzer gives a fix suggestion of Remove unused local variable. Unfortunately the result of running this action is:

  void removeUserId(int userId) {
  }

Since VS Code runs this action on save, it's a good way to lose code that has side effects, if you habitually hit Ctrl+S all the time like I do.

I would expect the result to be the following, assuming the analyzer can't actually tell if the function call has any side effects or not (and the fix should probably be renamed Remove unused local variable declaration in this case):

  void removeUserId(int userId) {
    profileUserIds.remove(userId);
  }

Dart info:

$ dart info

If providing this information as part of reporting a bug, please review the information
below to ensure it only contains things you're comfortable posting publicly.

#### General info

- Dart 3.2.0-210.2.beta (beta) (Mon Oct 9 18:32:04 2023 +0000) on "linux_x64"
- on linux / Linux 6.5.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Fri Oct  6 19:02:35 UTC 2023
- locale is en_US.utf8

#### Project info

- sdk constraint: '>=3.0.0 <4.0.0'
- dependencies: auto_size_text, boxy, cached_network_image, carousel_slider, collection, cross_file, cupertino_icons, device_info_plus, email_validator, exif, extended_image, flutter, flutter_cache_manager, flutter_email_sender, flutter_fgbg, flutter_image_utilities, flutter_localizations, flutter_persistent_value_notifier, flutter_reactive_value, flutter_svg, fluttertoast, geocoding, geolocator, get_it, getwidget, google_fonts, google_mlkit_face_detection, http, image_picker, image_picker_android, infinite_scroll_pagination, intl, jiffy, line_awesome_flutter, logging, material_dialogs, package_info_plus, path_provider, persistent_bottom_nav_bar_v2, provider, random_string, routemaster, share_plus, url_launcher
- dev_dependencies: flutter_launcher_icons, flutter_lints, flutter_test
- elided dependencies: 11

#### Process info

|  Memory |  CPU | Elapsed time | Command line                                                                               |
| ------: | ---: | -----------: | ------------------------------------------------------------------------------------------ |
|  475 MB | 0.3% |     01:29:57 | dart --enable-vm-service=0 --pause_isolates_on_start --disable-dart-dev -DSILENT_VM_SERVICE=true --write-service-info=file:<path>/vm.json --pause_isolates_on_exit --enable-asserts <path>/main.dart |
|  644 MB | 0.5% |     01:32:20 | dart ..<path>/serverpod_cli.dart generate --watch                                          |
|   75 MB | 0.0% |     01:29:57 | dart debug_adapter                                                                         |
|   58 MB | 0.0% |   2-09:16:13 | dart devtools --machine --try-ports 10 --allow-embedding                                   |
|   62 MB | 0.0% |     04:47:36 | dart devtools --machine --try-ports 10 --allow-embedding                                   |
| 1013 MB | 0.2% |   3-00:43:13 | dart language-server --protocol=lsp --client-id=VS-Code --client-version=3.75.20231009     |
|  931 MB | 0.0% |   2-09:16:13 | dart language-server --protocol=lsp --client-id=VS-Code --client-version=3.75.20231009     |
| 1269 MB | 2.1% |     04:47:36 | dart language-server --protocol=lsp --client-id=VS-Code --client-version=3.75.20231009     |
|   92 MB | 0.0% |   2-09:16:13 | flutter_tools.snapshot daemon                                                              |
|   86 MB | 0.1% |     04:47:36 | flutter_tools.snapshot daemon                                                              |
|   95 MB | 0.1% |     01:20:57 | flutter_tools.snapshot debug_adapter                                                       |
| 1242 MB | 0.2% |     01:20:57 | flutter_tools.snapshot run --machine --start-paused -d emulator-5554 --devtools-server-address http:<path>/ --target <path>/main.dart |
|  728 MB | 0.2% |     01:20:55 | frontend_server.dart.snapshot --sdk-root <path>/ --incremental --target=flutter --experimental-emit-debug-metadata -DFLUTTER_WEB_AUTO_DETECT=true -DFLUTTER_WEB_CANVASKIT_URL=https:<path>/ --output-dill <path>/app.dill --packages <path>/package_config.json -Ddart.vm.profile=false -Ddart.vm.product=false --enable-asserts --track-widget-creation --filesystem-scheme org-dartlang-root --initialize-from-dill build/61ebfca94496893cf79d3453ae0a6715.cache.dill.track.dill --source file:<path>/dart_plugin_registrant.dart --source package:flutter<path>/dart_plugin_registrant.dart -Dflutter.dart_plugin_registrant=file:<path>/dart_plugin_registrant.dart --verbosity=error --enable-experiment=alternative-invalidation-strategy |

[srawlins]

We have an 'unnecessary_statements' lint, which could perhaps inform whether to remove the right side as well.

Even if that isn't great, or needs design work, I think we should err on safety and just remove the assignment, not the right side.

Not sure what we'd do with multiple variables in a declaration. Or assignment patterns. Something like:

void f() {
  var x = 1, y = 2;
  print(x);
}

We could change this to:

void f() {
  var x = 1;
  2; // unless we can declare this is an unnecessary statement.
  print(x);
}

🤷