Remove evaluation code that was replaced by expression evaluator (#1673)

* Remove evaluation code that was replaced by expression evaluator

* Update changelog

* Addressed CR comments

* Temporarily disable webdev evaluation tests

Author: Anna Gringauze

dwds/CHANGELOG.md

@@ -1,6 +1,11 @@
 ## 16.0.0-dev
 - Fix a hang and report errors on hot reload exceptions from the injected
   client.
+- Remove `AppInspector.evaluate` code that has been replaced by expression
+  evaluation using a compiler in all scenarios.
+- Fix a bug where evaluation would fail with more than one parameter in
+  the scope.
+- Remove showing uncaptured values from the stack during evaluation.
 
 **Breaking changes**
 - Remove no longer used `ExpressionCompilerService.handler`.

dwds/lib/src/debugging/inspector.dart

@@ -251,22 +251,6 @@ class AppInspector extends Domain {
     return RemoteObject(result.result['result'] as Map<String, Object>);
   }
 
-  Future<RemoteObject> evaluate(
-      String isolateId, String targetId, String expression,
-      {Map<String, String> scope}) async {
-    scope ??= {};
-    final library = await getLibrary(isolateId, targetId);
-    if (library == null) {
-      throw UnsupportedError(
-          'Evaluate is only supported when `targetId` is a library.');
-    }
-    if (scope.isNotEmpty) {
-      return evaluateInLibrary(library, scope, expression);
-    } else {
-      return evaluateJsExpressionOnLibrary(expression, library.uri);
-    }
-  }
-
   /// Invoke the function named [selector] on the object identified by
   /// [targetId].
   ///
@@ -298,21 +282,6 @@ class AppInspector extends Domain {
         arguments);
   }
 
-  /// Evaluate [expression] as a member/message of the library identified by
-  /// [libraryUri].
-  ///
-  /// That is, we will just do 'library.$expression'
-  Future<RemoteObject> evaluateJsExpressionOnLibrary(
-      String expression, String libraryUri) {
-    final evalExpression = '''
-(function() {
-  ${globalLoadStrategy.loadLibrarySnippet(libraryUri)};
-  return library.$expression;
-})();
-''';
-    return jsEvaluate(evalExpression);
-  }
-
   /// Evaluate [expression] by calling Chrome's Runtime.evaluate.
   Future<RemoteObject> jsEvaluate(String expression,
       {bool awaitPromise = false}) async {
@@ -343,21 +312,6 @@ class AppInspector extends Domain {
     return jsCallFunctionOn(remoteLibrary, jsFunction, arguments);
   }
 
-  /// Evaluate [expression] from [library] with [scope] as
-  /// arguments.
-  Future<RemoteObject> evaluateInLibrary(
-      Library library, Map<String, String> scope, String expression) async {
-    final argsString = scope.keys.join(', ');
-    final arguments = scope.values.map(remoteObjectFor).toList();
-    final evalExpression = '''
-function($argsString) {
-  ${globalLoadStrategy.loadLibrarySnippet(library.uri)};
-  return library.$expression;
-}
-    ''';
-    return _evaluateInLibrary(library, evalExpression, arguments);
-  }
-
   /// Call [function] with objects referred by [argumentIds] as arguments.
   Future<RemoteObject> callFunction(
       String function, Iterable<String> argumentIds) async {

dwds/lib/src/debugging/location.dart

@@ -166,9 +166,7 @@ class Locations {
         _entrypoint, Uri.parse(url).path);
     final cache = _moduleToLocations[module];
     if (cache != null) return cache;
-    if (module == null) {
-      _logger.warning('No module for $url');
-    } else {
+    if (module != null) {
       await _locationsForModule(module);
     }
     return _moduleToLocations[module] ?? {};

dwds/lib/src/services/chrome_proxy_service.dart

@@ -462,10 +462,8 @@ ${globalLoadStrategy.loadModuleSnippet}("dart_sdk").developer.invokeExtension(
                 isolateId, library.uri, expression, scope),
             expression);
       }
-      // fall back to javascript evaluation
-      final remote = await _inspector?.evaluate(isolateId, targetId, expression,
-          scope: scope);
-      return await _inspector?.instanceHelper?.instanceRefFor(remote);
+      throw RPCError('evaluateInFrame', RPCError.kInvalidRequest,
+          'Expression evaluation is not supported for this configuration.');
     }, (result) => DwdsEvent.evaluate(expression, result));
   }
 

dwds/lib/src/services/expression_evaluator.dart

@@ -78,6 +78,7 @@ class ExpressionEvaluator {
     String expression,
     Map<String, String> scope,
   ) async {
+    scope ??= {};
     if (_compiler == null) {
       return _createError(ErrorKind.internal,
           'ExpressionEvaluator needs an ExpressionCompiler');
@@ -89,10 +90,8 @@ class ExpressionEvaluator {
 
     final module = await _modules.moduleForlibrary(libraryUri);
 
-    if (scope != null && scope.isNotEmpty) {
-      final params = scope.keys.join(', ');
-      expression = '($params) => $expression';
-    }
+    // Wrap the expression in a lambda so we can call it as a function.
+    expression = _createDartLambda(expression, scope.keys);
     _logger.finest('Evaluating "$expression" at $module');
 
     // Compile expression using an expression compiler, such as
@@ -106,23 +105,13 @@ class ExpressionEvaluator {
       return _formatCompilationError(jsResult);
     }
 
+    // Strip try/catch incorrectly added by the expression compiler.
+    var jsCode = _maybeStripTryCatch(jsResult);
+
     // Send JS expression to chrome to evaluate.
-    RemoteObject result;
-    if (scope != null && scope.isNotEmpty) {
-      // Strip try/catch.
-      // TODO: remove adding try/catch block in expression compiler.
-      // https://github.com/dart-lang/webdev/issues/1341
-      final lines = jsResult.split('\n');
-      final inner = lines.getRange(2, lines.length - 3).join('\n');
-      final function = 'function(t) {'
-          '  return $inner(t);'
-          '}';
-      result = await _inspector.callFunction(function, scope.values);
-      result = await _formatEvaluationError(result);
-    } else {
-      result = await _inspector.debugger.evaluate(jsResult);
-      result = await _formatEvaluationError(result);
-    }
+    jsCode = _createJsLambdaWithTryCatch(jsCode, scope.keys);
+    var result = await _inspector.callFunction(jsCode, scope.values);
+    result = await _formatEvaluationError(result);
 
     _logger.finest('Evaluated "$expression" to "$result"');
     return result;
@@ -207,30 +196,21 @@ class ExpressionEvaluator {
       return _formatCompilationError(jsResult);
     }
 
+    // Strip try/catch incorrectly added by the expression compiler.
+    var jsCode = _maybeStripTryCatch(jsResult);
+
+    // Send JS expression to chrome to evaluate.
+    jsCode = _createTryCatch(jsCode);
+
     // Send JS expression to chrome to evaluate.
     var result = await _inspector.debugger
-        .evaluateJsOnCallFrameIndex(frameIndex, jsResult);
+        .evaluateJsOnCallFrameIndex(frameIndex, jsCode);
     result = await _formatEvaluationError(result);
 
     _logger.finest('Evaluated "$expression" to "$result"');
     return result;
   }
 
-  String _valueToLiteral(RemoteObject value) {
-    if (value.value == null) {
-      return null;
-    }
-
-    final ret = value.value.toString();
-    if (value.type == 'string') {
-      return '\'$ret\'';
-    }
-
-    return ret;
-  }
-
-  bool _isUndefined(RemoteObject value) => value.type == 'undefined';
-
   RemoteObject _formatCompilationError(String error) {
     // Frontend currently gives a text message including library name
     // and function name on compilation error. Strip this information
@@ -286,28 +266,7 @@ class ExpressionEvaluator {
       for (var p in variables) {
         final name = p.name;
         final value = p.value;
-        if (_isUndefined(value)) continue;
-
-        if (scopeType == 'closure') {
-          // Substitute potentially unavailable captures with their values from
-          // the stack.
-          //
-          // Note: this makes some uncaptured values available for evaluation,
-          // which might not be formally correct but convenient, for evample:
-          //
-          // int x = 0;
-          // var f = (int y) {
-          //   // 'x' is not captured so it not available at runtime but is
-          //   // captured on stack, so the code below will make it available
-          //   // for evaluation
-          //   print(y);
-          // }
-          // TODO(annagrin): decide if we would like not to support evaluation
-          // of uncaptured variables
-
-          final capturedValue = _valueToLiteral(value);
-          jsScope[name] = capturedValue ?? name;
-        } else {
+        if (!_isUndefined(value)) {
           jsScope[name] = name;
         }
       }
@@ -325,6 +284,68 @@ class ExpressionEvaluator {
     return jsScope;
   }
 
+  bool _isUndefined(RemoteObject value) =>
+      value == null || value.type == 'undefined';
+
+  /// Strip try/catch incorrectly added by the expression compiler.
+  /// TODO: remove adding try/catch block in expression compiler.
+  /// https://github.com/dart-lang/webdev/issues/1341, then remove
+  /// this stripping code.
+  String _maybeStripTryCatch(String jsCode) {
+    // Match the wrapping generated by the expression compiler exactly
+    // so the maching does not succeed naturally after the wrapping is
+    // removed:
+    //
+    // Expression compiler's wrapping:
+    //
+    // '\ntry {'
+    // '\n  ($jsExpression('
+    // '\n    $args'
+    // '\n  ))'
+    // '\n} catch (error) {'
+    // '\n  error.name + ": " + error.message;'
+    // '\n}';
+    //
+    final lines = jsCode.split('\n');
+    if (lines.length > 5) {
+      final tryLines = lines.getRange(0, 2).toList();
+      final bodyLines = lines.getRange(2, lines.length - 3);
+      final catchLines =
+          lines.getRange(lines.length - 3, lines.length).toList();
+      if (tryLines[0].isEmpty &&
+          tryLines[1] == 'try {' &&
+          catchLines[0] == '} catch (error) {' &&
+          catchLines[1] == '  error.name + ": " + error.message;' &&
+          catchLines[2] == '}') {
+        return bodyLines.join('\n');
+      }
+    }
+    return jsCode;
+  }
+
+  String _createJsLambdaWithTryCatch(
+      String expression, Iterable<String> params) {
+    final args = params.join(', ');
+    return '  '
+        '  function($args) {\n'
+        '    try {\n'
+        '      return $expression($args);\n'
+        '    } catch (error) {\n'
+        '      return error.name + ": " + error.message;\n'
+        '    }\n'
+        '} ';
+  }
+
+  String _createTryCatch(String expression) => '  '
+      '  try {\n'
+      '    $expression;\n'
+      '  } catch (error) {\n'
+      '    error.name + ": " + error.message;\n'
+      '  }\n';
+
+  String _createDartLambda(String expression, Iterable<String> params) =>
+      '(${params.join(', ')}) => $expression';
+
   /// Returns Chrome script uri for Chrome script ID.
   String _urlForScriptId(String scriptId) =>
       _inspector.remoteDebugger.scripts[scriptId]?.url;

dwds/test/chrome_proxy_service_test.dart

@@ -36,7 +36,10 @@ void main() {
   group('shared context', () {
     setUpAll(() async {
       setCurrentLogWriter(debug: debug);
-      await context.setUp(verboseCompiler: false);
+      await context.setUp(
+        enableExpressionEvaluation: true,
+        verboseCompiler: false,
+      );
     });
 
     tearDownAll(() async {

dwds/test/fixtures/fakes.dart

@@ -52,12 +52,6 @@ class FakeInspector extends Domain implements AppInspector {
     throw UnsupportedError('This is a fake');
   }
 
-  @override
-  Future<RemoteObject> evaluate(
-          String isolateId, String targetId, String expression,
-          {Map<String, String> scope}) =>
-      null;
-
   @override
   Future<Obj> getObject(String isolateId, String objectId,
           {int offset, int count}) =>

dwds/test/inspector_test.dart

@@ -45,6 +45,9 @@ void main() {
   Future<RemoteObject> libraryPublicFinal() =>
       inspector.jsEvaluate(libraryVariableExpression('libraryPublicFinal'));
 
+  Future<RemoteObject> libraryPrivate() =>
+      inspector.jsEvaluate(libraryVariableExpression('_libraryPrivate'));
+
   test('send toString', () async {
     final remoteObject = await libraryPublicFinal();
     final toString = await inspector.invokeMethod(remoteObject, 'toString', []);
@@ -107,8 +110,7 @@ void main() {
     setUp(() async {
       isolateId = inspector.isolate.id;
       bootstrapLibrary = inspector.isolate.rootLib;
-      instance = await inspector.evaluate(
-          isolateId, bootstrapLibrary.id, 'libraryPublicFinal');
+      instance = await libraryPublicFinal();
     });
 
     test('invoke top-level private', () async {
@@ -139,8 +141,7 @@ void main() {
     });
 
     test('invoke instance method with object parameter 2', () async {
-      final libraryPrivateList = await inspector.evaluate(
-          isolateId, bootstrapLibrary.id, '_libraryPrivate');
+      final libraryPrivateList = await libraryPrivate();
       final remote = await inspector.invoke(isolateId, instance.objectId,
           'equals', [libraryPrivateList.objectId]);
       expect(

webdev/test/e2e_test.dart

@@ -482,16 +482,10 @@ void main() {
 
               await vmService.streamListen('Debug');
 
-              // Evaluate falls back to js evaluation
-              var result = await vmService.evaluate(
-                  isolate.id, library.id, 'main.toString()');
-
               expect(
-                  result,
-                  const TypeMatcher<InstanceRef>().having(
-                      (instance) => instance.valueAsString,
-                      'valueAsString',
-                      contains('Hello World!!')));
+                  () => vmService.evaluate(
+                      isolate.id, library.id, 'main.toString()'),
+                  throwsRPCError);
             } finally {
               await vmService?.dispose();
               await exitWebdev(process);
@@ -501,5 +495,5 @@ void main() {
         });
       });
     }
-  });
+  }, skip: 'dart-lang/sdk#49373');
 }