Fix deserialization codegen with manually implemented builder.

The generated code now checks at runtime if the builder method auto-instantiated the nested builder.

Add `FieldMixin` to reuse code between value generator and serializer generator.

Author: davidmorgan

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 # 8.4.3 (unreleased)
 
+- Fix generated deserialization code when there is a manually written builder
+  with nullable fields.
 - Drop dev dependency on `quiver`.
 - Disable all linting of generated code.
 - Change generated hash code implementation so it formats better when there are

built_value_generator/lib/src/field_mixin.dart

@@ -0,0 +1,51 @@
+import 'package:analyzer/dart/analysis/results.dart';
+import 'package:analyzer/dart/ast/ast.dart';
+import 'package:analyzer/dart/element/element.dart';
+import 'package:built_value/built_value.dart';
+import 'package:built_value_generator/src/dart_types.dart';
+
+/// Common logic between `ValueSourceField` and `SerializerSourceField`.
+mixin FieldMixin {
+  FieldElement? get builderElement;
+  ParsedLibraryResult get parsedLibrary;
+
+  bool get builderFieldExists => builderElement != null;
+
+  /// Gets the type of the field in a manually written builder.
+  ///
+  /// Returns `dynamic` if there is no such field.
+  ///
+  /// Goes via the AST if needed. This happens if the type has not been
+  /// generated yet, which will be the case for a nested builder field if the
+  /// builder is in this same library.
+  @memoized
+  String get fullBuildElementType {
+    if (!builderFieldExists) return 'dynamic';
+
+    // Try to get a resolved type first, it's faster.
+    var result = DartTypes.tryGetName(builderElement!.getter?.returnType,
+        withNullabilitySuffix: true);
+
+    if (result != null && result != 'dynamic') return result;
+    // Go via AST to allow use of unresolvable types not yet generated;
+    // this includes generated Builder types.
+    result = parsedLibrary
+        .getElementDeclaration(builderElement!)
+        ?.node
+        .parent
+        ?.childEntities
+        .first
+        .toString();
+
+    if (result != null) return result;
+
+    result = builderElement!.getter != null
+        ? (parsedLibrary.getElementDeclaration(builderElement!.getter!)?.node
+                as MethodDeclaration?)
+            ?.returnType
+            .toString()
+        : null;
+
+    return result ?? 'dynamic';
+  }
+}

built_value_generator/lib/src/serializer_source_class.dart

@@ -309,6 +309,17 @@ abstract class SerializerSourceClass
 
   String generateSerializer() {
     if (isBuiltValue) {
+      // Add local $cast function if it will be used in code generated by
+      // _generateFieldDeserializers.
+      var needsCastFn = hasBuilder &&
+          fields.any((f) =>
+              f.builderFieldUsesNestedBuilder && f.builderFieldIsNullable);
+      var maybeCastFn = needsCastFn
+          ? r'''
+T $cast<T>(dynamic any) => any as T;
+'''
+          : '';
+
       return '''
 class $serializerImplName implements StructuredSerializer<$genericName> {
   @override
@@ -331,6 +342,7 @@ class $serializerImplName implements StructuredSerializer<$genericName> {
   $genericName deserialize(Serializers serializers, Iterable<Object$orNull> serialized,
       {FullType specifiedType = FullType.unspecified}) {
     ${_generateGenericsSerializerPreamble()}
+    $maybeCastFn
     ${fields.isEmpty ? 'return ${_generateNewBuilder()}.build();' : '''
     final result = ${_generateNewBuilder()};
 
@@ -516,11 +528,31 @@ class $serializerImplName implements PrimitiveSerializer<$genericName> {
       // If cast exists and is not nullable.
       var maybeNotNull = !field.isNullable && cast.isNotEmpty ? notNull : '';
       if (field.builderFieldUsesNestedBuilder) {
-        if (field.builderFieldAutoCreatesNestedBuilder || hasBuilder) {
+        if (hasBuilder && field.builderFieldIsNullable) {
+          // The manually implemented builder might or might not return a
+          // builder. If it does, use `replace` as usual. If not, use `$cast`
+          // to avoid computing a new type of static cast.
+          return '''
+case '${escapeString(field.wireName)}':
+  var maybeBuilder = result.${field.name};
+  var fieldValue = serializers.deserialize(
+      value, specifiedType: $fullType)$notNull $cast;
+  if (maybeBuilder == null) {
+    result.${field.name} = \$cast(fieldValue.toBuilder());
+  } else {
+    maybeBuilder.replace(fieldValue);
+  }
+  break;
+''';
+        } else if (field.builderFieldAutoCreatesNestedBuilder) {
+          // Use a looser cast where possible for calls to `replace`.
+          final looseCast = field.generateCast(
+              compilationUnit, _genericBoundsAsMap,
+              forReplace: true);
           return '''
 case '${escapeString(field.wireName)}':
   result.${field.name}.replace(serializers.deserialize(
-      value, specifiedType: $fullType)$notNull $cast);
+      value, specifiedType: $fullType)$notNull $looseCast);
   break;
 ''';
         } else {

built_value_generator/lib/src/serializer_source_field.dart

@@ -12,13 +12,15 @@ import 'package:analyzer/dart/element/type.dart';
 import 'package:built_collection/built_collection.dart';
 import 'package:built_value/built_value.dart';
 import 'package:built_value_generator/src/dart_types.dart';
+import 'package:built_value_generator/src/field_mixin.dart';
 import 'package:built_value_generator/src/metadata.dart'
     show metadataToStringValue;
 import 'package:built_value_generator/src/strings.dart';
 
 part 'serializer_source_field.g.dart';
 
 abstract class SerializerSourceField
+    with FieldMixin
     implements Built<SerializerSourceField, SerializerSourceFieldBuilder> {
   static final BuiltMap<String, String> typesWithBuilder =
       BuiltMap<String, String>({
@@ -29,8 +31,10 @@ abstract class SerializerSourceField
     'BuiltSetMultimap': 'SetMultimapBuilder',
   });
   BuiltValue get settings;
+  @override
   ParsedLibraryResult get parsedLibrary;
   FieldElement get element;
+  @override
   FieldElement? get builderElement;
 
   factory SerializerSourceField(
@@ -142,11 +146,12 @@ abstract class SerializerSourceField
   }
 
   @memoized
-  bool get builderFieldUsesNestedBuilder {
-    var builderFieldElementIsValid =
-        (builderElement?.getter?.isAbstract == false) &&
-            !builderElement!.isStatic;
+  bool get builderFieldElementIsValid =>
+      (builderElement?.getter?.isAbstract == false) &&
+      !builderElement!.isStatic;
 
+  @memoized
+  bool get builderFieldUsesNestedBuilder {
     // If the builder is present, check it to determine whether a nested
     // builder is needed. Otherwise, use the same logic as built_value when
     // it decides whether to use a nested builder.
@@ -157,6 +162,19 @@ abstract class SerializerSourceField
             DartTypes.needsNestedBuilder(element.getter!.returnType);
   }
 
+  @memoized
+  @override
+  String get fullBuildElementType => super.fullBuildElementType;
+
+  @memoized
+  bool get builderFieldIsNullable {
+    if (!isNonNullByDefault) return true;
+    if (!builderFieldElementIsValid) return true;
+
+    return fullBuildElementType == 'dynamic' ||
+        fullBuildElementType.endsWith('?');
+  }
+
   @memoized
   bool get builderFieldAutoCreatesNestedBuilder =>
       builtValueField.autoCreateNestedBuilder ??
@@ -192,10 +210,15 @@ abstract class SerializerSourceField
   ///
   /// Generics are cast to the bound of the generic. If there is no bound,
   /// no cast is needed, and an empty string is returned.
+  ///
+  /// Set [forReplace] to loosen generics of cast where possible for calling
+  /// a `replace` method.
   String generateCast(CompilationUnitElement compilationUnit,
-      BuiltMap<String, String> classGenericBounds) {
+      BuiltMap<String, String> classGenericBounds,
+      {bool forReplace = false}) {
     var result = _generateCast(
-        typeInCompilationUnit(compilationUnit), classGenericBounds);
+        typeInCompilationUnit(compilationUnit), classGenericBounds,
+        forReplace: forReplace);
     return result == 'Object' ? '' : 'as $result';
   }
 
@@ -236,16 +259,19 @@ abstract class SerializerSourceField
   }
 
   String _generateCast(String type, BuiltMap<String, String> classGenericBounds,
-      {bool topLevel = true}) {
+      {bool topLevel = true, bool forReplace = false}) {
     var resultNullabilitySuffix = (!topLevel && type.endsWith('?')) ? '?' : '';
     var bareType = _getBareType(type);
 
     // `built_collection` `replace` methods don't care about the full generic
     // type, so we can be less precise about the cast. This doesn't add any
     // particular value for vanilla `built_value` but it allows plugging in
     // serializers that don't get the generic types correct.
+    //
+    // Only if `forReplace` was passed.
     String generics;
-    if (topLevel &&
+    if (forReplace &&
+        topLevel &&
         DartTypes.isBuiltCollectionTypeName(bareType) &&
         builderFieldUsesNestedBuilder) {
       if (bareType == 'BuiltList' || bareType == 'BuiltSet') {

built_value_generator/lib/src/serializer_source_field.g.dart

@@ -12,6 +12,7 @@ import 'package:analyzer/dart/element/type.dart';
 import 'package:built_collection/built_collection.dart';
 import 'package:built_value/built_value.dart';
 import 'package:built_value_generator/src/dart_types.dart';
+import 'package:built_value_generator/src/field_mixin.dart';
 import 'package:built_value_generator/src/fixes.dart';
 import 'package:built_value_generator/src/fields.dart' show collectFields;
 import 'package:built_value_generator/src/metadata.dart'
@@ -28,10 +29,13 @@ const _suggestedTypes = <String, String>{
 };
 
 abstract class ValueSourceField
+    with FieldMixin
     implements Built<ValueSourceField, ValueSourceFieldBuilder> {
   BuiltValue get settings;
+  @override
   ParsedLibraryResult get parsedLibrary;
   FieldElement get element;
+  @override
   FieldElement? get builderElement;
 
   factory ValueSourceField(
@@ -134,9 +138,6 @@ abstract class ValueSourceField
             annotation.getField('autoCreateNestedBuilder')?.toBoolValue());
   }
 
-  @memoized
-  bool get builderFieldExists => builderElement != null;
-
   @memoized
   bool get builderFieldIsNormalField =>
       builderFieldExists &&
@@ -156,39 +157,12 @@ abstract class ValueSourceField
       builderElement!.getter!.isAbstract;
 
   @memoized
-  String get _fullBuildElementType {
-    if (!builderFieldExists) return 'dynamic';
-
-    // Try to get a resolved type first, it's faster.
-    var result = DartTypes.tryGetName(builderElement!.getter?.returnType,
-        withNullabilitySuffix: true);
-
-    if (result != null && result != 'dynamic') return result;
-    // Go via AST to allow use of unresolvable types not yet generated;
-    // this includes generated Builder types.
-    result = parsedLibrary
-        .getElementDeclaration(builderElement!)
-        ?.node
-        .parent
-        ?.childEntities
-        .first
-        .toString();
-
-    if (result != null) return result;
-
-    result = builderElement!.getter != null
-        ? (parsedLibrary.getElementDeclaration(builderElement!.getter!)?.node
-                as MethodDeclaration?)
-            ?.returnType
-            .toString()
-        : null;
-
-    return result ?? 'dynamic';
-  }
+  @override
+  String get fullBuildElementType => super.fullBuildElementType;
 
   @memoized
   String get buildElementType {
-    var result = _fullBuildElementType;
+    var result = fullBuildElementType;
     // If we went via the AST there may be an import prefix, but we don't
     // want one here. Strip it off.
     if (result.contains('.')) {
@@ -197,7 +171,7 @@ abstract class ValueSourceField
     return _removeNullabilitySuffix(result);
   }
 
-  bool get builderElementTypeIsNullable => _fullBuildElementType.endsWith('?');
+  bool get builderElementTypeIsNullable => fullBuildElementType.endsWith('?');
 
   bool get builderElementSetterIsNullable {
     if (!builderFieldExists) return false;