[ddc] Add rejection for removed const fields

Reject hot reload requests that remove fields from const classes. This
is consistent with the VM implementation.

Change-Id: I73ff62795abbca62565ee6efd9cb245485d010ac
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/403321
Reviewed-by: Morgan :) <[email protected]>
Commit-Queue: Nicholas Shahan <[email protected]>
Reviewed-by: Nate Biggs <[email protected]>

Author: nshahan

pkg/dev_compiler/lib/src/kernel/hot_reload_delta_inspector.dart

@@ -34,7 +34,10 @@ class HotReloadDeltaInspector {
           // No previous version of the class to compare with.
           continue;
         }
-        _checkConstClassConsistency(acceptedClass, deltaClass);
+        if (acceptedClass.hasConstConstructor) {
+          _checkConstClassConsistency(acceptedClass, deltaClass);
+          _checkConstClassDeletedFields(acceptedClass, deltaClass);
+        }
       }
     }
     return _rejectionMessages;
@@ -46,10 +49,36 @@ class HotReloadDeltaInspector {
   /// [acceptedClass] and [deltaClass] must represent the same class in the
   /// last known accepted and delta components respectively.
   void _checkConstClassConsistency(Class acceptedClass, Class deltaClass) {
-    if (acceptedClass.hasConstConstructor && !deltaClass.hasConstConstructor) {
+    assert(acceptedClass.hasConstConstructor);
+    if (!deltaClass.hasConstConstructor) {
       _rejectionMessages.add('Const class cannot become non-const: '
           "Library:'${deltaClass.enclosingLibrary.importUri}' "
           'Class: ${deltaClass.name}');
     }
   }
+
+  /// Records a rejection error when [acceptedClass] and [deltaClass] are both
+  /// const but fields have been removed from [deltaClass].
+  ///
+  /// [acceptedClass] and [deltaClass] must represent the same class in the
+  /// last known accepted and delta components respectively.
+  void _checkConstClassDeletedFields(Class acceptedClass, Class deltaClass) {
+    assert(acceptedClass.hasConstConstructor);
+    if (!deltaClass.hasConstConstructor) {
+      // Avoid reporting errors when fields are removed but the delta class is
+      // also no longer const. That is already reported by
+      // [_checkConstClassConsistency].
+      return;
+    }
+    // Verify all fields are still present.
+    final acceptedFields = {
+      for (var field in acceptedClass.fields) field.name.text
+    };
+    final deltaFields = {for (var field in deltaClass.fields) field.name.text};
+    if (acceptedFields.difference(deltaFields).isNotEmpty) {
+      _rejectionMessages.add('Const class cannot remove fields: '
+          "Library:'${deltaClass.enclosingLibrary.importUri}' "
+          'Class: ${deltaClass.name}');
+    }
+  }
 }

pkg/dev_compiler/test/hot_reload_delta_inspector_test.dart

@@ -155,6 +155,108 @@ Future<void> main() async {
           await compileComponents(initialSource, deltaSource);
       expect(deltaInspector.compareGenerations(initial, delta), isEmpty);
     });
+    test('rejection when removing a field', () async {
+      final initialSource = '''
+          var globalVariable;
+
+          class A {
+            final String s, t, w;
+            const A(this.s, this.t, this.w);
+          }
+
+          main() {
+            globalVariable = const A('hello', 'world', '!');
+            print(globalVariable.s);
+          }
+          ''';
+      final deltaSource = '''
+          var globalVariable;
+
+          class A {
+            final String s, t;
+            const A(this.s, this.t);
+          }
+
+          main() {
+            print('hello world');
+          }
+          ''';
+      final (:initial, :delta) =
+          await compileComponents(initialSource, deltaSource);
+      expect(
+          deltaInspector.compareGenerations(initial, delta),
+          unorderedEquals([
+            'Const class cannot remove fields: '
+                "Library:'memory:///main.dart' Class: A"
+          ]));
+    });
+    test('rejection when removing a field while adding another', () async {
+      final initialSource = '''
+          var globalVariable;
+
+          class A {
+            final String s, t, w;
+            const A(this.s, this.t, this.w);
+          }
+
+          main() {
+            globalVariable = const A('hello', 'world', '!');
+            print(globalVariable.s);
+          }
+          ''';
+      final deltaSource = '''
+          var globalVariable;
+
+          class A {
+            final String s, t, x;
+            const A(this.s, this.t, this.x);
+          }
+
+          main() {
+            print('hello world');
+          }
+          ''';
+      final (:initial, :delta) =
+          await compileComponents(initialSource, deltaSource);
+      expect(
+          deltaInspector.compareGenerations(initial, delta),
+          unorderedEquals([
+            'Const class cannot remove fields: '
+                "Library:'memory:///main.dart' Class: A"
+          ]));
+    });
+    test('no error when removing field while also making class const',
+        () async {
+      final initialSource = '''
+          var globalVariable;
+
+          class A {
+            final String s, t, w;
+            A(this.s, this.t, this.w);
+          }
+
+          main() {
+            globalVariable = A('hello', 'world', '!');
+            print(globalVariable.s);
+          }
+          ''';
+      final deltaSource = '''
+          var globalVariable;
+
+          class A {
+            final String s, t;
+            const A(this.s, this.t);
+          }
+
+          main() {
+            print('hello world');
+          }
+          ''';
+      final (:initial, :delta) =
+          await compileComponents(initialSource, deltaSource);
+      expect(() => deltaInspector.compareGenerations(initial, delta),
+          returnsNormally);
+    });
   });
 }
 

pkg/reload_test/lib/frontend_server_controller.dart

@@ -202,7 +202,8 @@ class HotReloadFrontendServerController {
 
   /// Clears the controller's state between commands.
   ///
-  /// Note: this does not reset the Frontend Server's state.
+  /// Note: this does not reset the Frontend Server's state and for that reason
+  /// the local [totalErrors] count is not reset here.
   void _clearState() {
     sources.clear();
     accumulatedOutput.clear();
@@ -227,6 +228,13 @@ class HotReloadFrontendServerController {
   Future<CompilerOutput> sendRecompile(String entrypointPath,
       {List<String> invalidatedFiles = const [],
       String boundaryKey = fakeBoundaryKey}) async {
+    // Currently the `FrontendCompiler` used in this test suite clears errors
+    // before performing a recompile but not an initial compile. Since we reuse
+    // the same instance and issue an initial compile request for each test we
+    // must track the number of errors on our side carefully so we can tell when
+    // a new error has appeared. Resetting the [totalErrors] count here should
+    // match the state on the FES side.
+    totalErrors = 0;
     if (!started) throw Exception('Frontend Server has not been started yet.');
     _state = FrontendServerState.awaitingResult;
     final command = 'recompile $entrypointPath $boundaryKey\n'