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

copyWith may not work correctly when using null #906

Closed
b4tchkn opened this issue May 12, 2023 · 19 comments · Fixed by #1067
Closed

copyWith may not work correctly when using null #906

b4tchkn opened this issue May 12, 2023 · 19 comments · Fixed by #1067
Assignees
Labels
bug Something isn't working

Comments

@b4tchkn
Copy link

b4tchkn commented May 12, 2023

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 b4tchkn added bug Something isn't working needs triage labels May 12, 2023
@tjarvstrand
Copy link

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

@rrousselGit
Copy link
Owner

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

This is indeed a bug.

@tjarvstrand
Copy link

Ah, TIL. Sorry about that!

@iandis
Copy link

iandis commented Aug 14, 2023

Any update on this?

@rrousselGit
Copy link
Owner

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

@myxzlpltk
Copy link

I see. It using Freezed() as default

@erlangparasu
Copy link

erlangparasu commented Dec 20, 2023

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)

@b4tchkn
Copy link
Author

b4tchkn commented Feb 19, 2024

Any update on this?
Our app has not been able to update this because of this bug.
I think this change has affected.
#808

Is there any solution?

@erlangparasu
Copy link

@b4tchkn which version is currently used in your app?

@b4tchkn
Copy link
Author

b4tchkn commented Feb 20, 2024

We are still using 2.2.1.

@b4tchkn
Copy link
Author

b4tchkn commented Feb 26, 2024

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

@husainazkas
Copy link

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

@ostk0069
Copy link

ostk0069 commented Apr 8, 2024

any updates?

@keiwanmosaddegh
Copy link

@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
Copy link

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
        ...
      );

@erlangparasu
Copy link

erlangparasu commented Apr 10, 2024

@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

@b4tchkn
Copy link
Author

b4tchkn commented Apr 11, 2024

@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.

@erlangparasu
Copy link

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
Copy link

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants