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

unused_element reports on unused parameters whose presence is idiomatic #49025

Closed
maxlapides opened this issue May 12, 2022 · 37 comments
Closed
Assignees
Labels
analyzer-warning Issues with the analyzer's Warning codes area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug

Comments

@maxlapides
Copy link

Assumptions:

  • Flutter 3.0
  • user_super_parameters lint rule is enabled
  • unused_element analyzer rule is enabled

This code chunk produces an analyzer issue:

class _MyWidget extends StatelessWidget {
  const _MyWidget({super.key});

  @override
  Widget build(BuildContext context) {
    return Container();
  }
}

"A value for optional parameter 'key' isn't ever given. Try removing the unused parameter."

But, when the same widget is not private, there is no issue:

class MyWidget extends StatelessWidget {
  const MyWidget({super.key});

  @override
  Widget build(BuildContext context) {
    return Container();
  }
}

I would like to keep the super.key on the private widgets, but I also want to keep the unused_element rule enabled. Is there any way to achieve this?

❯ flutter doctor -v
[✓] Flutter (Channel stable, 3.0.0, on macOS 12.3.1 21E258 darwin-arm, locale en-US)
    • Flutter version 3.0.0 at /Users/maxlapides/fvm/versions/3.0.0
    • Upstream repository https://github.com/flutter/flutter.git
    • Framework revision ee4e09cce0 (2 days ago), 2022-05-09 16:45:18 -0700
    • Engine revision d1b9a6938a
    • Dart version 2.17.0
    • DevTools version 2.12.2

[✓] Android toolchain - develop for Android devices (Android SDK version 31.0.0)
    • Android SDK at /Users/maxlapides/Library/Android/sdk
    • Platform android-31, build-tools 31.0.0
    • Java binary at: /Applications/Android Studio.app/Contents/jre/Contents/Home/bin/java
    • Java version OpenJDK Runtime Environment (build 11.0.11+0-b60-7772763)
    • All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS (Xcode 13.3.1)
    • Xcode at /Applications/Xcode.app/Contents/Developer
    • CocoaPods version 1.11.3

[✓] Chrome - develop for the web
    • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome

[✓] Android Studio (version 2021.1)
    • Android Studio at /Applications/Android Studio.app/Contents
    • Flutter plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/9212-flutter
    • Dart plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/6351-dart
    • Java version OpenJDK Runtime Environment (build 11.0.11+0-b60-7772763)

[✓] VS Code (version 1.67.1)
    • VS Code at /Applications/Visual Studio Code.app/Contents
    • Flutter extension version 3.40.0

[✓] Connected device (3 available)
    • iPhone 13 (mobile) • 8A362B17-9737-4B87-B0E4-77BF160D0A50 • ios            •
      com.apple.CoreSimulator.SimRuntime.iOS-15-4 (simulator)
    • macOS (desktop)    • macos                                • darwin-arm64   • macOS 12.3.1 21E258 darwin-arm
    • Chrome (web)       • chrome                               • web-javascript • Google Chrome 101.0.4951.64

[✓] HTTP Host Availability
    • All required HTTP hosts are available

• No issues found!
@bwilkerson
Copy link
Member

@srawlins

@srawlins
Copy link
Member

More or less a duplicate of #48401

@srawlins srawlins added the area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. label May 16, 2022
@srawlins srawlins transferred this issue from dart-lang/linter May 16, 2022
@srawlins
Copy link
Member

I would like to keep the super.key on the private widgets, but I also want to keep the unused_element rule enabled. Is there any way to achieve this?

No I don't think so. If you want to keep an unused parameter on your private widget, you will keep getting an unused_element report.

@srawlins
Copy link
Member

How does this relate to user_super_parameters? If you remove the named parameter (and pass null directly) do you get a user_super_parameters report?

@maxlapides
Copy link
Author

maxlapides commented May 16, 2022

@srawlins It's related to the super parameters because this breaks unused_element:

class _MyWidget extends StatelessWidget {
  const _MyWidget({super.key});

  @override
  Widget build(BuildContext context) {
    return Container();
  }
}

But this does not:

class _MyWidget extends StatelessWidget {
  const _MyWidget({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return Container();
  }
}

This doesn't really have much to do with the lint rule use_super_parameters, but rather just the new Dart 2.17 super parameters syntax.

@maxlapides maxlapides changed the title use_super_parameters and unused_element conflict in private Flutter widgets Dart 2.17 super parameters and unused_element conflict in private Flutter widgets May 16, 2022
@srawlins
Copy link
Member

Oops, that second example should also have an unused_element report, since no argument is ever given.

@maxlapides
Copy link
Author

It's super common in Flutter to have this key parameter on all widgets, regardless of whether the key parameter is ever used. It would be great to be able to disable unused_element just for the keys in Flutter.

@srawlins
Copy link
Member

I can update the title of this issue to make that the ask.

@maxlapides
Copy link
Author

@srawlins That would be great, thanks!

@srawlins srawlins changed the title Dart 2.17 super parameters and unused_element conflict in private Flutter widgets unused_element reports on unused parameters whose presence is idiomatic May 16, 2022
@srawlins srawlins added analyzer-warning Issues with the analyzer's Warning codes type-enhancement A request for a change that isn't a bug P4 labels May 16, 2022
@orevial
Copy link

orevial commented May 17, 2022

@srawlins @maxlapides this issue looks perfectly legit to me, why would you want to shut a warning about an unused element ?

Implementing super with key is legit on a public widget because you could want to export it as a library widget in which case you want to make sure you offer the possibility to give a key to anyone using your widget. However in the case of a private widget, if you are not using the key when calling your widget, why declare the key ?

To go further, I don't think that the new super parameters syntax has an issue, on the contrary I think that the old syntax should also trigger this unused_element warning on private widgets, to me that's the real issue 🙂

@goderbauer
Copy link
Contributor

I agree with the previous comment. There's no reason to have the key parameter on a private widget if nobody is using it. If you need to pass the key, you can always add it to the private widget since you're in full control of the code.

For a public widget (that may get exported from a library you don't have control over) you want to make sure it can always take a key. If the key parameter is omitted, you'd have to ask the library author to release a new version with the key parameter added.

@maxlapides
Copy link
Author

I'm not strongly opposed to removing the key parameters on the private widgets, but I do think there's a reasonable argument to be made that it is less work to have the super.key on all the private widgets because it's generated by a code snippet. Then, it's always available when you need it later, you don't need to go add it manually by editing a file that you may not have otherwise needed to touch.

@khobotin-elopage
Copy link

I agree with Max. It's common for widgets to have key parameter whether they are public or private.
And it seems there have been an exception for the key parameter in unused_element rule because this

const _MyWidget({Key? key}) : super(key: key);

looks legit for it.

Also it's more likely to forget to add the key whenever I want to make my private widget a public.

@srawlins
Copy link
Member

If there is not a report for unused_element for const _MyWidget({Key? key}) : super(key: key);, then I believe there is a bug in unused_element.

@orevial
Copy link

orevial commented May 19, 2022

I understand your point of view although I disagree :

  • IDE's snippets adding the key automatically when generating widgets is not a good reason IMO to implement an exception to this rule, the IDE snippet should follow language conventions and not the opposite
  • I understand the argument about the risk of forgetting to add key on public widgets but there is a linter rule specifically designed for this case : use_key_in_widget_constructors which will make sure you never forget a key on public widgets 😉

@srawlins
Copy link
Member

srawlins commented May 19, 2022

Generally speaking, the unused_* and unnecessary_* rules come from a policy of rejecting the "but I might need it later" argument in favor of highlighting all unused and unnecessary code. I think these rules will always report as much unused and unnecessary code as is technically correct.

If a dev or a team firmly believes some code should remain, however unused, they can disable the rules or suppress them by various means. If disabling "unused_element" across a codebase is too big a hammer, we may split a diagnostic out, as per #48401, for unused parameters, as this argument has come up a few times, "but I might need it later."

dmiedev added a commit to dmiedev/spacex that referenced this issue Jul 23, 2022
dmiedev added a commit to dmiedev/spacex that referenced this issue Jul 24, 2022
* feat: customize app theme

* feat: implement basic launch page

* feat: add launch state handling

* feat: implement launch card

* feat: add floating search bar to LaunchPage

* feat: add filtering chips

* feat: replace SliverList with SliverGrid

To support different screen sizes better.

* feat: add last launch page handling

* feat: add loading indicators to launch grid

* feat: add searching to LaunchPage

* fix: make search bar clear button work properly

* feat: add no items found indicator to grid of launch cards

* fix: ensure the binding is initialized before getting storage directory

* feat: add first page error indicator to launch grid

* feat: add new page error indicator to launch grid

* refactor: break up the launch page into different widgets

* feat: add filtering chip dialogs

* fix: make buttons in dialogs white

* feat: implement launch page sorting

* feat: add time filtering to launch page

* refactor: change launch state handling

* refactor: shorten launch bloc code

* feat: add launch successfulness filtering

* feat: add launch flight number filtering

* feat: add launch year filtering

* refactor: split up the launches feature into two features

* feat: add launch rocket filtering

* feat: add checkbox theme

* refactor: remove excessive event addition to launch bloc

* feat: create spacex_ui package with common widgets

* refactor: split up filtering chips file into multiple files

* refactor: use spacex_ui widgets in the rest of the app

* feat: add string localization

* docs: add missing project documentation

* refactor: remove key parameter from private widgets

Linter false positive: dart-lang/sdk#49025

* fix: fix FilteringChip layout

* refactor: obtain widget colors from Theme

* feat: change dialog text button color

* test: add basic app test
@kuyazee
Copy link

kuyazee commented Mar 27, 2024

@srawlins Strangely, I am getting this warning 'unused_element_parameter' isn't a recognized error code. dart(unrecognized_error_code):

analyzer:
  errors:
    unused_element_parameter: ignore

Works to me with:

analyzer:
  errors:
    unused_element: ignore

This worked for me on flutter version 3.19.4

@lore-co
Copy link

lore-co commented Aug 1, 2024

Any news?

@srawlins
Copy link
Member

The splitting CL is reverted; re-opening.

copybara-service bot pushed a commit that referenced this issue Aug 15, 2024
This reverts commit c1976b0.

Reason for revert: It seems this change caused failures on Flutter
HHH bot. Example log can be found at [0]:
```
| lib/src/super_reader/super_reader.dart:615:14: Error: Final field 'showDebugLeaderBounds' is not initialized.
| Try to initialize the field in the declaration or in every constructor.
|   final bool showDebugLeaderBounds;
|              ^^^^^^^^^^^^^^^^^^^^^

```
Looking at the sources it seems that may be related to the
`// ignore: unused_element` analyzer directive:

```
   602	/// A [SuperReaderDocumentLayerBuilder] that builds a [SelectionLeadersDocumentLayer], which positions
   603	/// leader widgets at the base and extent of the user's selection, so that other widgets
   604	/// can position themselves relative to the user's selection.
   605	class _SelectionLeadersDocumentLayerBuilder implements SuperReaderDocumentLayerBuilder {
   606	  const _SelectionLeadersDocumentLayerBuilder({
   607	    required this.links,
   608	    // ignore: unused_element
   609	    this.showDebugLeaderBounds = false,
   610	  });
   611
   612	  /// Collections of [LayerLink]s, which are given to leader widgets that are
   613	  /// positioned at the selection bounds, and around the full selection.
   614	  final SelectionLayerLinks links;
   615
   616	  /// Whether to paint colorful bounds around the leader widgets, for debugging purposes.
   617	  final bool showDebugLeaderBounds;
```

So tentatively reverting this CL.

[0] https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket/8739650477551803313/+/u/Run_customer_testing_tests/stdout

Original change's description:
> analyzer: separate unused_element_parameter from unused_element
>
> Fixes #49025
>
> This allows users to blanket ignore unused_element_parameter without
> ignoring unused_element. They are reported in distinct situations so it
> is valid to separate them.
>
> Change-Id: I4844a6a0e0a67cd5e37ed8735b1526e174deb950
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/378500
> Reviewed-by: Brian Wilkerson <[email protected]>
> Reviewed-by: Phil Quitslund <[email protected]>
> Commit-Queue: Samuel Rawlins <[email protected]>
> Reviewed-by: Ryan Macnak <[email protected]>

Change-Id: Ibbba75fe56601c7c4b5535c9142cf94c2dd80b91
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/380460
Reviewed-by: Christopher Fujino <[email protected]>
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/380689
Commit-Queue: Kevin Chisholm <[email protected]>
copybara-service bot pushed a commit that referenced this issue Aug 16, 2024
…d_element"

This reverts commit c1976b0.

Note: this is the second attempt at reverting this CL. The previous commit claimed to do the revert but did not actually revert the code.

Reason for revert: It seems this change caused failures on Flutter
HHH bot. Example log can be found at [0]:
```
| lib/src/super_reader/super_reader.dart:615:14: Error: Final field 'showDebugLeaderBounds' is not initialized.
| Try to initialize the field in the declaration or in every constructor.
|   final bool showDebugLeaderBounds;
|              ^^^^^^^^^^^^^^^^^^^^^

```
Looking at the sources it seems that may be related to the
`// ignore: unused_element` analyzer directive:

```
   602	/// A [SuperReaderDocumentLayerBuilder] that builds a [SelectionLeadersDocumentLayer], which positions
   603	/// leader widgets at the base and extent of the user's selection, so that other widgets
   604	/// can position themselves relative to the user's selection.
   605	class _SelectionLeadersDocumentLayerBuilder implements SuperReaderDocumentLayerBuilder {
   606	  const _SelectionLeadersDocumentLayerBuilder({
   607	    required this.links,
   608	    // ignore: unused_element
   609	    this.showDebugLeaderBounds = false,
   610	  });
   611
   612	  /// Collections of [LayerLink]s, which are given to leader widgets that are
   613	  /// positioned at the selection bounds, and around the full selection.
   614	  final SelectionLayerLinks links;
   615
   616	  /// Whether to paint colorful bounds around the leader widgets, for debugging purposes.
   617	  final bool showDebugLeaderBounds;
```

So tentatively reverting this CL.

[0] https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket/8739650477551803313/+/u/Run_customer_testing_tests/stdout

Original change's description:
> analyzer: separate unused_element_parameter from unused_element
>
> Fixes #49025
>
> This allows users to blanket ignore unused_element_parameter without
> ignoring unused_element. They are reported in distinct situations so it
> is valid to separate them.
>
> Tested: Presubmit CI
> Cq-Include-Trybots: luci.dart.try:flutter-analyze-try,analyzer-win-release-try,pkg-win-release-try
> Change-Id: I4844a6a0e0a67cd5e37ed8735b1526e174deb950
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/378500
> Reviewed-by: Brian Wilkerson <[email protected]>
> Reviewed-by: Phil Quitslund <[email protected]>
> Commit-Queue: Samuel Rawlins <[email protected]>
> Reviewed-by: Ryan Macnak <[email protected]>

Change-Id: I85c98a2d8b24d38d438015e7a10bd99ab914fc7a
Cq-Include-Trybots: luci.dart.try:analyzer-win-release-try,pkg-win-release-try
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/380460
Cherry-pick-request: #56486
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/380703
Reviewed-by: Slava Egorov <[email protected]>
Commit-Queue: Alexander Thomas <[email protected]>
@athomas
Copy link
Member

athomas commented Aug 16, 2024

3.6.0-149.0.dev includes the fix, in case someone wants to try it. However, it's still reverted on main so the next dev release will likely not contain the fix anymore.

srujzs added a commit to dart-lang/web that referenced this issue Aug 19, 2024
The latter was separated from the former to avoid conflation (see dart-lang/sdk#49025). Makes CI green again.
@srawlins
Copy link
Member

@iinozemtsev this is still reverted on main, right? re-open?

@iinozemtsev
Copy link
Member

oh, I didn't expect that pushing into a fork can close the issue, sorry!

@iinozemtsev iinozemtsev reopened this Aug 22, 2024
@srawlins
Copy link
Member

Oh, I see, haha! Thanks much!

copybara-service bot pushed a commit that referenced this issue Aug 27, 2024
…sed_element""

This reverts commit b551690.

Reason for revert: flutter customer tests failing: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8738601049545714785/+/u/run_test.dart_for_customer_testing_shard_and_subshard_None/stdout

Original change's description:
> Reapply "analyzer: separate unused_element_parameter from unused_element"
>
> Fixes #49025. Fixes #48401
>
> This allows users to blanket ignore unused_element_parameter without
> ignoring unused_element. They are reported in distinct situations so it
> is valid to separate them.
>
> This reverts commit b888da7.
>
> Change-Id: I8ea52fcdcb491c140c1283602d6911c939e78d50
> Tested: trybots
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/381882
> Reviewed-by: Ben Konyi <[email protected]>
> Commit-Queue: Samuel Rawlins <[email protected]>
> Reviewed-by: Brian Wilkerson <[email protected]>

Change-Id: Ie0df2f4be45e5db2fa255dcf8c30ddf8408c155b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/382420
Bot-Commit: Rubber Stamper <[email protected]>
Reviewed-by: Matan Lurey <[email protected]>
Commit-Queue: Samuel Rawlins <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
Reviewed-by: Ryan Macnak <[email protected]>
srawlins added a commit to srawlins/super_editor that referenced this issue Sep 9, 2024
In dart-lang/sdk#49025, we are splitting the `unused_element` code, so that the
warning reported is `unused_element_parameter`.

In this change, I change each ignored case to ignore each warning name, to make
the code forwards-compatible.
srawlins added a commit to flutter/tests that referenced this issue Sep 26, 2024
super_editor needs to be updated to handle an upcoming Dart change, dart-lang/sdk#49025, where unused parameters must be ignored with `// ignore: unused_element_parameter` instead of `// ignore: unused_element`.

A PR making this change has been unaddressed for 3 weeks. superlistapp/super_editor#2308

super_editor tests can be re-enabled when that PR is accepted, and the SHA here is bumped.
matthew-carroll pushed a commit to superlistapp/super_editor that referenced this issue Sep 26, 2024
…2308)

In dart-lang/sdk#49025, we are splitting the `unused_element` code, so that the
warning reported is `unused_element_parameter`.

In this change, I change each ignored case to ignore each warning name, to make
the code forwards-compatible.
auto-submit bot pushed a commit to flutter/tests that referenced this issue Sep 26, 2024
super_editor needs to be updated to handle an upcoming Dart change, dart-lang/sdk#49025, where unused parameters must be ignored with `// ignore: unused_element_parameter` instead of `// ignore: unused_element`.

A PR making this change has been unaddressed for 3 weeks. superlistapp/super_editor#2308

super_editor tests can be re-enabled when that PR is accepted, and the SHA here is bumped.
copybara-service bot pushed a commit that referenced this issue Oct 30, 2024
…sed_element""

This reverts commit b3f31a0.

Reason for revert: Customer tests are now disabled.

Original change's description:
> Revert "Reapply "analyzer: separate unused_element_parameter from unused_element""
>
> This reverts commit b551690.
>
> Reason for revert: flutter customer tests failing: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8738601049545714785/+/u/run_test.dart_for_customer_testing_shard_and_subshard_None/stdout
>
> Original change's description:
> > Reapply "analyzer: separate unused_element_parameter from unused_element"
> >
> > Fixes #49025. Fixes #48401
> >
> > This allows users to blanket ignore unused_element_parameter without
> > ignoring unused_element. They are reported in distinct situations so it
> > is valid to separate them.
> >
> > This reverts commit b888da7.
> >
> > Change-Id: I8ea52fcdcb491c140c1283602d6911c939e78d50
> > Tested: trybots
> > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/381882
> > Reviewed-by: Ben Konyi <[email protected]>
> > Commit-Queue: Samuel Rawlins <[email protected]>
> > Reviewed-by: Brian Wilkerson <[email protected]>
>
> Change-Id: Ie0df2f4be45e5db2fa255dcf8c30ddf8408c155b
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/382420
> Bot-Commit: Rubber Stamper <[email protected]>
> Reviewed-by: Matan Lurey <[email protected]>
> Commit-Queue: Samuel Rawlins <[email protected]>
> Reviewed-by: Brian Wilkerson <[email protected]>
> Reviewed-by: Ryan Macnak <[email protected]>

Change-Id: Icbe69690e83daecc5a0649d2f632ffc33ba6394c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/386722
Reviewed-by: Brian Wilkerson <[email protected]>
Reviewed-by: Matan Lurey <[email protected]>
Commit-Queue: Samuel Rawlins <[email protected]>
Reviewed-by: Ben Konyi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-warning Issues with the analyzer's Warning codes area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests