feat: make generated classes sealed/final #1121
Conversation
|
|
This is valuable. Although I'm wondering whether we need a flag at all. Why not always make the generated class |
|
Happy to hear that 😊 If I would implement that change in the scope of my project I would certainly make |
|
From my side the PR is now "ready", meaning I have no further actionable task on my list. Would be happy to remove a whole bunch of code and just hardcode Tried to make the CI work, but I guess the reason it failed has nothing to do with my changes, so I just added a dependency override for Also I was thinking if And finally, I tried to add a changeset as the bot requested, but I was unable to make it work by following the guide it linked to, so I assume using this package is somewhat outdated? |
You can go ahead and do this change. Only Removing the flag and making the generated classes |
|
You can ignore the changeset bot btw |
Yegair
changed the title
Yegair
force-pushed
the
feature/finalized
branch
from
af0373d to
2ef0d8b
Compare
|
Removed the configuration part, so basically it now just adds the I think it should be ready to merge. If there is something else that should be changed, just let me know, but throughout the week it might take a while until I can get to it. And since I get the chance to talk to you directly, just wanted to let you know how valuable Freezed and especially Riverpod are to the project I am working on (https://choosy.de if you want to know what people are building with your libs 😉). Since migrating from Bloc to Riverpod the ease of using providers from inside other providers made such a huge difference. Cant wait to get a first glimpse of the wizardry you (hopefully) are going to do with macros once they are stable 😊. |
|
Cool! I'll leave this open for a few more days, but it should be good to go. |
WalkthroughThis pull request refines sealed class handling in the Freezed package. The Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant SealedClass
participant Analyzer
TestRunner->>SealedClass: Instantiate sealed class (e.g., FinalizedFoo)
TestRunner->>Analyzer: Execute switch pattern check on instance
Analyzer-->>TestRunner: Return PATTERN_NEVER_MATCHES_VALUE_TYPE warning
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (6)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (4)
packages/freezed/test/integration/finalized.dart (2)
10-13: Consider consolidating similar test casesThis class is structurally identical to
FinalizedFoo. Consider whether having two identical test cases adds value, or if the test coverage could be achieved with a single class.
15-20: LGTM: Comprehensive test case for pattern matchingThis class provides excellent coverage for testing pattern matching with multiple constructors, which is crucial for validating the compile-time warnings mentioned in the PR objectives.
Consider adding test cases for:
- Nested sealed classes
- Generic sealed classes
- Sealed classes with complex property types
These additional cases would help ensure the feature works correctly in more complex scenarios.
packages/freezed/test/finalized_test.dart (2)
8-42: Consider enhancing test coverage for single constructor case.The test effectively verifies the warning for impossible pattern matches. Consider these enhancements:
- Add a positive test case to verify that valid pattern matches don't trigger warnings
- Verify the error message content for better diagnostic coverage
- Assert the error location (line/column) to ensure precise diagnostic reporting
Example enhancement:
test('causes pattern_never_matches_value_type warning when trying to match on pattern that can never match', () async { final main = await resolveSources( { 'freezed|test/integration/main.dart': r''' library main; import 'finalized.dart'; void main() { switch (FinalizedFoo()) { case FinalizedBar(): break; case FinalizedFoo(): break; } } ''', }, (r) => r.findLibraryByName('main'), ); final errorResult = await main!.session .getErrors('/freezed/test/integration/main.dart') as ErrorsResult; expect(errorResult.errors, hasLength(1)); final [error] = errorResult.errors; expect(error.errorCode.errorSeverity, ErrorSeverity.WARNING); expect(error.errorCode.name, 'PATTERN_NEVER_MATCHES_VALUE_TYPE'); + expect(error.message, contains('pattern can never match')); + expect(error.location.lineNumber, 10); + expect(error.location.columnNumber, 10); }); + test('does not warn for valid pattern matches', () async { + final main = await resolveSources( + { + 'freezed|test/integration/main.dart': r''' + library main; + import 'finalized.dart'; + + void main() { + switch (FinalizedFoo()) { + case FinalizedFoo(): + break; + } + } + ''', + }, + (r) => r.findLibraryByName('main'), + ); + + final errorResult = await main!.session + .getErrors('/freezed/test/integration/main.dart') as ErrorsResult; + + expect(errorResult.errors, isEmpty); + });
44-85: Consider refactoring multiple constructors test for better maintainability.While the test effectively verifies the core functionality, consider these improvements:
- Split into separate test cases for each constructor variant
- Extract common error verification logic into a test utility
- Add positive test cases for valid pattern matches
Example refactoring:
+Future<void> expectPatternMatchWarning(String source) async { + final main = await resolveSources( + {'freezed|test/integration/main.dart': source}, + (r) => r.findLibraryByName('main'), + ); + + final errorResult = await main!.session + .getErrors('/freezed/test/integration/main.dart') as ErrorsResult; + + expect(errorResult.errors, hasLength(1)); + final [error] = errorResult.errors; + expect(error.errorCode.errorSeverity, ErrorSeverity.WARNING); + expect(error.errorCode.name, 'PATTERN_NEVER_MATCHES_VALUE_TYPE'); +} group('multiple constructors', () { - test('causes pattern_never_matches_value_type warning when trying to match on pattern that can never match', + test('warns when matching FinalizedMultiple.b against FinalizedBar', () async { - final main = await resolveSources( - { - 'freezed|test/integration/main.dart': r''' + await expectPatternMatchWarning(r''' library main; import 'finalized.dart'; void main() { switch (FinalizedMultiple.b()) { case FinalizedBar(): break; } } - ''', - }, - (r) => r.findLibraryByName('main'), - ); + '''); + }); + test('warns when matching FinalizedMultiple.b against FinalizedMultipleA', + () async { + await expectPatternMatchWarning(r''' + library main; + import 'finalized.dart'; + + void main() { + switch (FinalizedMultiple.b()) { + case FinalizedMultipleA(): + break; + } + } + '''); + }); + test('allows matching FinalizedMultiple.b against FinalizedMultipleB', + () async { + final main = await resolveSources( + { + 'freezed|test/integration/main.dart': r''' + library main; + import 'finalized.dart'; + + void main() { + switch (FinalizedMultiple.b()) { + case FinalizedMultipleB(): + break; + } + } + ''', + }, + (r) => r.findLibraryByName('main'), + ); + + final errorResult = await main!.session + .getErrors('/freezed/test/integration/main.dart') as ErrorsResult; + + expect(errorResult.errors, isEmpty); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/freezed/lib/src/templates/concrete_template.dart(2 hunks)packages/freezed/pubspec_overrides.yaml(1 hunks)packages/freezed/test/finalized_test.dart(1 hunks)packages/freezed/test/integration/finalized.dart(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/freezed/pubspec_overrides.yaml
🔇 Additional comments (4)
packages/freezed/test/integration/finalized.dart (2)
1-3: LGTM: Proper Freezed setup
The import and part directive follow the standard Freezed setup pattern.
5-8: LGTM: Simple sealed class test case
This provides a good test case for the simplest possible sealed class scenario with a single constructor.
Let's verify the test coverage for this class:
packages/freezed/test/finalized_test.dart (1)
1-7: LGTM! Well-structured test setup.
The imports and main test group structure are appropriate for testing the sealed/final class behavior.
packages/freezed/lib/src/templates/concrete_template.dart (1)
67-67:
Update minimum Dart SDK constraint due to use of final class and sealed class
The use of final class (line 67) and sealed class (line 89) requires Dart SDK version 2.17.0 or higher. Please update the minimum SDK version in your pubspec.yaml file to >=2.17.0 to ensure compatibility and prevent build errors for users on earlier SDK versions.
Run the following script to verify the current SDK constraints:
Also applies to: 89-89
|
My appologies, I kind of forgot about this PR. Let me see what we can do here. |
|
Friendly Reminder to take a look at this PR 😊 If there is a way I can help testing it, just let me know! |
Is this PR stil relevant as now freezed support "extends" ? |
Yegair
force-pushed
the
feature/finalized
branch
from
47f79b8 to
c7dcdbb
Compare
At least the original way how it was implemented will no longer work. Just did a quick rebase from upstream, but have to write quite a few more tests to make sure it works in combination with the new features from release Will take a closer look on the weekend! |
|
Sorry as you can see, I got sidetracked. I'm not sure whether changing the generated code makes sense, when analyzer likely should know already that the After-all, |
|
Actually ignore me, I can see why the analyzer works this way. if you don't mind fixing the conflicts, we can merge this. For now, let's not enable it by default though. I'd rather not make a 4.0 right after a 3.0 |
Adds a new configuration parameter
finalize: boolthat allows to mark generated classes asfinalin the case of concrete classes orsealedin the case of abstract classes. It is disabled by default.Enabling it allows the Dart analyzer/compiler to report patterns that will never match the type of a freezed class at compile time.
For example:
Why do I think this is a good idea?
I have recently had an issue in one of my projects where I have a
@freezed sealed class DataFromApiand I introduced a new@freezed sealed class DataFromDatabasethat was intended to be used in many but not all cases whereDataFromApiwas being used so far.It was a normal change, but I messed up due to the heavy use of pattern matching like
The changes mostly looked like this
I was expecting the Dart analyzer/compiler to yell at me when changing the type of
datain the example above toDataFromDatabase, but that was not the case. So I had to find all the now broken patterns by hand. Naturally I overlooked one case and the tests didn't catch it, so I got a Bug in production 😅.IMHO the Dart analyzer/compiler should be able to help me in this case no matter if I use freezed or not, since the classes in question were marked as
sealed. So I opened an issue over there: dart-lang/sdk#56616However, since this might never be implemented or maybe is completely impossible, I thought it should be possible and rather simple to make freezed help me out in that case. All that needs to be done is mark the generated classes as
finalorsealed, and then the analyzer will step in and report any pattern that can never match at compile time.What needs to be done before it might be merged?
Good question, I think first of all someone needs to decide if this even is a good idea or if it would mess things up. In case it should be merged, I assume a few more tests need to be written to make sure the
finalizeflag works well with other configuration options. However, so far I have a hard time figuring out what even might break due to this change, so I would need help to decide what special/edge cases to consider when writing tests.Summary by CodeRabbit
New Features
Tests