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

Generic freezed generates an incorrect copyWith #807

Closed
shilangyu opened this issue Nov 29, 2022 · 3 comments · Fixed by #808
Closed

Generic freezed generates an incorrect copyWith #807

shilangyu opened this issue Nov 29, 2022 · 3 comments · Fixed by #808
Labels
bug Something isn't working needs triage

Comments

@shilangyu
Copy link

Describe the bug

When using a generic freezed object, the copywith implementation defaults fields to null which is incorrect since the type argument can be a nullable type which violates the feature of freezed: copyWith can set nulls.

To Reproduce

I created a repo with the described bug and generated files: https://github.com/shilangyu/freezed-copywith-null

Here is a summary:

Given a freezed object

@freezed
class State<T> with _$State<T> {
  const factory State({
    required T value,
  }) = _State<T>;
}

The generated copyWith defaults to null

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

Which breaks usages such as

  test('copy null', () {
    const state = State<int?>(value: 123);

    // all good
    expect(state.value, 123);

    // ugh-oh, that should work, but it does not
    final copied = state.copyWith(value: null);
    expect(copied.value, null);
  });

Expected behavior

The above copyWith with a null value should work, as previous versions of freezed worked like that and no such breaking change was stated in the changelog. Luckily when upgrading freezed in our project tests caught this issue.

Additional info

The problem originates from this #735 PR. I would argue, that the change with conditional null and freezed should be reverted. The speed gain is negligible and creates many edge cases, another example would be #803 which IMO is not a low priority bug since it broke existing code. The PR uses some e.isNullable to decide whether to use freezed or null, my educated guess would be that this only handles types which are explicitly marked as nullable but not those that can have instances which are null.

@shilangyu shilangyu added bug Something isn't working needs triage labels Nov 29, 2022
@rrousselGit
Copy link
Owner

I'll see what can be done.

@rrousselGit
Copy link
Owner

There you go. I've handled both dynamic and generic

I'll try to fix other issues and make a release

@shilangyu
Copy link
Author

That was quick, thanks

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

Successfully merging a pull request may close this issue.

2 participants