sdk
sdk copied to clipboard
Provide analyzer support for verifying `copyWith`-style implementations.
In the absence of a good way to copy immutable objects with some field changes, the Flutter team often implements a copyWith
function that copies the original object with optional overrides of individual fields. It's invaluable for being able to work with immutable classes.
However, in the implementation, it's really easy to fat finger something and not hook it up correctly, resulting in possible catastrophic bugs. We try to write unit tests to handle this, but sometimes even then the test can miss it if we choose the wrong test.
For instance, an example implementation looks like this:
@override
StarBorder copyWith({
BorderSide? side,
double? points,
double? innerRadiusRatio,
double? pointRounding,
double? valleyRounding,
double? rotation,
double? squash,
}) {
return StarBorder(
side: side ?? this.side,
points: points ?? this.points,
rotation: rotation ?? this.rotation,
innerRadiusRatio: innerRadiusRatio ?? this.innerRadiusRatio,
pointRounding: valleyRounding ?? this.pointRounding,
valleyRounding: valleyRounding ?? this.valleyRounding,
squash: squash ?? this.squash,
);
}
Can you spot the error? If you're looking for it, it's pretty obvious, but it is easy to miss in a review. (pointRounding
is being assigned the value of valleyRounding
).
The unnecessary_this
analysis warning can sometimes help here if you've renamed a field but not the name of the parameter in the copyWith
(another common error), but it doesn't catch the case above.
Ideally there'd be a general lint that could check all instances of a copyWith
for these kinds of errors, but I realize that the variation in implementations probably makes that impossible.
Are there any good ways to improve the situation? Could we have an annotation that a parameter must be used in the implementation? Could we annotate these kinds of copying functions and force them to conform to a pattern (must accept a parameter with the same name as all public fields, must use all parameters in result, target parameters must match source parameters, etc.)? Could we have a way of auto-generating these copy functions instead of writing them?
I mean, it seems like the best solution is a language solution that lets us describe how to copy an immutable class with changes, but I don't know what that is, and it seems like it might be hard to come up with a good general solution. In the meantime, it would be good to be able to avoid these common errors.
/cc @bwilkerson
Could we have an annotation that a parameter must be used in the implementation?
That's certainly feasible, though it seems like a lot of work to annotate every parameter.
Could we annotate these kinds of copying functions and force them to conform to a pattern (must accept a parameter with the same name as all public fields, must use all parameters in result, target parameters must match source parameters, etc.)?
That's also doable, but so is a lint and the lint doesn't need to be added to every method. If a user doesn't like the way the lint works they don't have to enable it (though we might want to give it a name making it clear that it follow's Flutter's conventions for such a method and not necessarily everyone's conventions).
Could we have a way of auto-generating these copy functions instead of writing them?
Yes. There are at least three code generators on the first page of results using the query https://pub.dev/packages?q=generate+copyWith that purport to generate a copyWith
method.
I don't know what the technical issues would be with Flutter using a code generator, so I don't know whether that solution is feasible.
I mean, it seems like the best solution is a language solution ...
Generating a copyWith
method is one of the motivating use cases for the macros feature: https://github.com/dart-lang/language/blob/eba8f41b2f06b229c59216c2e37aa3050aa46645/working/macros/motivation.md#L173.
It's also being discussed as a possibility for some other language features, such as https://github.com/dart-lang/language/blob/eb9a58d8a6b4204f15cfe5bd044b60897305b6fe/working/extension_structs/overview.md.
But you're right, neither of these will be supported anytime soon, even if they're approved at some future date.