dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.23k stars 1.58k forks source link

unnecessary_set_literal fails to catch all cases #56373

Open shilangyu opened 3 months ago

shilangyu commented 3 months ago

Consider the following snippet:

void f() {
  g(p: () => {1}); // NO LINT
  h(() => {1}); // LINT
  final void Function() j = () => {1}; // NO LINT
}

void g({required void Function() p}) {}
void h(void Function() p) {}

The unnecessary_set_literal lint only works when the function expression is passed to a function as a position argument. I see no reason why the other two examples should not be linted as well.

To underline the importance of this lint working with named arguments consider the very common onPressed named argument in flutter widgets. This is a common place where this accidental set literal problem can arise. For example when adding an onPressed that will just mutate some state:

MyWidget(
    onPressed: () => {
        setState(() {
            state = newState;
        })
    }
    // ...
)

Tested on Dart SDK 3.6.0-88.0.dev

dart-github-bot commented 3 months ago

Summary: The unnecessary_set_literal lint incorrectly fails to detect unnecessary set literals when passed as named arguments or assigned to variables. This leads to potential errors, especially in common Flutter scenarios like onPressed callbacks.

shilangyu commented 2 months ago

The problem stems from this line: https://github.com/dart-lang/sdk/blob/2cf3222e4f1c00c36e1b072c1de8db31dad01de7/pkg/analyzer/lib/src/error/best_practices_verifier.dart#L1288

Changing it to final parameterType = node.staticType; fixes the issue. But actually it seems to be a bug that the current version does not work with named arguments. For those node.staticParameterElement is null, which doesn't seem like it should be the happening.