From bff8d6c6a17e53581a51462b848089e7862d3607 Mon Sep 17 00:00:00 2001
From: Simon Binder <oss@simonbinder.eu>
Date: Fri, 27 Jan 2023 17:12:49 +0100
Subject: [PATCH] Fix nullability issues around record types

---
 .../analysis/resolver/shared/dart_types.dart  | 47 +++++++++----------
 .../dart/custom_row_classes_test.dart         |  2 +-
 .../drift/custom_row_classes_test.dart        |  2 +-
 drift_dev/test/writer/writer_test.dart        |  2 +-
 4 files changed, 24 insertions(+), 29 deletions(-)

diff --git a/drift_dev/lib/src/analysis/resolver/shared/dart_types.dart b/drift_dev/lib/src/analysis/resolver/shared/dart_types.dart
index 08ccb414..7385266e 100644
--- a/drift_dev/lib/src/analysis/resolver/shared/dart_types.dart
+++ b/drift_dev/lib/src/analysis/resolver/shared/dart_types.dart
@@ -47,6 +47,7 @@ ExistingRowClass? validateExistingClass(
       generateInsertable: generateInsertable,
       knownDriftTypes: knownTypes,
       typeProvider: library.typeProvider,
+      typeSystem: library.typeSystem,
     );
   }
 
@@ -238,14 +239,17 @@ ExistingRowClass defaultRecordRowClass({
   required List<DriftColumn> columns,
   required bool generateInsertable,
   required TypeProvider typeProvider,
+  required TypeSystem typeSystem,
   required KnownDriftTypes knownDriftTypes,
 }) {
   final type = typeProvider.createRecordType(
     positional: const [],
     named: [
       for (final column in columns)
-        MapEntry(column.nameInDart,
-            regularColumnType(typeProvider, knownDriftTypes, column)),
+        MapEntry(
+            column.nameInDart,
+            regularColumnType(
+                typeProvider, typeSystem, knownDriftTypes, column)),
     ],
   );
 
@@ -424,7 +428,8 @@ bool checkType(
   if (typeConverter != null) {
     expectedDartType = typeConverter.dartType;
     if (typeConverter.canBeSkippedForNulls && columnIsNullable) {
-      expectedDartType = typeProvider.makeNullable(expectedDartType);
+      expectedDartType =
+          typeProvider.makeNullable(expectedDartType, typeSystem);
     }
   } else {
     expectedDartType = typeProvider.typeFor(columnType, knownTypes);
@@ -440,18 +445,25 @@ bool checkType(
 }
 
 DartType regularColumnType(
-    TypeProvider typeProvider, KnownDriftTypes knownTypes, HasType type) {
+  TypeProvider typeProvider,
+  TypeSystem typeSystem,
+  KnownDriftTypes knownTypes,
+  HasType type,
+) {
   final converter = type.typeConverter;
   if (converter != null) {
     var dartType = converter.dartType;
     if (type.nullable && converter.canBeSkippedForNulls) {
-      return typeProvider.makeNullable(dartType);
+      return typeProvider.makeNullable(dartType, typeSystem);
     } else {
       return dartType;
     }
+  } else {
+    final typeForNonNullable = typeProvider.typeFor(type.sqlType, knownTypes);
+    return type.nullable
+        ? typeProvider.makeNullable(typeForNonNullable, typeSystem)
+        : typeForNonNullable;
   }
-
-  return typeProvider.typeFor(type.sqlType, knownTypes);
 }
 
 extension on TypeProvider {
@@ -499,24 +511,7 @@ extension CreateRecordType on TypeProvider {
     );
   }
 
-  DartType makeNullable(DartType type) {
-    if (type is InterfaceType) {
-      return type.element.instantiate(
-        typeArguments: type.typeArguments,
-        nullabilitySuffix: NullabilitySuffix.question,
-      );
-    } else if (type is NeverType) {
-      return nullType;
-    } else if (type is RecordType) {
-      return createRecordType(
-        positional: [for (final type in type.positionalFields) type.type],
-        named: [
-          for (final type in type.namedFields) MapEntry(type.name, type.type),
-        ],
-        nullabilitySuffix: NullabilitySuffix.question,
-      );
-    } else {
-      return type;
-    }
+  DartType makeNullable(DartType type, TypeSystem typeSystem) {
+    return typeSystem.leastUpperBound(type, nullType);
   }
 }
diff --git a/drift_dev/test/analysis/resolver/dart/custom_row_classes_test.dart b/drift_dev/test/analysis/resolver/dart/custom_row_classes_test.dart
index ad0c38c0..c09811c8 100644
--- a/drift_dev/test/analysis/resolver/dart/custom_row_classes_test.dart
+++ b/drift_dev/test/analysis/resolver/dart/custom_row_classes_test.dart
@@ -578,7 +578,7 @@ class Users extends Table {
             .having((e) => e.isRecord, 'isRecord', isTrue)
             .having((e) => e.targetClass, 'targetClass', isNull)
             .having((e) => e.targetType.toString(), 'targetType',
-                '({int id, String name, DateTime birthday})'),
+                '({DateTime birthday, int id, String name})'),
       );
       expect(table.nameOfRowClass, 'User');
     }, skip: requireDart('3.0.0-dev'));
diff --git a/drift_dev/test/analysis/resolver/drift/custom_row_classes_test.dart b/drift_dev/test/analysis/resolver/drift/custom_row_classes_test.dart
index 039a0746..f1c532c2 100644
--- a/drift_dev/test/analysis/resolver/drift/custom_row_classes_test.dart
+++ b/drift_dev/test/analysis/resolver/drift/custom_row_classes_test.dart
@@ -148,7 +148,7 @@ CREATE TABLE foo (
             .having((e) => e.isRecord, 'isRecord', isTrue)
             .having((e) => e.targetClass, 'targetClass', isNull)
             .having((e) => e.targetType.toString(), 'targetType',
-                '({String foo, int? bar})'),
+                '({int? bar, String foo})'),
       );
       expect(table.nameOfRowClass, 'FooData');
     }, skip: requireDart('3.0.0-dev'));
diff --git a/drift_dev/test/writer/writer_test.dart b/drift_dev/test/writer/writer_test.dart
index a524b11a..f4d92935 100644
--- a/drift_dev/test/writer/writer_test.dart
+++ b/drift_dev/test/writer/writer_test.dart
@@ -73,7 +73,7 @@ class Database {}
     checkOutputs({
       'a|lib/a.drift.dart': decodedMatches(allOf(
         contains(
-          'typedef User = ({int id, String name, DateTime? birthDate});',
+          'typedef User = ({DateTime? birthDate, int id, String name});',
         ),
         contains(
           'return (id: attachedDatabase.typeMapping.read(DriftSqlType.int, data[\'\${effectivePrefix}id\'])!, '