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

avoid_redundant_argument_values lint rule false positives #88

Open
RobertOdrowaz opened this issue Dec 27, 2023 · 2 comments
Open

avoid_redundant_argument_values lint rule false positives #88

RobertOdrowaz opened this issue Dec 27, 2023 · 2 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@RobertOdrowaz
Copy link

Describe the issue
avoid_redundant_argument_values lint rule reports false positives when assigning null using copyWith method.

Environment details

Paste the flutter environment detail.

flutter doctor
[✓] Flutter (Channel stable, 3.16.5, on macOS 14.1.1 23B81 darwin-arm64, locale
    en-PL)
[✓] Android toolchain - develop for Android devices (Android SDK version
    32.1.0-rc1)
[✓] Xcode - develop for iOS and macOS (Xcode 15.0.1)
[✓] Chrome - develop for the web
[✓] Android Studio (version 2023.1)
[✓] VS Code (version 1.85.1)
[✓] Connected device (3 available)
    ! Error: Browsing on the local area network for Robert’s iPhone. Ensure the
      device is unlocked and attached with a cable or associated with the same
      local area network as this Mac.
      The device must be opted into Developer Mode to connect wirelessly. (code
      -27)
[✓] Network resources

• No issues found!

flutter --version
Flutter 3.16.5 • channel stable • https://github.com/flutter/flutter.git
Framework • revision 78666c8dc5 (8 days ago) • 2023-12-19 16:14:14 -0800
Engine • revision 3f3e560236
Tools • Dart 3.2.3 • DevTools 2.28.4

Paste the package version.

dependencies:
  copy_with_extension: 5.0.4
dev_dependencies:
  copy_with_extension_gen: 5.0.4

To Reproduce

Steps to reproduce the behaviour:

  1. Paste code below into a copy_with_test.dart file
import 'package:copy_with_extension/copy_with_extension.dart';
import 'package:flutter_test/flutter_test.dart';

part 'copy_with_test.g.dart';

@CopyWith()
class TestClass {
  const TestClass(this.field);

  final String? field;
}

void main() {
  test('sdf', () {
    const testClass = TestClass('non null value');

    final copiedTestClass = testClass.copyWith(field: null);

    expect(copiedTestClass.field, null);
  });
}
  1. Enable avoid_redundant_argument_values lint rule
  2. Chek infos
image

Expected behaviour

No info should be reported.

Additional context

I believe this is caused by copyWith getter being typed as _$TestClassCWProxy.call method in _$TestClassCWProxy is defined as

TestClass call({
  String? field,
});

which in fact has null default value for field. Linter doesn't take into account the actual implementation which has a different default value so false positive info is reported.

@shilangyu
Copy link
Contributor

shilangyu commented Jan 2, 2024

Freezed is able to get away with this, but I believe it is a coincidence. Here is a simplified view on the situation. Consider the following model for which we will generate a copyWith:

class Model {
  const Model({required this.field});

  final String? field;
}

Field is nullable so we should be able to copyWith with a null.

The generated code for this is roughly (removed the field copyWiths):

abstract class _$ModelCWProxy {
  Model call({
    String? field,
  });
}

class _$ModelCWProxyImpl implements _$ModelCWProxy {
  const _$ModelCWProxyImpl(this._value);

  final Model _value;

  @override
  Model call({
    Object? field = const $CopyWithPlaceholder(),
  }) {
    return Model(
      field: field == const $CopyWithPlaceholder()
          ? _value.field
          : field as String?,
    );
  }
}

extension $ModelCopyWith on Model {
  _$ModelCWProxy get copyWith => _$ModelCWProxyImpl(this);
}

Using const Model(field: null).copyWith(field: null) will raise a avoid_redundant_argument_values warning. False positive.

After looking what Freezed does (I am not sure if they are doing it on purpose because of this lint, or they just happen to be doing something that works here), we can alter slightly the definition of _$ModelCWProxy: make it generic over the return type of call. This results in identical typing, just expressed a different way:

abstract class _$ModelCWProxy<$Res> {
  $Res call({
    String? field,
  });
}

class _$ModelCWProxyImpl implements _$ModelCWProxy<Model> {
// as before...
}

extension $ModelCopyWith on Model {
  _$ModelCWProxy<Model> get copyWith => _$ModelCWProxyImpl(this);
}

However, now const Model(field: null).copyWith(field: null) raises no warnings. I have no idea why that is. I checked the implementation of this lint and found nothing that would indicate this behavior.

Now, in the general case it is impossible to prevent this false positive. The linter cannot know what is the underlying implementation (after all this is the point of abstraction). If copy_with implements this hack, it will remove the (very) annoying warning but it may come back any day.

@numen31337 numen31337 added the enhancement New feature or request label Nov 29, 2024
@numen31337
Copy link
Owner

For future reference, I’ve noticed that the warning disappears in the simplest use case when following the suggested fix from #88 (comment). However, I need to double-check this solution and ensure it doesn’t cause any side effects in more complex scenarios.

It’s concerning that the issue disappears for unknown reasons. It’s likely that it reaches the limits of introspection or something of this kind. Even removing the default value $CopyWithPlaceholder() from the implementation doesn’t make the warning reappear, which confirms that the issue is gone, not because the analyzer somehow sees the implementation but something else.

@numen31337 numen31337 added the help wanted Extra attention is needed label Nov 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants