fix: include type parameters in switch cases and copyWith factories f… #1335
Conversation
|
Warning Rate limit exceeded@julienzarka has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 11 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughTemplates for copyWith and pattern/when generation were changed to emit explicit generic parameters in generated declarations; a sealed generic test fixture and tests were added to validate generated switch cases and copyWith implementations for generics. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/freezed/test/sealed_generic_switch_test.dart (1)
10-29: Consider defensive assertion before cast and const path extraction for code clarity.The unchecked
as ErrorsResultcast and hardcoded path follow the established pattern throughout the test suite, but adding a type assertion and extracting the path to a constant would improve readability and provide clearer error messages if the analyzer result type ever changes.Suggested improvement
test('sealed class with type parameters generates correct switch cases', () async { - final main = await resolveSources( + final library = await resolveSources( {'freezed|test/integration/sealed_generic_switch/sealed_generic.dart': useAssetReader}, (r) => r.libraries.firstWhere( (element) => element.firstFragment.source.toString().contains('sealed_generic'), ), ); - final errorResult = - await main.session.getErrors( - '/freezed/test/integration/sealed_generic_switch/sealed_generic.freezed.dart', - ) - as ErrorsResult; + const generatedPath = + '/freezed/test/integration/sealed_generic_switch/sealed_generic.freezed.dart'; + final result = await library.session.getErrors(generatedPath); + expect(result, isA<ErrorsResult>(), reason: 'Expected ErrorsResult for $generatedPath'); + final errorResult = result as ErrorsResult; expect(errorResult.errors, isEmpty); });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/freezed/lib/src/templates/copy_with.dartpackages/freezed/lib/src/templates/pattern_template.dartpackages/freezed/test/integration/sealed_generic_switch/sealed_generic.dartpackages/freezed/test/sealed_generic_switch_test.dart
🔇 Additional comments (5)
packages/freezed/lib/src/templates/pattern_template.dart (2)
290-303: Good fix: propagate generic type args intoswitchcase patterns (when variants).
Same rationale as_mapImpl; keeps pattern matching consistent for generic sealed unions.
218-226: Good fix: propagate generic type args into switch case patterns.This correctly aligns case patterns with generic redirected types. The implementation uses
genericsParameterTemplate, which renders empty string for non-generic classes (avoiding stray<>) and generates<T>or<T, U>as needed for generic types.Dart 3 pattern matching supports this syntax:
case SomeGeneric<T>():is valid across stable Dart SDK versions. Integration tests confirm the generated patterns likecase SealedGenericFirst<T>():produce no analyzer errors.packages/freezed/lib/src/templates/copy_with.dart (1)
63-71: Correct: include$Resin the factory redirect implementation type args.
This makes the factory target’s generic arguments match the generated impl class declaration and avoids type-resolution failures.packages/freezed/test/integration/sealed_generic_switch/sealed_generic.dart (1)
1-9: Nice minimal fixture for exercising generic sealed codegen.
Covers the exact scenario that previously produced missing type args in switch cases and copyWith impl references.packages/freezed/test/sealed_generic_switch_test.dart (1)
31-45: String-based assertions look good for guarding against regressions in generated output.
One thing to double-check is that the generated.freezed.dartis reliably present in the test environment (otherwise this becomes order/CI-step dependent).
julienzarka
force-pushed
the
fix/sealed-generic-type-parameters
branch
from
ef95336 to
24a1809
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/freezed/test/sealed_generic_switch_test.dart (1)
46-60: Simplify: Redundant type check after explicit assertion.Line 41 already asserts
fileResultis aFileResult, so the conditional check on line 46 is redundant. If the assertion passes, the type is guaranteed.♻️ Simplify by removing redundant check
- if (fileResult is FileResult) { - final content = fileResult.content; - if (content.isNotEmpty) { - // Verify that switch cases include type parameters - expect(content, contains('case SealedGenericFirst<T>():')); - expect(content, contains('case SealedGenericSecond<T>():')); - - // Verify that copyWith factory includes type parameters - expect(content, contains('= _\$SealedGenericCopyWithImpl<T, \$Res>;')); - expect(content, contains('= _\$SealedGenericFirstCopyWithImpl<T, \$Res>;')); - expect(content, contains('= _\$SealedGenericSecondCopyWithImpl<T, \$Res>;')); - } - // If content is empty, the first test's error check is sufficient - // as it confirms the generated code compiles without errors - } + final content = fileResult.content; + if (content.isNotEmpty) { + // Verify that switch cases include type parameters + expect(content, contains('case SealedGenericFirst<T>():')); + expect(content, contains('case SealedGenericSecond<T>():')); + + // Verify that copyWith factory includes type parameters + expect(content, contains('= _\$SealedGenericCopyWithImpl<T, \$Res>;')); + expect(content, contains('= _\$SealedGenericFirstCopyWithImpl<T, \$Res>;')); + expect(content, contains('= _\$SealedGenericSecondCopyWithImpl<T, \$Res>;')); + } + // If content is empty, the first test's error check is sufficient + // as it confirms the generated code compiles without errors
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/freezed/lib/src/templates/copy_with.dartpackages/freezed/lib/src/templates/pattern_template.dartpackages/freezed/test/integration/sealed_generic_switch/sealed_generic.dartpackages/freezed/test/sealed_generic_switch_test.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/freezed/lib/src/templates/pattern_template.dart
🔇 Additional comments (4)
packages/freezed/lib/src/templates/copy_with.dart (1)
67-67: LGTM! Type parameter fix correctly applied.The addition of
${genericsParameter.append('\$Res')}ensures the factory's implementation class reference includes all necessary type parameters, fixing compilation errors for generic sealed classes. This matches the pattern used elsewhere in the template (e.g., line 97, line 149).packages/freezed/test/integration/sealed_generic_switch/sealed_generic.dart (1)
1-9: LGTM! Well-structured test fixture.The test fixture appropriately exercises generic sealed class generation with:
- Explicit type parameters on both the sealed class and factory redirections
- Two factory constructors to test multiple switch cases
- Minimal complexity, focused on the specific issue being fixed
packages/freezed/test/sealed_generic_switch_test.dart (2)
7-25: LGTM! Clear test for compilation correctness.The test effectively verifies that generic sealed classes generate error-free code, which is the primary objective of this fix.
48-56: LGTM! Content assertions correctly verify the fix.The string expectations properly validate both aspects of the PR fix:
- Switch case patterns include type parameters (lines 50-51)
- CopyWith factory implementations include type parameters (lines 54-56)
The defensive check for empty content with fallback to the first test is appropriate.
julienzarka
force-pushed
the
fix/sealed-generic-type-parameters
branch
from
24a1809 to
7c28846
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/freezed/test/sealed_generic_switch_test.dart (2)
7-25: Solid integration test for compilation errors.The test correctly verifies that the generated code compiles without errors, which is the primary validation that generic switch cases are properly formed. The defensive check for
ErrorsResulttype is good practice.One minor suggestion: the
firstWherepredicate (line 11-12) uses string matching on the source path. While functional, this could match unintended files if the repo contains multiple files with "sealed_generic" in the path.♻️ Optional: More specific library resolution
(r) => r.libraries.firstWhere( (element) => - element.firstFragment.source.toString().contains('sealed_generic'), + element.firstFragment.source.uri.path.endsWith('sealed_generic_switch/sealed_generic.dart'), ),This ensures exact matching of the intended test fixture file.
27-59: Consider making the content verification more explicit.The test performs valuable string-based verification of the generated code patterns. However, the conditional check at line 47 (
if (content.isNotEmpty)) means the test passes silently when content cannot be retrieved, relying entirely on the first test's error checking.While the comments explain this fallback, it could mask issues where:
- The file isn't generated properly but has no analysis errors
- The session can't access the file for other reasons
- The generated code is syntactically valid but missing the expected patterns
♻️ Optional: Make content check more explicit
Consider either:
- Making content availability explicit:
if (content.isNotEmpty) { // Verify that switch cases include type parameters expect(content, contains('case SealedGenericFirst<T>():')); // ... other checks +} else { + // Document why empty content is acceptable or fail explicitly + fail('Generated file content is empty - cannot verify type parameters in generated code'); }
- Or add a skip condition:
if (content.isNotEmpty) { // Verify that switch cases include type parameters expect(content, contains('case SealedGenericFirst<T>():')); // ... other checks +} else { + skip('File content not available in this test environment - relying on error check in first test'); }This makes the test's behavior more explicit and easier to debug if it starts failing.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/freezed/lib/src/templates/copy_with.dartpackages/freezed/lib/src/templates/pattern_template.dartpackages/freezed/test/integration/sealed_generic_switch/sealed_generic.dartpackages/freezed/test/sealed_generic_switch_test.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/freezed/test/integration/sealed_generic_switch/sealed_generic.dart
- packages/freezed/lib/src/templates/copy_with.dart
🔇 Additional comments (3)
packages/freezed/lib/src/templates/pattern_template.dart (2)
219-219: LGTM! Correct fix for generic switch case patterns.The addition of
${data.genericsParameterTemplate}ensures that switch case patterns include type parameters (e.g.,case ClassName<T>():instead ofcase ClassName():), which is necessary for Dart's exhaustiveness checking with generic sealed classes. This is consistent with how generics are used elsewhere in the file (line 201).
291-291: LGTM! Parallel fix for when-style pattern matching.This applies the same type parameter fix to the
when-family of methods, ensuring consistency across all pattern-matching code generation paths.packages/freezed/test/sealed_generic_switch_test.dart (1)
1-6: LGTM! Test setup is appropriate.The imports and test structure are correct for integration testing with
build_testand the analyzer.
julienzarka
force-pushed
the
fix/sealed-generic-type-parameters
branch
3 times, most recently
from
b1f089a to
60e643c
Compare
…or sealed classes - Add type parameters to switch case patterns for generic sealed classes - Include type parameters in copyWith factory implementations - Add comprehensive tests for both generic and non-generic sealed classes - Ensure backward compatibility with non-generic sealed classes Tests: - Verify generic sealed classes generate correct switch cases with type params - Verify copyWith factories include type parameters for generic sealed classes - Verify non-generic sealed classes continue to work correctly (backward compatibility)
julienzarka
force-pushed
the
fix/sealed-generic-type-parameters
branch
from
60e643c to
0d0b28e
Compare
|
Please, merge this PR as soon as possible , it is a critical problem |
2 participants
Fix: Include type parameters in switch cases and copyWith factories for sealed classes
Problem
When generating code for sealed classes with type parameters, the generated code was missing type parameters in two critical places:
This caused compilation errors where Dart's analyzer flags switch statements as non-exhaustive when type parameters are missing. It also caused type errors where CopyWith factory methods cannot resolve the correct implementation class. As a result, projects need post-generation patching scripts to fix the generated code.
Solution
I made minimal, surgical changes to include type parameters where required:
Pattern Template (pattern_template.dart):
CopyWith Template (copy_with.dart):
Impact
Sealed classes with type parameters now generate correct, compilable code. This eliminates the need for post-generation patching scripts. The changes maintain backward compatibility since classes without type parameters are unaffected. All existing tests pass.
Testing
I added comprehensive test coverage:
Test Results:
Files Changed
Example
Before:
After:
Summary by CodeRabbit
Bug Fixes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.