freezed icon indicating copy to clipboard operation
freezed copied to clipboard

copyWith may not work correctly when using null

Open b4tchkn opened this issue 1 year ago • 12 comments

Describe the bug It was working fine when I was using 2.2.1, but when I update to 2.3.0 and higher it does not work correctly.

To Reproduce

Below is a simple model for checking behavior.

import 'package:freezed_annotation/freezed_annotation.dart';

part 'sample_model.freezed.dart';

@freezed
class SampleModel<T extends Object> with _$SampleModel<T> {
  const factory SampleModel({
    T? value,
  }) = _SampleModel<T>;
}

Here is the test code. Test passes in 2.2.1, fails in 2.3.0 and higher.

import 'package:flutter_playground_app/sample_model.dart';
import 'package:flutter_test/flutter_test.dart';

void main() {
  test('sample model test', () {
    const sampleModel = SampleModel(value: 9);
    expect(sampleModel, const SampleModel(value: 9));

    final updatedSampleModel = sampleModel.copyWith(value: null);
    expect(updatedSampleModel, const SampleModel<int>(value: null));
  });
}

The following differences were found in the code automatically generated by freezed.


~~~

/// @nodoc
class _$SampleModelCopyWithImpl<T extends Object, $Res,
    $Val extends SampleModel<T>> implements $SampleModelCopyWith<T, $Res> {
  _$SampleModelCopyWithImpl(this._value, this._then);

  // ignore: unused_field
  final $Val _value;
  // ignore: unused_field
  final $Res Function($Val) _then;

  @pragma('vm:prefer-inline')
  @override
  $Res call({
    Object? value = null, // here! `Object? value = freezed` in 2.2.1.
  }) {
    return _then(_value.copyWith(
      value: null == value // here! `freezed == value` in 2.2.1.
          ? _value.value
          : value // ignore: cast_nullable_to_non_nullable
              as T?,
    ) as $Val);
  }
}

~~~

/// @nodoc
class __$$_SampleModelCopyWithImpl<T extends Object, $Res>
    extends _$SampleModelCopyWithImpl<T, $Res, _$_SampleModel<T>>
    implements _$$_SampleModelCopyWith<T, $Res> {
  __$$_SampleModelCopyWithImpl(
      _$_SampleModel<T> _value, $Res Function(_$_SampleModel<T>) _then)
      : super(_value, _then);

  @pragma('vm:prefer-inline')
  @override
  $Res call({
    Object? value = null, // here! `Object? value = freezed` in 2.2.1.
  }) {
    return _then(_$_SampleModel<T>(
      value: null == value // here! `freezed == value` in 2.2.1.
          ? _value.value
          : value // ignore: cast_nullable_to_non_nullable
              as T?,
    ));
  }
}

/// @nodoc

class _$_SampleModel<T extends Object> implements _SampleModel<T> {
  const _$_SampleModel({this.value});

~~~

Expected behavior

The above test code must pass.

b4tchkn avatar May 12 '23 07:05 b4tchkn

To me, the behavior in 2.3.0+ is correct, and the previous behavior was a bug.

Since there is no way for the code distinguish between a passed null value and an omitted argument, null has to mean that you don't want to update the field. Otherwise you would always have to be really careful to pass values for all nullable fields every time you call copyWith.

This is a known limitation. If you want to unset a field you have to manually create a new instance. Alternatively, if you find that too inconvenient, can always wrap the nullable value in something like an Optional

tjarvstrand avatar May 23 '23 07:05 tjarvstrand

@tjarvstrand That is incorrect. Freezed is correctly able to determine if the parameter is passed or not.

This is indeed a bug.

rrousselGit avatar May 23 '23 08:05 rrousselGit

Ah, TIL. Sorry about that!

tjarvstrand avatar May 23 '23 09:05 tjarvstrand

Any update on this?

iandis avatar Aug 14 '23 12:08 iandis

Nope. I'm on other things If it's critical for you, I'm happy to review a PR :)

Otherwise it'll wait a bit

rrousselGit avatar Aug 16 '23 07:08 rrousselGit

I see. It using Freezed() as default

myxzlpltk avatar Sep 04 '23 00:09 myxzlpltk

Agree. The default value of properties when calling .copyWith() are Freezed object instead of null. So the behavior for checking field is passed or not, is correct as expected. (current version of freezed: 2.4.6)

erlangparasu avatar Dec 20 '23 00:12 erlangparasu

Any update on this? Our app has not been able to update this because of this bug. I think this change has affected. https://github.com/rrousselGit/freezed/pull/808

Is there any solution?

b4tchkn avatar Feb 19 '24 04:02 b4tchkn

@b4tchkn which version is currently used in your app?

erlangparasu avatar Feb 19 '24 07:02 erlangparasu

We are still using 2.2.1.

b4tchkn avatar Feb 20 '24 12:02 b4tchkn

How do I modify the To Reproduce test code listed in the Body to make it pass?

b4tchkn avatar Feb 26 '24 09:02 b4tchkn

any updates? I'm pretty sure it's a bug in freezed generator. the copyWith parameters should be wrapped in a Function like ValueGetter if it's nullable types. This has been happened in another tools like Dart Data Class Generator on vscode extension in the past (from ricardoemerson/dart-data-class-tools#9) to ricardoemerson/dart-data-class-tools#11)). So you should learn from another experience like I mention. @rrousselGit

husainazkas avatar Mar 09 '24 11:03 husainazkas

any updates?

ostk0069 avatar Apr 08 '24 18:04 ostk0069

@tjarvstrand I'm a little lost here, since I assumed copyWith would work the way you described here.

I just started using Freezed and was surprised to see my object properties overwritten with null.

What is the correct approach when using Freezed's copyWith? How can I tell the method to not replace the property value if the passed value is null?

I understand not passing the value as method argument when null is one way, but that would mean that I'd have to check for each property I might want to update if they should be part of copyWith method or not?

      return user.copyWith(
        username: newUser.username // nullable
        email: newUser.phoneNumber, // nullable
        isPrivate: newUser.isPrivateProfile, // nullable
        ...
      );

would need to be written as:

      if (newUser.username != null) {
            return user.copyWith(
              username: newUser.username
            );
      }
      
      if (newUser.email != null) {
            return user.copyWith(
              email: newUser.email
            );
      }
      
      if (newUser.isPrivate != null) {
            return user.copyWith(
              isPrivate: newUser.isPrivate
            );
      }
...

Doesn't seem right. What am I missing?

keiwanmosaddegh avatar Apr 09 '24 14:04 keiwanmosaddegh

Or, I assume tailing each nullable property by a ?? oldValue is the way:

      return user.copyWith(
        username: newUser.username ?? user.username // nullable
        email: newUser.phoneNumber ?? user.phoneNumber, // nullable
        isPrivate: newUser.isPrivateProfile ?? user.isPrivateProfile, // nullable
        ...
      );

keiwanmosaddegh avatar Apr 09 '24 14:04 keiwanmosaddegh

@b4tchkn @rrousselGit

I am using freezed 2.4.7 freezed_annotation 2.4.1

Modify generated freezed file:

/// @nodoc
class _$SampleModelCopyWithImpl<T extends Object, $Res,
    $Val extends SampleModel<T>> implements $SampleModelCopyWith<T, $Res> {
  _$SampleModelCopyWithImpl(this._value, this._then);

  // ignore: unused_field
  final $Val _value;
  // ignore: unused_field
  final $Res Function($Val) _then;

  @pragma('vm:prefer-inline')
  @override
  $Res call({
    Object? value = null,
  }) {
    return _then(_value.copyWith(
      value: freezed == value // <-- NOTE: replace `null` with `freezed`
          ? _value.value
          : value // ignore: cast_nullable_to_non_nullable
              as T?,
    ) as $Val);
  }
}

// ...

and,

// ...

/// @nodoc
class __$$SampleModelImplCopyWithImpl<T extends Object, $Res>
    extends _$SampleModelCopyWithImpl<T, $Res, _$SampleModelImpl<T>>
    implements _$$SampleModelImplCopyWith<T, $Res> {
  __$$SampleModelImplCopyWithImpl(
      _$SampleModelImpl<T> _value, $Res Function(_$SampleModelImpl<T>) _then)
      : super(_value, _then);

  @pragma('vm:prefer-inline')
  @override
  $Res call({
    Object? value = null,
  }) {
    return _then(_$SampleModelImpl<T>(
      value: freezed == value // <-- NOTE: replace `null` with `freezed`
          ? _value.value
          : value // ignore: cast_nullable_to_non_nullable
              as T?,
    ));
  }
}

Then in test:

void main() {
  test('sample model test', () {
    printWrapped('hello');

    const sampleModel = SampleModel(value: 9);
    printWrapped(sampleModel);
    expect(sampleModel, const SampleModel(value: 9));

    final updatedSampleModel = sampleModel.copyWith(value: null);
    printWrapped(updatedSampleModel);
    expect(updatedSampleModel, const SampleModel<int>(value: null));
  });
}

Test result: image

erlangparasu avatar Apr 10 '24 23:04 erlangparasu

@erlangparasu

Thanks for your info.

Modify generated freezed file:

How was this done? If it is manual, that is not what I want. I am hoping that the same generated freezed file will be generated as before.

b4tchkn avatar Apr 11 '24 06:04 b4tchkn

Sorry, I'm just showing how the code is fixed. maybe we need to wait for the author to fix it based on the results of the manual patch I did earlier.

erlangparasu avatar Apr 11 '24 07:04 erlangparasu

Also note, this bug i test only appear when using generate freezed class with Generic (as described by @b4tchkn). Without generic, the generated code is works as expected (comparing with special freezed rather than null).

erlangparasu avatar Apr 11 '24 07:04 erlangparasu