Skip to content

Conversation

@devalgupta404
Copy link

@devalgupta404 devalgupta404 commented Oct 18, 2025

Fixes #1308 - EqualUnmodifiableListView does not correctly override ==

Changes

  • Updated EqualUnmodifiableListView, EqualUnmodifiableSetView, and EqualUnmodifiableMapView to use DeepCollectionEquality for content-based comparison
  • Fixed equality operator to compare collection contents instead of object references
  • Updated hashCode methods to use content-based hashing for consistency

Impact

This resolves the inconsistency where:

  • Freezed List objects compared by content
  • Freezed objects containing List properties compared by object reference

Now both scenarios use content-based comparison, making the behavior consistent and fixing the Riverpod ref.watch(Provider).select() issue.

Changes

Before:

// This would return false even with same content
final list1 = EqualUnmodifiableListView([1, 2, 3]);
final list2 = EqualUnmodifiableListView([1, 2, 3]);
print(list1 == list2); // false 

After:

// Now correctly returns true for same content
final list1 = EqualUnmodifiableListView([1, 2, 3]);
final list2 = EqualUnmodifiableListView([1, 2, 3]);
print(list1 == list2); // true 

Summary by CodeRabbit

  • Bug Fixes
    • Collection-view equality and hash computation now correctly consider nested collection contents, fixing incorrect comparisons and hashes.
    • Generated class hashCode behavior adjusted to respect const-ness and mixin sources, yielding more consistent and correct hashCode semantics.

@coderabbitai
Copy link

coderabbitai bot commented Oct 18, 2025

Walkthrough

Equality and hashCode implementations for unmodifiable collection views were changed to use deep collection equality; code generation templates were updated to propagate an isConst flag and to adjust emitted hashCode generation logic based on const-ness and mixin/source.

Changes

Cohort / File(s) Summary
Deep collection equality for unmodifiable views
packages/freezed_annotation/lib/freezed_annotation.dart
EqualUnmodifiableListView, EqualUnmodifiableSetView, and EqualUnmodifiableMapView now use DeepCollectionEquality().equals(_source, other._source) in operator == and DeepCollectionEquality().hash(_source) inside hashCode, replacing direct reference equality and direct hashing of _source.
Codegen: const-aware method and hashCode emission
packages/freezed/lib/src/templates/abstract_template.dart, packages/freezed/lib/src/templates/concrete_template.dart
Propagate a new isConst boolean into methods(...) and hashCodeMethod(...) call sites and signatures; hashCode emission logic now selects different expressions/caching paths depending on isConst and source, with adjusted branching for single vs multiple hashed properties.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Generator as Freezed Generator
  participant MethodsBuilder as methods(...)
  participant HashBuilder as hashCodeMethod(...)
  note right of Generator: When generating concrete classes

  Generator->>MethodsBuilder: call methods(..., isConst: constructor.isConst)
  MethodsBuilder-->>Generator: emitted methods (use isConst to choose constness)

  Generator->>HashBuilder: call hashCodeMethod(..., source: source, isConst: isConst)
  alt isConst or source is mixin
    HashBuilder-->>Generator: emit direct Object.hash / expression (no cached field)
  else not const and not mixin
    alt single property
      HashBuilder-->>Generator: emit simple Object.hash expression
    else many properties
      HashBuilder-->>Generator: emit cached _cachedHashCode using hashAll/hash expression
    end
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I hopped through views both map and list,
Found values lost in shallow mist.
I nudged equality to look inside,
And taught hash codes where secrets hide.
Now generated classes hum with const-aware pride.

Pre-merge checks and finishing touches

❌ Failed checks (2 inconclusive)
Check name Status Explanation Resolution
Linked Issues Check ❓ Inconclusive The code changes clearly address the core requirements from linked issue #1308. The freezed_annotation.dart file directly implements the three primary coding objectives: equality operators for EqualUnmodifiableListView, EqualUnmodifiableSetView, and EqualUnmodifiableMapView now use DeepCollectionEquality().equals() for content-based comparison instead of reference equality [requirement 1-2], and corresponding hashCode methods are updated to use DeepCollectionEquality().hash() for consistency [requirement 3]. The template changes in abstract_template.dart and concrete_template.dart appear to be supporting infrastructure for const-aware code generation and optimized hashCode logic. However, the raw_summary does not display any test file changes, making it impossible to verify whether objective 4 (updating tests to validate equality for equal-but-distinct collection instances) was completed as stated in the issue requirements. To reach a conclusive assessment, clarify whether test files have been updated to validate that Freezed objects containing equal-but-distinct collection instances correctly compare as equal. The core implementation changes satisfy the primary technical requirements from #1308, but verification of test coverage is needed to fully confirm all stated objectives were met. Additionally, consider documenting the necessity and scope of the template changes (isConst parameter propagation) to clarify their relationship to the primary fix.
Out of Scope Changes Check ❓ Inconclusive The primary changes in freezed_annotation.dart directly address issue #1308 and remain in scope. However, the modifications to abstract_template.dart and concrete_template.dart introduce parameter changes to the methods() and hashCodeMethod() function signatures and alter code generation logic for const-aware and mixin-aware hashCode computation. While these template changes appear to be supporting infrastructure for consistent equality and hashCode behavior across const and non-const classes, the issue #1308 makes no explicit mention of template changes or code generation modifications. The necessity of these template changes for implementing the fix is not explicitly explained in the provided PR objectives. Clarify whether the template changes in abstract_template.dart and concrete_template.dart are necessary supporting changes for the EqualUnmodifiable*View fix or represent separate optimization work. If they are necessary, document their relationship to the main issue and why they must be included in this PR. If they are separate concerns, consider whether they should be addressed in a separate pull request to keep this PR focused on the specific #1308 issue.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "Fix EqualUnmodifiableListView equality comparison (Fixes #1308)" is clear, concise, and directly summarizes the primary change. It accurately reflects the main objective of switching from reference-based to content-based equality comparison for the unmodifiable collection view classes. The title is specific enough to convey meaningful information without being overly verbose or vague. It appropriately references the linked issue, providing helpful context for history scanning.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c038bc5 and 5f914e3.

📒 Files selected for processing (2)
  • packages/freezed/lib/src/templates/abstract_template.dart (1 hunks)
  • packages/freezed/lib/src/templates/concrete_template.dart (4 hunks)
🔇 Additional comments (4)
packages/freezed/lib/src/templates/abstract_template.dart (1)

49-49: LGTM! Correct const-ness for mixin generation.

Passing isConst: false for mixin generation is correct. Mixins cannot declare instance fields, so they cannot cache hashCode regardless of const-ness. The explicit false value makes the semantics clear, even though the source: Source.mixin check in hashCodeMethod (line 502 of concrete_template.dart) would prevent caching anyway.

packages/freezed/lib/src/templates/concrete_template.dart (3)

53-53: LGTM! Proper propagation of const-awareness.

The isConst parameter is correctly propagated from the constructor through methods() to hashCodeMethod(). Using required ensures the flag is always explicitly provided, preventing accidental omissions.

Also applies to: 328-328, 334-334, 471-471


496-500: LGTM! Efficient hashCode expression selection.

The tiered approach is well-designed:

  • Single property: direct .hashCode access (minimal overhead)
  • 2-19 properties: Object.hash(...) for efficient fixed-arity hashing
  • 20+ properties: Object.hashAll([...]) to handle unlimited arguments

This correctly handles the parameter limit of Object.hash.


502-516: LGTM! Correct const-aware hashCode caching.

The conditional caching logic is well-reasoned:

No caching for const or mixin:

  • Const instances cannot have fields assigned post-construction (_cachedHashCode would violate const-ness)
  • Mixins cannot declare instance fields (only abstract members)

Caching for non-const classes:

  • Freezed classes are immutable (final fields), making caching safe
  • Lazy initialization with ??= provides performance benefit
  • Thread-safe in Dart's single-threaded execution model (worst case: harmless double computation)

This optimization appropriately balances correctness with performance.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

EqualUnmodifiableListView does not correctly override ==

1 participant