Flaky avoid_redundant_argument_values lint

[goderbauer]

This is a little strange and doesn't reproduce consistently, but I'll try to provide as much details as I have. In IntelliJ I have a workspace that has a bunch of projects open (including the flutter repository). In this one, I also have a tiny playground project with two files:

foo.dart:

class Foo {
  Foo({required this.foo});

  Bar? foo;
}

class Bar {

}

main.dart:

// @dart = 2.9

import 'foo.dart';

void main() {
  Foo(foo: null); // 🔥 sometimes this line has an incorrect avoid_redundant_argument_values
}

Now, sometimes I see what appears to be an incorrect avoid_redundant_argument_values warning on the line with the 🔥. The warning doesn't show up consistently. Sometimes it doesn't show up at all, sometimes it shows up for 5s to 10s and then disappears. Sometimes it sticks around indefinitely until I edit the code again. It's a bit of a mystery.

Here are random changes I have made to the files to make the warning appear/disappear (I believe the warning is incorrect in all cases):

• change the type of the foo field to int?
• change it back to Bar?
• remove the @dart directive from main.dart
• put the @dart directive back in
• add empty new lines at the end of the main.dart file

[goderbauer]

@srawlins

[goderbauer]

This whole investigation was triggered because in flutter we removed a bunch of // ignore: avoid_redundant_argument_values warnings that didn't appear to ignore anything and the PR was analyzer clean locally and on CI [1]. After the PR was submitted, Continued to stay green and then about 5 commits later, CI started to complain about the lines where we previously removed the //ignore avoid_redundant_argument_values. Interestingly, CI was still clean for the simple flutter analyze --flutter-repo case that just analyzes the entire repository. However, our analyzer benchmark (that copies the gallery a bunch of times and then analyzes the repository with all those copies in it) started failing as described above. We ended up putting these ignores back in, even though they shouldn't be needed [3].

[1] flutter/flutter#108924
[2] https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8806782704607467489/+/u/run_analyzer_benchmark/test_stdout
[3] flutter/flutter#108984

[srawlins]

@scheglov I sat with @goderbauer as he reproduced the flakiness in the IDE. I think this is a really good repro (at least on his machine with a large workspace open in IntelliJ).

The diagnostic would remain in the Diagnostics panel for a while (~a minute; seemingly stable) while we kept foo.dart in focus. If we moved over to main.dart, and made a trivial whitespace change, then 5 seconds later the diagnostic would disappear.

[scheglov]

Interesting. When I restart DAS, there is never any hint on Foo(foo: null). When I switch to Foo declaration, and switch back there is always the hint on Foo(foo: null). It seems that resolving the file with Foo changes the state of the element model for Foo.

What should happen here? The parameter is nullable, so passing null is not necessary. But the parameter is also required, so we have to provide something, e.g. null. So, we probably should not get the hint.

[goderbauer]

What should happen here? The parameter is nullable, so passing null is not necessary. But the parameter is also required, so we have to provide something, e.g. null. So, we probably should not get the hint.

I think there should be no hint here.

[christopherfujino]

What should happen here? The parameter is nullable, so passing null is not necessary. But the parameter is also required, so we have to provide something, e.g. null. So, we probably should not get the hint.

I think there should be no hint here.

not getting the hint would be great

[srawlins]

If I had a choice between correcting the hint and correcting the flakiness, I would choose the latter 😛

[scheglov]

Well, now I got it to the state when we always report the hint :-D

[scheglov]

The main issue is that when looking from a legacy library, we consider required this.foo as optional, as requested in #43397. So, the linter does not exit on !param.isOptional. The flakiness is because with element model we compute values only for constant.isOptional parameters. But this time, we are looking at the declaration of foo, which is required, so not optional. Apparently resolving the unit still compute its value somehow. We can "unflake" it by computing default values always.

So, I'm not sure how to make that it does not report the lint.

[scheglov]

A solution could be to update the lint rule so that it checks that the declaration of the parameter it required.

      if (param.declaration.isRequired || param.hasRequired) {
        continue;
      }

[scheglov]

@pq Not sure about the semantics here.

[scheglov]

https://dart-review.googlesource.com/c/sdk/+/253705 will update the analyzer to evaluate default values, so unflake it.

[pq]

A solution could be to update the lint rule so that it checks that the declaration of the parameter it required.

Is there a test case I can use to reproduce this in our reflective tests? Maybe once a deflaked analyzer is published?

[scheglov]

Here is what I used locally.

@reflectiveTest
class ZZZTest extends PubPackageResolutionTest {
  solo_test_XXX() async {
    writeTestPackageAnalysisOptionsFile(
      AnalysisOptionsFileConfig(
        lints: ['avoid_redundant_argument_values'],
      ),
    );

    final a = newFile('$testPackageLibPath/a.dart', r'''
class Foo {
  int? foo;
  Foo({required this.foo});
}
''');

    final b = newFile('$testPackageLibPath/b.dart', r'''
// @dart = 2.9
import 'a.dart';

void f() {
  Foo(foo: null);
}
''');

    // Uncomment to get the lint reported.
    // await resolveFile2(a.path);

    await resolveFile2(b.path);
    assertNoErrorsInResult(); // does not matter, just to see if there are or not diagnostics
  }
}

[scheglov]

FYI, https://dart-review.googlesource.com/c/sdk/+/253705 landed, and breaks Flutter at least once.
Looks like exactly the issue with the lint that this issue is about.

Analyzing 3 items...                                            

   info • Avoid redundant argument values • dev/integration_tests/android_semantics_testing/lib/src/tests/controls_page.dart:71:28 • avoid_redundant_argument_values

1 issue found. (ran in 170.3s)

[goderbauer]

It's complaining about this line here:

https://github.com/flutter/flutter/blob/49fbf19590fd206af1a78cde93ffba9eeb6bcc0f/dev/integration_tests/android_semantics_testing/lib/src/tests/controls_page.dart#L71

But onChanged is a required argument, removing it triggers the missing_required_param warning. So the lint there again is bogus and shouldn't really be there.

[scheglov]

True, as I described above, my change it to make it not flake.
Which means that the lint rule now always reports the lint.
We still need a fix (again, outlined above) to the lint rule.
For now the best I can think out is to // ignore it.