`copyWith` probably handles nulls incorrectly

[marcelgarus]

`copyWith` probably handles nulls incorrectly

[MarcelGarus]

Done. The newest version 2.0.0, already published on pub, generates a copy method allowing usage like this:

var freshApple = const Fruit(type: 'apple', color: 'green');
var someApple = freshApple.copy((fruit) => fruit..color = null);
var kiwi = someApple.copy((fruit) => fruit
  ..type = 'Kiwi'
  ..color = 'brown');

The definition looks like this:

/// Copies this [Fruit] with some changed attributes.
Fruit copy(void Function(MutableFruit mutable) changeAttributes) {
  assert(
      changeAttributes != null,
      "You called Fruit.copy, but didn't provide a function for changing "
      "the attributes.\n"
      "If you just want an unchanged copy: You don't need one, just use "
      "the original.");
  var mutable = this.toMutable();
  changeAttributes(mutable);
  return Fruit.fromMutable(mutable);
}

[MarcelGarus]

That is indeed true, because if a parameter to the method is null, the method doesn't know if it has been explicitly or implicitly set to null. I thought about this before but found no easy way to fix that.
Here are more complicated ways I considered:

Use mutable for copying
On the client code side you can of course convert it to a mutable, the set the fields, then re-convert it to the immutable. Doesn't change the fact that the copyWith method behaves unintuitive in some cases.

Use wrapper function
Change the signature to expect a function for nullable values (just like Iterable's firstWhere(..., orElse: () => otherValueWhichMayBeNull)).
However that leads to code that's a little bit uglier:

mySister = me.copyWith(
  firstName: 'Yvonne',
  photoUrl: () => null,
)

Default values
Third solution, add default values just like dart:ui's hashValues() function, which is defined as

const _default = Object();
int hashValues(Object arg1, [Object arg2 = _default, ...])

In the code you can check if _default == _arg2, so you can see if the user explicitly passed in null. Something like this would probably be the most elegant solution, but we'd need "sample objects" of all the Field's types as default parameter values and that's at least as far as I know impossible to achieve.

If anyone has any other suggestions on how to do this elegantly, you're welcome to share your thoughts.

[ildarsharafutdinov]

I guess there is also "use mutable in copyWith" option which is similar to builders in BuiltValue:

mySister = me.copyWith((mutableCopy) {
  mutableCopy.firstName = 'Yvonne';
  mutableCopy.photoUrl = null;
});

[MarcelGarus]

Oh true, that would basically be the first option with some syntactic sugar on top.
If you want to implement that and file a PR, that would be great.

[MarcelGarus]

Thought about this a bit more.
If you have a simple widget with only non-nullable fields, using a copyWith method with a builder would be unnecessary complex.
That's why I'd rather opt for a copyWith method, where you can change all the non-nullable fields as well as copyWithBuilder method that takes a builder.
Any opinions on that?

[ildarsharafutdinov]

What params will copyWith contain in case there are nullable fields in the class?
If it continues to accept nullables it will be error prone. If code-gen strips out nullables, it'll be weird.

So I'm in favor of keeping things simple with a single interface for mutations.

[MarcelGarus]

You're right.

What about we always have a builder method for mutations but if your class has only non-nullable fields, you can optionally explicitly ask for the copyWith method in the annotation, something like @DataClass(generateCopyWith=true).

And if the class contains nullable fields, that will be an error during code generation.

[ildarsharafutdinov]

I'm still in favor of copyWith((mutable) => ...) :)

E.g. requirements change and a field becomes nullable. A developer will need to remove generateCopyWith=true and then modify all fancy calls of copyWith.
Whereas copyWith((mutable) => ...) won't require changes in existing code.

[MarcelGarus]

Hmm that is indeed true...
Okay, I'll implement it that way.