Floor compatibility

[ryanheise]

Is your feature request related to a problem? Please describe.

In a past issue, @rrousselGit provided an example of how to integrate with Floor:

#314 (comment)

@freezed
abstract class MyTable with _$MyTable {
  @Entity()
  factory MyTable(String id) = Table
}

@Database(entities: [      Table    ])
abstract class AppDatabase extends FloorDatabase { ... }

At the time, this solution worked because the generated class Table was concrete as required by Floor. At some point after that, the generator was changed to produce Table as an abstract class, and this broke compatibility with Floor.

Describe the solution you'd like

A way to name and annotate a concrete data class. The stable release date of macros is unknown but it looks like it won't arrive until next year, so until then it would be nice to have an option similar to the old approach.

Describe alternatives you've considered

One workaround is mentioned in #314 although it involves manually writing a mirror constructor for Floor which adds to the maintenance burden.

Additional context

None

[rrousselGit]

I'd suggest asking Floor to change instead of Freezed.

I'd rather not have a feature just for one package

[ryanheise]

Thanks for your response, @rrousselGit .

Floor is certainly not the only package that treats data classes as concrete, json_serializable also works on concrete data classes, and it's a fairly widespread practice beyond.

I'd rather not have a feature just for one package

Precisely! I think it is indeed unfortunate that freezed treats json_serializable as a special case in the surface API, and I wondered whether it might have been able to support both floor and json_serializable in a more general way, in terms of surface API, if it had supported concrete data classes which are widely used (not least of which by floor and json_serializable). For example:

@freezed // generate data class code
class MyTable with _$MyTable {
  @Entity()           // generate database code
  @JsonSerializable() // generate json code
  factory MyTable(String id) = Table
}

You're much closer to the nexus of all of this to know whether or not this is feasible, however my preference would be to have a separation of concerns between freezed, floor, json_serializable and other generators, so that we can mix together multiple code generators on the same class, one concerning json, one concerning DB and one concerning the unique things that freezed brings to the table. Thus, allowing the app developer the freedom to mix and match their preferred substitute packages for each of these separate concerns.

Similarly, looking ahead to a post-macro world in which this will in fact be based on concrete classes, what would be nice to see is if we could mix in different macros on the same concrete class to handle separate concerns.

And it seems that supporting concrete data classes would be a way to maximise this interoperability given that concrete data classes is a rather ubiquitous practice outside of freezed, and not just a floor thing.

I'd suggest asking Floor to change instead of Freezed.

With respect to data classes being concrete, Floor is currently more idiomatic than freezed, and presumably freezed WILL become more idiomatic after macros are introduced, so I don't think it makes sense to make Floor conform to freezed's current choice of implementation detail when it is planning to eventually move toward concrete classes anyway.

The two realistic options are a) wait for freezed to become more idiomatic post macros, or b) offer a solution for concrete data classes for the current version in the meantime.

P.S. Do you happen to have a link to the design document on Dart macros? I'd like to stress the importance of interoperability between multiple macro packages applied to the same class.

[rrousselGit]

With respect to data classes being concrete, Floor is currently more idiomatic than freezed

According to who? :)
There's no reason to force an annotated class to be concrete. Interfaces with Redirecting factories are a very widely used way of defining classes.

Macros aren't going to change much here. The abstract class is used to ensure that things like copyWith and stuff are type-safe. It's likely going to stay there.
I see no reason why you think that macros would change something here :)

[ryanheise]

Macros aren't going to change much here. The abstract class is used to ensure that things like copyWith and stuff are type-safe. It's likely going to stay there.

For copyWith, you do not need to create a subclass of the dataclass itself, you can create a subclass of the function type that is assigned to copyWith without forcing the dataclass itself to be abstract. For sealed classes, it obviously introduces an abstract superclass with concrete subclasses, but it is the concrete subclasses that need to be mapped to DB tables, and that is where the concrete part comes in again. It is these concrete subclasses where integration with Floor is a relevant consideration.

(Aside: When you refer to union types, you are really just describing sealed classes and pattern matching which are not the same thing as union types. I mean, at the type system level, this is a supertype, and we've always had those. We do have FutureOr and nullable types.)

You didn't actually respond to my point about json_serializable also operating on concrete data classes, and if you supported concrete data classes, you might be able to handle both json_serializable and floor using a unified surface syntax instead of making an exception for json_serializable, which it seems you admitted was not a good idea.

According to who? :)

According to the Dart language tour and Dart style guide, this is the idiomatic form:

class Person {
  String name;
  int age;

  Person({required this.name, required this.age});

 // toString, ==, hashCode, copyWith omitted for brevity.
}

Or:

class Person {
  String name;
  int age;

  Person(this.name, this.age);

 // toString, ==, hashCode, copyWith omitted for brevity.
}

This is also exactly Floor supports.

Interfaces with Redirecting factories are a very widely used way of defining classes.

What I'm hearing from you is that Floor should change but freezed should not change. But by your logic, if a way of defining classes is very widely used, then a package should support it. The idiomatic form of defining classes above is much more widely used than redirecting factories, and so there is much more of a reason to support it. So while you could say that there is reason for Floor to change, there is even more reason for freezed to change.

On the idiomatic point, while we can grant that redirecting factories are a legitimate pattern in SOME cases, we can't call it idiomatic in circumstances when the only reason we are doing it is because we are implementing a hack to fit into what's possible in the build_runner system which is forcing us to write contrived code. Nobody really "wants" to assign to a factory some internal symbol that doesn't even exist yet until after the build_runner has run, and which is supposed to be an internal implementation detail that never gets exposed, but we are forced to write such contrived code because we are trying to automatically generate Dart code in a build system that doesn't have first-class support for code generation. And of course with macros, you don't have to expose this internal symbol anymore in the surface syntax, data classes can now be "less" contrived and therefore "more" idiomatic.

I see no reason why you think that macros would change something here :)

Macros certainly change this because they add first-class support in the language for what we were originally trying to do, allowing code to be less contrived and therefore more idiomatic. The first-class support for code generation should make it easier for the implementation to represent data classes as plain concrete classes, and to represent sealed classes as sealed superclasses with concrete subclasses. This would cause the fewest surprises when interoperating with other packages and other codebases.

[rrousselGit]

You keep talking about "idiomatic data classes", but there's no such thing in Dart. Data classes aren't a feature yet, and Freezed is trying to implement it.
If anything, in the Flutter community Freezed is one of the idiomatic ways to define a data class + sealed hierarchy.

To begin with, given:

@freezed
class MyClass with _$MyClass {
  @Foo()
  factory MyClass({@Foo() String? a, int? b}) = _MyClass;
}

Then the following will be in the generated code:

@Foo()
class _$MyClassImpl implements _MyClass {
  _$MyClassImpl({@Foo() this.a, this.b});

  @override
  @Foo()
  final String? a;
  ...
}

That's a "concrete class" which should match your standard.

In that sense, you've yet to explain why that doesn't work.
Freezed already should support any package, because it transposes annotations applied on the @freezed class to the generated code.

If there are some cases that don't work, we need a clear description about why that's the case.
Nothing changed with how Freezed handles its generated code. So if the previous solution given worked for Floor, then it should keep working. If it doesn't anymore, then either Freezed or Floor introduced a bug.
I still see the annotation in Freezed's generated code, so I can only assume that Floor changed something.

[ryanheise]

You keep talking about "idiomatic data classes", but there's no such thing in Dart.

I'd rather you address the underlying point, but since this is pedantry, let's move on.

Nothing changed with how Freezed handles its generated code.

But the generated class name in your very example above is clearly different from the example I linked to in my issue. In your #314 example, the user explicitly gives the name of the generated concrete class in the style of #50 , but in your latest example above, the explicitly given name is NOT of the generate concrete class but rather is the name of an abstract class. The generate concrete class has an internal, and UNDOCUMENTED, name. We can't depend on undocumented internal behaviour which could change at any time without needing to abide by semantic versioning.

It would feel safer if this were documented in the README so that we know that the author purposely considered how this package an integrate with code that depends on access to the concrete class. And hopefully with the macro version, the surface level class "just works" as expected without needing to depend on some internal implementation details.

[rrousselGit]

I'm not saying you should write @Database(entities: [_$TableImpl]). But based on #314 (comment), writing @Database(entities: [Table]) worked for them.

And the generated Table as always been abstract.

So the question remains: What changed and why isn't this working anymore?

I'm also not aware of what the error is nor the reason for that error.
What prints in your console when you pass the abstract class to @Database, and
why does Floor reject abstract classes with factory constructors?

[rrousselGit]

I'd rather you address the underlying point, but since this is pedantry, let's move on.

IMO this is a fairly important point in this issue.
The core of the problem is about who should respect who's conventions.

You're arguing that Freezed should respect Dart's convention, and therefore Freezed should adapt to Floor.
I'm saying that there's no Dart convention for what Freezed tries to do, and Freezed is the one introducing a convention. So Floor should adapt to Freezed.

For now, I have no justification why Floor couldn't accept abstract classes.

[rrousselGit]

Let's summarise things a bit:

• At the time, this solution worked because the generated class Table was concrete as required by Floor. At some point after that, the generator was changed to produce Table as an abstract class, and this broke compatibility with Floor.

  This is incorrect. Freezed has always generated an abstract class with redirecting factories.
  If that worked with Floor before, but it doesn't anymore, then it is Floor who changed ; not Freezed.

• Freezed doesn't need any special plumbing to work with json_serializable.
  Freezed only has logic for it to save a few lines of code, and help deal with sealed hierarchies (which json_serializable doesn't help with)

• Contrary to what you said, json_serializable doesn't reject abstract classes. It only cares about the constructor of a class.
  This is valid:

  @JsonSerializable()
  abstract class Example {
    factory Example(int a) = _Example;
    factory Example.fromJson(Map<String, dynamic> json) =>
        _$ExampleFromJson(json);
  
    int get a;
  }
  
  class _Example implements Example {
    const _Example(this.a);
  
    @override
    final int a;
  }

  ...Although Freezed doesn't rely on that when interacting with json_serializable. But it works ; which shows that Floor could reasonably do the same too (unless I miss one reason why Floor cannot support that).

---

From there, I think the only remaining argument is about the "What's the idiomatic way to define data classes".

So far, I'm of the opinion that what Freezed is generating isn't wrong.
I hear you when you complain about some compatibility issue. But I just fail to understand why Floor cannot support abstract classes with factory constructors. That's an equally common pattern in Dart (pretty much all of the Dart SDK is like that).

We could try and force Freezed to generate a concrete class. But I would need compelling reasons to do so.

Fwiw, I personally have plans for other Freezed features that may clash with making the generated class concrete.
For example, one of the commonly requested feature is to support copyWith as a method rather than a getter ; for the sake of implementing an interface that has a copyWith method.
That'd require an abstract class to be generated ; with factory constructors.

As such, I want good reasons before committing to anything.
But I don't know yet the true reason as to why abstract classes are a problem.

[ryanheise]

Let's look at your example from #314 :

@freezed
abstract class MyTable with _$MyTable {
  @Entity()
  factory MyTable(String id) = Table
}

@Database(entities: [      Table    ])
abstract class AppDatabase extends FloorDatabase { ... }

The reason this doesn't work is that the @Database annotation informs Floor that Table is a concrete class annotated with @Entity, having an "idiomatic" constructor to initialise the data fields (idiomatic in the sense I communicated before).

Table does not satisfy these requirements. #50 implied that it would, but it doesn't.

[rrousselGit]

Based on what you're saying, would moving the annotation from _$TableImpl to Table fix your issue?

[ryanheise]

Based on what I'm saying, obviously not.

[rrousselGit]

Why?
I've asked multiple times why Floor couldn't support abstract classes. You haven't answered yet

[ryanheise]

That's a bit unfair. You asked me:

Based on what you're saying, would moving the annotation from _$TableImpl to Table fix your issue?

And the answer to your question is no, based on what I said in the previous comment. Obviously. If that's not the question you intended to ask me, that's not my fault, I just gave you the answer to the question you asked. If you now want to know "why" the author of Floor designed it that way, go and ask the author of Floor, I can't speak for them. If you want to know "why" the author couldn't change it, maybe they could. But I've already addressed that point. If you want to know "why" mechanically an abstract class doesn't work in Floor, you can go and look at the source code of Floor and verify it for yourself that indeed it will not work with abstract classes. There's no way of getting around this unless either -- or both -- of you work toward interoperability, and pointing the finger at the other developer to change their package while you stand your ground is not an impressive response. Even if you persuade one developer to do it your way, maximum interoperability is achieved when each individual package developer takes it upon themselves to do their part. This is not mutually exclusive.

I've communicated the way Floor works to the extent that makes it possible to see why your proposal does not fix the issue, I've made a case for why supporting concrete classes would maximise interoperability, I've done all I can do. I give up, and will close the issue.

If you wish to pursue this on your own, you can, but I'll bow out. (it would be faster for me to write my own data class package that interoperates with concrete classes than it would take to persuade you of the value of this.)

[rrousselGit]

You're the one answering unfairly here.

You're coming here, asking for a big change that might conflict with other features. And when I ask questions, you dodge half of them, and refuse to answer a simple "why?".

If you do not know an answer to a question, that's fine. But you're the one asking for the feature here. If you want it to move forward, it's your responsibility to provide the information.
Have you even considered raising an issue on Floor to ask about abstract class support?

It isn't my job to contact Floor's author. It is yours. You're the one who wants changes.

---

Even if you persuade one developer to do it your way, maximum interoperability is achieved when each individual package developer takes it upon themselves to do their part. This is not mutually exclusive.

And I've already answered those concerns. Freezed is already interoperable with any package, thanks to it transposing annotations from the annotated class to the generated code.

If that aspect of Freezed is lacking, I've already expressed that I'm willing to change where the annotations are transposed.
I explicitly agreed that you shouldn't have to rely on _$Table.

I've communicated the way Floor works to the extent that makes it possible to see why your proposal does not fix the issue

No, you did not, because you didn't reply to my questions (besides one, where your answer what "no" with no explanation)

I asked many questions which are unanswered: #1131 (comment)
And I also repeatedly said that I'd need more concrete reasons as to why the generated class needs to be concrete.

Say tomorrow I made Table concrete, and another person came to me asking to make Table abstract because their package needs that. What now?

It is totally normal that I ask for information. What's abnormal is that you don't want to find the answers to the questions I ask.

[ryanheise]

For your information, the output when passing Table as an entity to Floor is remarkably uninformative, in that it says Table is not an entity. It doesn't tell you "why".

But in my previous answer, I dug up the source code and found the exact 3 requirements that Floor checks for in order to accept it as an entity. The class must:

1. be concrete
2. be annotated with @Entity
3. have an idiomatic constructor

Now when you came back and said, Ah, OK so if we just support only (2) will that "fix" the issue? Of course it wouldn't. If you think ignoring the other requirements would "fix" it, then you either misread my answer, or you don't understand what "requirements" are, or you're asking me this question with some implicit subtext which I haven't been able to guess because I am taking your question literally. When you ask a question that has such an obvious answer when taken literally, and you don't accept the literal answer, I'm truly at a loss. I don't know how best to help you. Of course you can blame me, which of course you did many times, but that's not helping me to understand exactly what miscommunication occurred.

As another example of miscommunication that results from my literal reading of your words, you claim that "Freezed is already interoperable with any package". Now take it or leave it, but if I were you, you might want to avoid absolute claims that Freezed is interoperable with ANY package, in the very issue where it has been reported that Freezed is not interoperable with Floor since the concrete class is hidden. But even still, as a claim, Wow. I mean even my packages are not interoperable with every package, and I go out of my way to make sure I implement things using the lowest common denominator that I can manage. The best I can try to do is to aim to maximise interoperability, but it's not possible to achieve 100% interoperability with everything. When you make a literal claim that you have achieved it, I think this is just representative of the sort of miscommunication we've been having, since I've been taking your questions very literally, and yet my understanding of what you said doesn't seem to match what you think it means.

Have I thought about contacting the other authors to perhaps support abstract classes? Of course I have. Not recently, but years ago when I first encountered this issue. But as it turns out, someone else already opened an issue on the Floor repository before me. The authors said this is difficult with the current implementation since it needs access to the implementation of the class. People have enquired on the status of this over the years, although the authors have reported the situation is still as it was. As I said, there are reasons to improve both packages individually. It would be nice if Floor was better. It would be nice if freezed was better. There is already an issue on Floor's repo to make it better. This issue here, was intended to be a similar issue about making freezed better.

Lastly, you proposed a thought experiment where tomorrow someone might request that the generated data classes be abstract because they need it for interoperability. But realistically that would never happen because access to the concrete class serves to the lowest common denominator. I don't think you will be able to contrive a realistic scenario where a new package arrives on the scene, interoperability with freezed is useful, BUT, the other package requires the concrete class to be hidden. Let's be clear, this concrete class exists, but hypothetically it must be inaccessible to everyone in order for this hypothetical package to work... Earlier, you proposed moving just the annotations up from the Impl class to Table. If you also moved the whole concrete Impl class itself up to Table, the issue would be resolved. That seems very feasible, but I digress, this is no longer an issue I have the energy to champion, and I will pursue an alternative.

Note: I've unsubscribed from the issue, so I will not be notified of any further comments (however, as a courtesy I will check tomorrow morning to ensure I've read any further followup, should there be one, although I will not add further to it). I think you've done excellent work on freezed, and want to express my appreciation for your hard work in order to build it. I am unsubscribing because I'm confident I've left you with enough information should you wish to pursue this, and more importantly, my time is short and I really need to get back to other productive matters.

[rrousselGit]

Now when you came back and said, Ah, OK so if we just support only (2) will that "fix" the issue? Of course it wouldn't. If you think ignoring the other requirements would "fix" it, then you either misread my answer, or you don't understand what "requirements" are, or you're asking me this question with some implicit subtext which I haven't been able to guess because I am taking your question literally.

The question is literal.

But bear in mind: There are more than one way to solve a problem. Freezed doesn't have to be responsible for all 3 points you listed.

2. is IMO the gist of the issue right now, and the most likely to get a change in Freezed.
3. is already working AFAIK (unless your meaning of "idiomatic constructor" implies something I'm missing ?)

The issue is 1). And what I've been trying to get from you by asking various questions is asking whether 1) could be tackled by Floor itself.
As I mentioned, json_serializable doesn't have that requirement for example.

As such, fixing 2) could be the only thing Freezed has to do for Floor support. And the rest of the logic would be done in Floor. So out of my scope.

It could be that that's still not good enough.
But as is, I don't think we've established that making generated classes concrete is the only solution to support Floor.

---

About interoperability, come on, try to be bit charitable here.
Obviously I never said Freezed supports 100% of the packages. You're putting words in my mouth.

I said that Freezed already has mechanisms for other packages to interop with. So if a package wants to, it can.

You pointed one possible issue (the transpotion of annotation happening on the _$Table instead of Table).
As I said, I wholeheartedly agree that this is a concern.

But for the case of making classes concrete, that's a whole different matter.
Freezed classes aren't even subclassable at the moment.

---

Of course I have. Not recently, but years ago when I first encountered this issue. But as it turns out, someone else already opened an issue on the Floor repository before me. The authors said this is difficult with the current implementation since it needs access to the implementation of the class.

That issue probably contains many valuable information for me.
A link early in our discussions would've probably been very helpful.

It would be nice if freezed was better. There is already an issue on Floor's repo to make it better. This issue here, was intended to be a similar issue about making freezed better.

I agree, and I respect that.
Although the discussion went on a bad path, let it be clear:
The time you spent on this issue is commendable.

It just feels like you were a bit fixated on the "Freezed should generate concrete classes", when I'm pretty sure there are alternatives.

I've said it before, but the point that you raised about annotations on _$Table vs Table later in the discussion is much more valuable feedback to me.
If the discussion didn't start around concrete VS abstract classes but rather as a broader description of the problem, we probably wouldn't be talking like that right now.

Ultimately, concrete VS abstract don't matter. The only thing that matters is whether Floor integration works.

Heck, Floor could hardcode "if , when @Entities([Table]) is found, optionally look for _$TableImpl".
I've seen some packages do that ... although it's obviously not that great.

I just don't want us to be fixated on one solution, when another could be out there.

Earlier, you proposed moving just the annotations up from the Impl class to Table. If you also moved the whole concrete Impl class itself up to Table, the issue would be resolved. That seems very feasible, but I digress, this is no longer an issue I have the energy to champion, and I will pursue an alternative

I don't think it is as simple as you're making it out to be.
As I mentioned, there's literally a PR opened right now about making the generated classes final — which could be incompatible with Floor (if it needs to subclass/implement the class).

Similarly one feature request is to support:

abstract class Interface {
  Interface copyWith({int age});
}

@freezed
class Person with _$Person implements Interface {
  < some constructors >
}

That too would be directly incompatible with making "Table" concrete.

There are many scenarios where interoperability with Floor could break based on other features.
Or reversly, interop with Floor could prevent the implementation of other Freezed features.. Or significantly increase the maintenance burden or the complexity of the package.

To begin with, very few people complained about Floor integration to me.
So the value of such change doesn't seem high, and the ramifications are possibly complex.
So of course I'm asking many questions and try to look for any possible alternertive.