feat: add common properties with different nullability to base mixin

[DevNico]

The new field isCommonWithDifferentNullability might not be perfect but it was the best way I could come up with to propagate the information.

[DevNico]

In theory, we could also add all properties that are not common in all constructors and do not have a type conflict to the base mixin as nullable.

What do you think? @rrousselGit?

[DevNico]

Also instead of removing properties with different nullability, we could add them but only allow non-null inputs.

[DevNico]

Also instead of removing properties with different nullability, we could add them but only allow non-null inputs.

I went ahead and implemented that feature as well

[TimWhiting]

I don't think this closes #715 which is requesting for a common base class, whereas this just addresses the nullability aspect. This is a great first step though!

[DevNico]

You're right I read that issue a while ago and needed this specific feature now so I went ahead and implemented it. I vaguely remembered reading something about it and therefore just quickly added the closing statement without re-reading the original issue.

Removed the closing statement since it is out of scope for what I currently require and would also require a lot more work.

[rrousselGit]

Hello! Thanks for your work!

Supporting cases like:

factory Foo.first(int a) = _First;
factory Foo.second(double a) = _Second;
...


Foo foo;
num a = foo.a;

should not be as complex as I originally thought.

I later learned about TypeSystem/TypeProvider (which can be found on LibraryElement I believe). They provide utilities find the nearest common type between two properties

It's probably worth using that directly instead.

[DevNico]

If you haven't started working on that I can give it a go and try to refactor this PR to use TypeSystem/TypeProvider

[rrousselGit]

Sure, go ahead 😄

[DevNico]

Properties with a common super type that isn't Object now have getters in the base mixin. 🎉

I have not included them in the copyWith since we can only upcast to the common supertype but not downcast.

[DevNico]

I also noticed that the super constructors could be refactored to make use of the super. shorthand. Is that something you'd be alright with? I could do that in this PR or open another one for those changes.

[rrousselGit]

Great!

The super syntax isn't used yet because Freezed supports an sdk version lower than 2.17

It's not very important considering the code is generated

[rrousselGit]

I'm a bit busy until the Flutter Viking talks the 1st of September. Not sure I'll be able to properly review this PR before

But thanks a lot for your work 😊

[TimWhiting]

@rrousselGit I'm happy to help review PRs if you are busy with other things. Excited for this PR!

[rrousselGit]

@TimWhiting Sure go ahead!

I should be a bit more available by September.

[DevNico]

@rrousselGit all two types have Object / Object? as a common supertype (except for dynamic maybe? I'd have to check that) would you say it is desirable to have those parameters in the common base mixin as Object?

As an example take:

factory Foo.first(String a) = _First;
factory Foo.second(double a) = _Second;

Foo foo;
Object a = foo.a;

Also, would a parameter be in the base mixin if all or one was typed with Object initially?

[rrousselGit]

If all the cases possess the same variable, I think showing Object/Object? is fine.
No dynamic though, that's for sure.

[rrousselGit]

There are a few conflicts and some small tests changes. But otherwise LGTM!

Thanks for your work 😄

[rrousselGit]

Looking back at the logic for obtaining the common subtype between union cases, the logic seems to do more than necessary

Typedefs and generated types are not supported by Freezed in general. Trying to support it for this specific case doesn't make sense, as they are still unsupported for everything else.

[rrousselGit]

Alright, time to merge this!

Sorry for the delay :)