[ddc] Add hot reload checks for instance methods

Handles errors and type changes on a hot reload when call sites
that were statically valid are retained and run after the reload.

Change-Id: I8bebbd7bc7acc97f55ff930e8f456f99146fbf21
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/440082
Reviewed-by: Mark Zhou <[email protected]>
Commit-Queue: Nicholas Shahan <[email protected]>
Reviewed-by: Nate Biggs <[email protected]>

Author: nshahan

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

@@ -6551,6 +6551,19 @@ class LibraryCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
     // Otherwise generate this as a normal typed method call.
     var jsName = _emitMemberName(name, member: target);
     var invocation = js.call('#.#(#)', [jsReceiver, jsName, jsArguments]);
+    if (_shouldRewriteInvocationWithHotReloadChecks(target)) {
+      var checkedInvocation = _rewriteInvocationWithHotReloadChecks(
+        jsReceiver,
+        jsName,
+        target,
+        node.arguments,
+        node.getStaticType(_staticTypeContext),
+        _nodeStart(node),
+      );
+      // As an optimization, avoid extra checks when the invocation code was
+      // compiled in the same generation that it is running.
+      return _emitHotReloadSafeInvocation(invocation, checkedInvocation);
+    }
     return _isNullCheckableJsInterop(node.interfaceTarget)
         ? _wrapWithJsInteropNullCheck(invocation)
         : invocation;
@@ -7636,6 +7649,8 @@ class LibraryCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
       ..sourceInformation = _nodeStart(node);
     if (_shouldRewriteInvocationWithHotReloadChecks(target)) {
       var checkedCall = _rewriteInvocationWithHotReloadChecks(
+        fn.receiver,
+        fn.selector,
         target,
         node.arguments,
         node.getStaticType(_staticTypeContext),
@@ -7678,6 +7693,10 @@ class LibraryCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
   /// time to include checks to preserve soundness in the presence of hot
   /// reloads at runtime.
   ///
+  /// The compiled JavaScript [receiver] and [selector] should be passed so that
+  /// they can be reused for the validated invocation after the checks have
+  /// passed.
+  ///
   /// The checks are similar to the those performed when making a dynamic call.
   /// The [arguments] are checked for the correct shape and runtime types.
   /// Additionally after the invocation, the returned value is checked against
@@ -7689,7 +7708,9 @@ class LibraryCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
   /// The resulting expression for the validated call site will receive the
   /// [originalCallSiteSourceLocation].
   js_ast.Expression _rewriteInvocationWithHotReloadChecks(
-    Member target,
+    js_ast.Expression receiver,
+    js_ast.Expression selector,
+    Procedure target,
     Arguments arguments,
     DartType expectedReturnType,
     SourceLocation? originalCallSiteSourceLocation,
@@ -7738,8 +7759,7 @@ class LibraryCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
     // still valid.
     var checkResult = _emitScopedId('\$result');
     _letVariables!.add(checkResult);
-    var jsTarget = _emitStaticTarget(target);
-    var jsTypeArguments = [
+    var typeArguments = [
       // TODO(nshahan): Remove this check if we stop rewriting calls to SDK
       // functions.
       if (_reifyGenericFunction(target))
@@ -7748,9 +7768,9 @@ class LibraryCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
     var correctnessCheck = _runtimeCall(
       'hotReloadCorrectnessChecks(#, #, #, #, #)',
       [
-        jsTarget.receiver,
-        jsTarget.selector,
-        js_ast.ArrayInitializer(jsTypeArguments),
+        receiver,
+        selector,
+        js_ast.ArrayInitializer(typeArguments),
         js_ast.ArrayInitializer(hoistedPositionalVariables),
         hoistedNamedVariables.isEmpty
             ? js_ast.LiteralNull()
@@ -7766,14 +7786,18 @@ class LibraryCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
         : js_ast.Binary(',', letAssignments, checkAssignment);
     // Create a new invocation of the original target but passing all the
     // arguments via their let variables.
-    var validatedCallSite = js_ast.Call(jsTarget, [
-      ...jsTypeArguments,
-      ...hoistedPositionalVariables,
-      if (hoistedNamedVariables.isNotEmpty)
-        js_ast.ObjectInitializer([
-          for (var e in hoistedNamedVariables.entries)
-            js_ast.Property(js.string(e.key), e.value),
-        ]),
+    var validatedCallSite = js.call('#.#(#)', [
+      receiver,
+      selector,
+      [
+        ...typeArguments,
+        ...hoistedPositionalVariables,
+        if (hoistedNamedVariables.isNotEmpty)
+          js_ast.ObjectInitializer([
+            for (var e in hoistedNamedVariables.entries)
+              js_ast.Property(js.string(e.key), e.value),
+          ]),
+      ],
     ])..sourceInformation = originalCallSiteSourceLocation;
     // Cast the result of the checked call or the value returned from a
     // `NoSuchMethod` invocation.

sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/operations.dart

@@ -744,13 +744,15 @@ Object? hotReloadCorrectnessChecks(
       ),
     );
   }
-  var functionType = JS<Object?>(
-    '',
-    '#[#][#]',
-    receiver,
-    name,
-    JS_GET_NAME(JsGetName.SIGNATURE_NAME),
-  );
+  var functionType = _jsInstanceOf(receiver, Object)
+      ? getMethodType(receiver, name)
+      : JS<Object?>(
+          '',
+          '#[#][#]',
+          receiver,
+          name,
+          JS_GET_NAME(JsGetName.SIGNATURE_NAME),
+        );
   if (functionType == null) {
     // Allow JavaScript interop calls without checking arguments.
     // TODO(nshahan): Potentially we should be checking arguments to static
@@ -1546,7 +1548,7 @@ bool isStateBearingSymbol(property) => JS<bool>(
 /// copies the members of [classDeclaration] and its prototype's properties to
 /// the existing class. Existing members not prefixed by a special identifier
 /// are replaced (see [isStateBearingSymbol]).
-declareClass(library, classIdentifier, classDeclaration) {
+declareClass(Object library, Object classIdentifier, Object classDeclaration) {
   var originalClass = JS<Object>('!', '#.#', library, classIdentifier);
   if (JS<bool>('!', '# === void 0', originalClass)) {
     JS('', '#.# = #', library, classIdentifier, classDeclaration);
@@ -1559,13 +1561,16 @@ declareClass(library, classIdentifier, classDeclaration) {
       !isStateBearingSymbol(property),
       originalClassProto,
     );
+    // Reconcile instance members.
+    deleteClassMembers(originalClassProto, newClassProto);
     copyProperties(originalClassProto, newClassProto, copyWhen: copyWhenProto);
     var copyWhen = (property) => JS<bool>(
       '!',
       '# || # === void 0',
       !isStateBearingSymbol(property),
       originalClass,
     );
+    // Reconcile static members.
     deleteClassMembers(originalClass, classDeclaration);
     copyProperties(originalClass, classDeclaration, copyWhen: copyWhen);
   }
@@ -1579,10 +1584,12 @@ declareClass(library, classIdentifier, classDeclaration) {
 ///
 /// Called from generated code.
 void deleteClassMembers(Object oldClass, Object newClass) {
-  for (var name in getOwnNamesAndSymbols(oldClass)) {
-    if (JS<Object?>('', '#.#', newClass, name) == null &&
-        !isStateBearingSymbol(name)) {
-      JS('', 'delete #.#', oldClass, name);
+  var oldClassNamesAndSymbols = getOwnNamesAndSymbols(oldClass);
+  var newClassNamesAndSymbols = getOwnNamesAndSymbols(newClass);
+  for (var property in oldClassNamesAndSymbols) {
+    if (JS<bool>('', '!#.includes(#)', newClassNamesAndSymbols, property) &&
+        !isStateBearingSymbol(property)) {
+      JS('', 'delete #.#', oldClass, property);
     }
   }
 }

tests/hot_reload/call_deleted_instance_method_ddc/config.json

@@ -0,0 +1,5 @@
+{
+  "exclude": [
+    "vm"
+  ]
+}

tests/hot_reload/call_deleted_instance_method_ddc/main.0.dart

@@ -0,0 +1,30 @@
+// Copyright (c) 2025, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'package:expect/expect.dart';
+import 'package:reload_test/reload_test_utils.dart';
+
+var retained;
+C? c;
+
+class C {
+  String deleted() {
+    return 'hello';
+  }
+}
+
+helper() {
+  c = C();
+  retained = () => c!.deleted();
+  return retained();
+}
+
+Future<void> main() async {
+  helper();
+  await hotReload();
+  Expect.throws<NoSuchMethodError>(
+    helper,
+    (error) => '$error'.contains('deleted'),
+  );
+}

tests/hot_reload/call_deleted_instance_method_ddc/main.1.dart

@@ -0,0 +1,44 @@
+// Copyright (c) 2025, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'package:expect/expect.dart';
+import 'package:reload_test/reload_test_utils.dart';
+
+var retained;
+C? c;
+
+class C {}
+
+helper() {
+  return retained();
+}
+
+Future<void> main() async {
+  helper();
+  await hotReload();
+  Expect.throws<NoSuchMethodError>(
+    helper,
+    (error) => '$error'.contains('deleted'),
+  );
+}
+
+/** DIFF **/
+/*
+ var retained;
+ C? c;
+ 
+-class C {
+-  String deleted() {
+-    return 'hello';
+-  }
+-}
++class C {}
+ 
+ helper() {
+-  c = C();
+-  retained = () => c!.deleted();
+   return retained();
+ }
+ 
+*/

tests/hot_reload/call_deleted_instance_method_vm/config.json

@@ -0,0 +1,6 @@
+{
+  "exclude": [
+    "chrome",
+    "d8"
+  ]
+}

tests/hot_reload/call_deleted_instance_method_vm/main.0.dart

@@ -0,0 +1,30 @@
+// Copyright (c) 2025, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'package:expect/expect.dart';
+import 'package:reload_test/reload_test_utils.dart';
+
+var retained;
+C? c;
+
+class C {
+  String deleted() {
+    return 'hello';
+  }
+}
+
+helper() {
+  c = C();
+  retained = () => c!.deleted();
+  return retained();
+}
+
+Future<void> main() async {
+  helper();
+  await hotReload();
+  Expect.throws(
+    helper,
+    (error) => '$error'.contains('Lookup failed: deleted in @methods in C'),
+  );
+}

tests/hot_reload/call_deleted_instance_method_vm/main.1.dart

@@ -0,0 +1,44 @@
+// Copyright (c) 2025, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'package:expect/expect.dart';
+import 'package:reload_test/reload_test_utils.dart';
+
+var retained;
+C? c;
+
+class C {}
+
+helper() {
+  return retained();
+}
+
+Future<void> main() async {
+  helper();
+  await hotReload();
+  Expect.throws(
+    helper,
+    (error) => '$error'.contains('Lookup failed: deleted in @methods in C'),
+  );
+}
+
+/** DIFF **/
+/*
+ var retained;
+ C? c;
+ 
+-class C {
+-  String deleted() {
+-    return 'hello';
+-  }
+-}
++class C {}
+ 
+ helper() {
+-  c = C();
+-  retained = () => c!.deleted();
+   return retained();
+ }
+ 
+*/

tests/hot_reload/call_instance_method_parameters_change_ddc/config.json

@@ -0,0 +1,5 @@
+{
+  "exclude": [
+    "vm"
+  ]
+}

tests/hot_reload/call_instance_method_parameters_change_ddc/main.0.dart

@@ -0,0 +1,30 @@
+// Copyright (c) 2025, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'package:expect/expect.dart';
+import 'package:reload_test/reload_test_utils.dart';
+
+var retained;
+C? c;
+
+class C {
+  int parametersChange(int i) {
+    return i + 10;
+  }
+}
+
+helper() {
+  c = C();
+  retained = () => c!.parametersChange(32);
+  return retained!();
+}
+
+Future<void> main() async {
+  Expect.equals(42, helper());
+  await hotReload();
+  Expect.throws<TypeError>(
+    helper,
+    (error) => '$error'.contains("'int' is not a subtype of type 'String'"),
+  );
+}