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

Support deep collection equality in records #953

Open
AlexDochioiu opened this issue Jul 19, 2023 · 3 comments
Open

Support deep collection equality in records #953

AlexDochioiu opened this issue Jul 19, 2023 · 3 comments
Assignees
Labels
enhancement New feature or request

Comments

@AlexDochioiu
Copy link

Describe the bug
operator == is not correct when using dart 3 records containing List (checks reference instead of list content)

Test Class:

@freezed
class SampleClass with _$SampleClass {
  const SampleClass._();

  const factory SampleClass({
    required (List<int>, bool) record,
  }) = _SampleClass;
}

Failing Test:

  test("freezed with records", () {
    final first = SampleClass(record: ([1], false));
    final second = SampleClass(record: ([1], false));

    expect(first, second);
  });

Generated (wrong) equality function:

  @override
  bool operator ==(dynamic other) {
    return identical(this, other) ||
        (other.runtimeType == runtimeType &&
            other is _$_SampleClass &&
            (identical(other.record, record) || other.record == record));
  }

<Please add a small sample to that can be executed to reproduce the problem. As a general rule, 100 lines is the maximum>

Expected behavior
The test should pass.

@AlexDochioiu AlexDochioiu added bug Something isn't working needs triage labels Jul 19, 2023
@rrousselGit rrousselGit changed the title operator == is not correct when using dart 3 records containing List (checks reference instead of list content) Support deep collection equality in records Jul 19, 2023
@rrousselGit rrousselGit added enhancement New feature or request and removed bug Something isn't working needs triage labels Jul 19, 2023
@rrousselGit
Copy link
Owner

Not really a bug, so editing this a bit

That's consistent with how Freezed works when dealing with objects:

class Result<T> {
  Result(this.value);
  final T value;
}

@freezed
class SampleClass with _$SampleClass {
  const SampleClass._();

  const factory SampleClass({
    required Result<List<int>> obj,
  }) = _SampleClass;
}

We could support explicitly typed records.
But I feel like that could be confusing, as if a record is downcasted to say Object, equality would behave differently:

@freezed
class SampleClass with _$SampleClass {
  const SampleClass._();

  const factory SampleClass(Object obj) = _SampleClass;
}

// Will be false, and there's no way to support this any other way.
SampleClass(([1],)) == SampleClass(([1],) 

In the end, the issue is that records aren't introspectable.

So I'm not sure that's desirable.

@AlexDochioiu
Copy link
Author

AlexDochioiu commented Jul 19, 2023

Hmm, I see your perspective. I still think from a user point of view it would make more sense for records to check equality based on content. Otherwise I guess the whole equality becomes pointless as it will likely almost always return false.

At the very least I'd suggest adding warnings when records are used in freezed classes.

P.S. Never checked / not sure how code analyzer/gen works in dart. I hope it can detect dart records.

@rrousselGit
Copy link
Owner

The main issue is that like mentioned, whenever a record is assigned to Object, equality as you desire would stop working.

I feel like it would be very confusing if sometimes deep equality in records worked, and sometimes it didn't.

Supporting it would also require significant work, as DeepCollectionEquality wouldn't work anymore.
If a user typed List<(List,)> then equality wouldn't work either. So we need to stop using DeepCollectionEquality to manually implement all those cases. Yet it'd still be limited

I'd suggest making a custom List which overrides ==

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants