freezed icon indicating copy to clipboard operation
freezed copied to clipboard

feat: make generated classes sealed/final

Open Yegair opened this issue 1 year ago โ€ข 21 comments

Adds a new configuration parameter finalize: bool that allows to mark generated classes as final in the case of concrete classes or sealed in 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:

@Freezed(finalize: true)
sealed class Foo with _$Foo {
  const Foo._();
  const factory Foo() = _Foo;
}

@Freezed(finalize: true)
sealed class Bar with _$Bar {
  const Bar._();
  const factory Bar() = Bar;
}

void main() {
  switch (Foo()) {
    // This will now cause an analyzer warning: pattern_never_matches_value_type.
    // Without finalize: true it would not cause any warning/error at all.
    case Bar():
      print("matched Bar()");
      return;

    case Foo():
      print("matched Foo()");
      return;
  }
}

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 DataFromApi and I introduced a new @freezed sealed class DataFromDatabase that was intended to be used in many but not all cases where DataFromApi was being used so far.

It was a normal change, but I messed up due to the heavy use of pattern matching like

switch (data) {
  case DataFromApi(someCondition: true):
    // ...
    break;

  case DataFromApi(someCondition: false):
    // ...
    break;

  default:
    // ...
    break;
}

The changes mostly looked like this

- final data = await loadDataFromApi();
+ final data = await loadDataFromDatabase();

switch (data) {
  case DataFromApi(someCondition: true):
    // ...
    break;

    // ...
}

I was expecting the Dart analyzer/compiler to yell at me when changing the type of data in the example above to DataFromDatabase, 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: https://github.com/dart-lang/sdk/issues/56616

However, 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 final or sealed, 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 finalize flag 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

    • Added a new property for class modifiers, allowing dynamic inclusion of modifiers like final for generated classes.
    • Introduced new sealed and abstract class patterns that utilize the class modifiers feature.
  • Tests

    • Expanded the test suite to validate the behavior of final and sealed classes.
    • Enhanced checks for proper warnings when using pattern matching with final classes.
    • Improved coverage for the .fromJson method in the Freezed class, including additional property validations.

Yegair avatar Aug 31 '24 13:08 Yegair

โš ๏ธ No Changeset found

Latest commit: 4774262e93bfe068feb9bb79326891f33b57b925

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

changeset-bot[bot] avatar Aug 31 '24 13:08 changeset-bot[bot]

This is valuable.

Although I'm wondering whether we need a flag at all. Why not always make the generated class final if it helps? Extending a Freezed class isn't supported anyway, as they only have factory constructors.

rrousselGit avatar Aug 31 '24 13:08 rrousselGit

Happy to hear that ๐Ÿ˜Š

If I would implement that change in the scope of my project I would certainly make final/sealed the default. However, I find it very hard to anticipate how freezed is being used out there in the wild and making it the default feels like doing a breaking change. So I would ask you to make that decision, but for me at least it would definitely work.

Yegair avatar Aug 31 '24 14:08 Yegair

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 sealed and final when generating subclasses, but as I said before: I would 100% support this, but I don't think I am in a position to make that decision.

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 frontend_server_client: ^4.0.0. However, I am unsure if this might have negative side effects, so would be happy to remove it if requested.

Also I was thinking if finalize is a good term to use (I'm not a native english speaker), but maybe makeGeneratedClassesFinal or something in this direction would be easier to understand for users?

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?

Yegair avatar Sep 01 '24 16:09 Yegair

Would be happy to remove a whole bunch of code and just hardcode sealed and final when generating subclasses, but as I said before: I would 100% support this, but I don't think I am in a position to make that decision.

You can go ahead and do this change. Only _$Foo classes are sub-classable at the moment, and that's not something folks should be using anyway.

Removing the flag and making the generated classes final are beneficial to more people this way. Otherwise most folks won't even know that the option is there and won't benefit from the warning.

rrousselGit avatar Sep 01 '24 16:09 rrousselGit

You can ignore the changeset bot btw

rrousselGit avatar Sep 01 '24 16:09 rrousselGit

Removed the configuration part, so basically it now just adds the final/sealed modifiers and of course left in some tests that make sure it works as intended.

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 ๐Ÿ˜Š.

Yegair avatar Sep 02 '24 07:09 Yegair

Cool! I'll leave this open for a few more days, but it should be good to go. I just need to try it locally and evaluate the possible danger of this final change to be safe :)

rrousselGit avatar Sep 02 '24 18:09 rrousselGit

Walkthrough

The pull request introduces a new property for class modifiers in the Freezed package, allowing dynamic inclusion of modifiers like final in generated class declarations. This involves updates to the class structure and the addition of new fields and methods in the models and annotations. Comprehensive unit tests are added to validate the behavior of final and sealed classes, ensuring correct handling of patterns and JSON deserialization.

Changes

Cohort / File(s) Change Summary
Class Modifiers
packages/freezed/lib/src/templates/concrete_template.dart, packages/freezed/lib/src/models.dart, packages/freezed_annotation/lib/freezed_annotation.dart, packages/freezed_annotation/lib/freezed_annotation.g.dart
Added a new property for class modifiers in class declarations; introduced a getter and field in models, and updated JSON deserialization to include class modifiers.
Test Updates
packages/freezed/test/finalized_test.dart, packages/freezed/test/integration/finalized.dart, packages/freezed_annotation/test/freezed_test.dart
Added and updated tests to validate behavior of final/sealed classes, including warnings for pattern matches and assertions for JSON .fromJson properties.

Sequence Diagram(s)

No sequence diagram is provided, as the changes in this pull request do not introduce a new feature or significantly modify the control flow. The changes are focused on adding a new property to control class modifiers and updating the corresponding tests.

Estimated code review effort

๐ŸŽฏ 3 (Moderate) | โฑ๏ธ ~25 minutes

The changes in this pull request have a moderate level of complexity. The addition of a new property to control class modifiers in the Freezed package requires updates to the class structure, models, and annotations. The comprehensive suite of tests added to validate the behavior of final and sealed classes adds further complexity to the review. While the changes are focused on a specific aspect of the Freezed package, the spread across multiple files and the introduction of new tests make the overall effort moderately complex.

Suggested reviewers

  • rrousselGit

Poem

I'm a little bunny, hopping through code so bright,
Adding a final touch to every class in sight.
With a skip and a jump, the tests now run clean,
Sealed and abstract, shining like a dream.
In fields and templates, the magic is spun โ€“
CodeRabbit delights in the changes we've done!
๐Ÿ‡๐Ÿ’ป

[!TIP]

๐Ÿ”Œ Remote MCP (Model Context Protocol) integration is now available!

Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.

โœจ Finishing Touches
๐Ÿงช Generate unit tests
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment

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
๐Ÿชง Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

coderabbitai[bot] avatar Nov 03 '24 11:11 coderabbitai[bot]

My appologies, I kind of forgot about this PR. Let me see what we can do here. Feel free to ping me if I forget again.

rrousselGit avatar Nov 18 '24 18:11 rrousselGit

Friendly Reminder to take a look at this PR ๐Ÿ˜Š

If there is a way I can help testing it, just let me know!

Yegair avatar Jan 06 '25 08:01 Yegair

This is valuable.

Although I'm wondering whether we need a flag at all. Why not always make the generated class final if it helps? Extending a Freezed class isn't supported anyway, as they only have factory constructors.

Is this PR stil relevant as now freezed support "extends" ?

EArminjon avatar Feb 27 '25 08:02 EArminjon

Is this PR stil relevant as now freezed support "extends" ?

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 3.x.x.

Will take a closer look on the weekend!

Yegair avatar Feb 27 '25 11:02 Yegair

Sorry as you can see, I got sidetracked.
I'm not sure how relevant it is anymore. To be honest, looking back at the motivation, I wonder if the iDE isn't to blame instead.

I'm not sure whether changing the generated code makes sense, when analyzer likely should know already that the case doesn't make sense for a sealed class.

After-all, sealed classes are some form of final classes

rrousselGit avatar Feb 27 '25 14:02 rrousselGit

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

rrousselGit avatar Feb 27 '25 14:02 rrousselGit

Updated the implementation, should be ready to review

  • added new parameter @Freezed(makeGeneratedClassesFinal: true) which defaults to null.
  • if makeGeneratedClassesFinal is null, it should be treated as if it was false, to not introduce a breaking change
  • added a few more tests, mostly to see if it works with custom super-/sub-classes

To really test it, I integrated it with the project I am working on (which contains hundreds of freezed classes). However, due to https://github.com/RevenueCat/purchases-flutter/issues/1288 I am currently stuck on version 2.4.x. Had to add a dependency override for freezed_annotation: 3.0.0. If I'm not mistaken, it should not matter which version of freezed_annotation is present at runtime, since everything happens at build time. So my reasoning was that I can safely ignore any transitive freezed_annotation dependencies, because naturally those dependencies have already been built, but TBH I do still don't fully understand how build_runner works, so I am not entirely sure.

Yegair avatar Mar 16 '25 09:03 Yegair

Thinking about it, I wonder if this should be slightly generalised. We could have @Freezed(classKeywords: ['final']) This would support base/mixin/sealed/... (we'd need an assert error if the keyword is invalid)

I won't really want to move the goalpost forever though. But it feels like this change makes sense. Thought?

rrousselGit avatar Mar 16 '25 13:03 rrousselGit

I do like the idea of using a more general API. Of course we can just use a List<String>, but it should also be possible to do something like

@Freezed(classModifiers: [FreezedClassModifier.Final])
abstract class Foo with _$Foo {
  factory Foo() = _Foo;
}

Might be a little more developer friendly, because one would get autocomplete. However, due to all modifiers being keywords we either have to use UpperCamelCase (like in the example) or something like FreezedClassModifier.$final.

When it comes to which modifiers should be supported, I think everything that makes the class effectively abstract is problematic, for example:

@Freezed(classModifiers: [FreezedClassModifier.Sealed])
abstract class Foo with _$Foo {
  factory Foo() = _Foo;
               // ^^^^
               // The redirecting constructor 'Foo' can't redirect to a constructor of the abstract class '_Foo'.
}

So that would leave the modifiers final, mixin, base, unless there is a way of using abstract, sealed, interface on the generated classes that I am not aware of.


That being said, I do have one "concern" with that change. Allowing more modifiers might lead to people using Freezed in unintended ways, which in turn might make future features more difficult to implement without breaking stuff. However, that is just a gut feeling that I have, and I can not support it with examples.

If I would have to make the decision, I would probably start by allowing final and base only. Then I'd wait and see if anyone asks for more modifiers.

Yegair avatar Mar 16 '25 18:03 Yegair

Did a quick experiment on it. Let me know what you think of it. Not sure why CI is currently failing (something with Dart Format ๐Ÿค”), tests should still be passing though.

Yegair avatar Mar 16 '25 19:03 Yegair

Ah, I kind of forgot that we wouldn't be able to mark the generated class sealed. Even though Freezed has inheritance now, the factory constructor still wouldn't work.

So we'd only gain mixin/base. That's a bit limited, so probably not worth the trouble

Do you mind reverting your last commit? I'll merge this then. I've already reviewed the previous changes.

rrousselGit avatar Mar 16 '25 20:03 rrousselGit

Didn't explicitly mention it, but from my side this PR is ready. Did a revert commit for the last one, but if you want a clean history you can just squash everything into a single commit when merging

Yegair avatar Mar 24 '25 18:03 Yegair

@rrousselGit Just rebased this one onto the latest master. Do you think it could be merged? Would love to get rid of my forked freezed ๐Ÿ˜…

Yegair avatar Aug 17 '25 07:08 Yegair