copy_with_extension icon indicating copy to clipboard operation
copy_with_extension copied to clipboard

`avoid_redundant_argument_values ` lint rule false positives

Open RobertOdrowaz opened this issue 1 year ago • 1 comments

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.

RobertOdrowaz avatar Dec 27 '23 19:12 RobertOdrowaz

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.

shilangyu avatar Jan 02 '24 20:01 shilangyu