[dartdevc] fix #36532, adds DDC support for CFE constant flag

This fixes DDC to work when CFE's constants flag is enabled.
The fixes are primarily due to how CFE inlines constants, and how it
handles "unevaluated constants" (these arise from modular compilation
and outline kernel files).

Specifically this CL addresses the following issues:

- private names now work across modules (landed in this CL:
  https://dart-review.googlesource.com/c/sdk/+/98926)
- correctly emit tearoffs regardless of how they're represeted
  (constant or StaticGet)
- correctly recognize the internal SDK and JS interop annotations,
  regardless of how they're represented (constants or expressions)
- change how DDC represents `undefined` in its internal SDK, so
  its detected regardless of constant inlining
- emit numeric constants as integers in JS when possible


Change-Id: I0fbda9d24578075f655ea945ed1b2ceed124af48
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/99240
Reviewed-by: Mark Zhou <markzipan@google.com>
Commit-Queue: Jenny Messerly <jmesserly@google.com>

Author: Jenny Messerly

pkg/dev_compiler/lib/src/analyzer/code_generator.dart

@@ -2449,12 +2449,12 @@ class CodeGenerator extends Object
 
       if (param.isOptional) {
         JS.Expression defaultValue;
-        if (paramNode != null) {
+        if (findAnnotation(param, isUndefinedAnnotation) != null) {
+          defaultValue = null;
+        } else if (paramNode != null) {
           var paramDefault = (paramNode as DefaultFormalParameter).defaultValue;
           if (paramDefault == null) {
             defaultValue = JS.LiteralNull();
-          } else if (_isJSUndefined(paramDefault)) {
-            defaultValue = null;
           } else {
             defaultValue = _visitExpression(paramDefault);
           }
@@ -2513,16 +2513,6 @@ class CodeGenerator extends Object
         (_classProperties?.covariantParameters?.contains(p) ?? false);
   }
 
-  bool _isJSUndefined(Expression expr) {
-    expr = expr is AsExpression ? expr.expression : expr;
-    if (expr is Identifier) {
-      var element = expr.staticElement;
-      return isSdkInternalRuntime(element.library) &&
-          element.name == 'undefined';
-    }
-    return false;
-  }
-
   JS.Fun _emitNativeFunctionBody(MethodDeclaration node) {
     String name = getAnnotationName(node.declaredElement, isJSAnnotation) ??
         node.name.name;
@@ -4270,10 +4260,6 @@ class CodeGenerator extends Object
       List<VariableDeclaration> fields) {
     var lazyFields = <VariableDeclaration>[];
     for (var field in fields) {
-      // Skip our magic undefined constant.
-      var element = field.declaredElement as TopLevelVariableElement;
-      if (element.name == 'undefined') continue;
-
       var init = field.initializer;
       if (init == null ||
           init is Literal ||

pkg/dev_compiler/lib/src/analyzer/js_interop.dart

@@ -65,6 +65,9 @@ bool isNotNullAnnotation(DartObjectImpl value) =>
 bool isNullCheckAnnotation(DartObjectImpl value) =>
     isBuiltinAnnotation(value, '_js_helper', '_NullCheck');
 
+bool isUndefinedAnnotation(DartObjectImpl value) =>
+    isBuiltinAnnotation(value, '_js_helper', '_Undefined');
+
 /// Returns the name value of the `JSExportName` annotation (when compiling
 /// the SDK), or `null` if there's none. This is used to control the name
 /// under which functions are compiled and exported.

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

@@ -8,7 +8,7 @@ import 'dart:math' show max, min;
 import 'package:front_end/src/api_unstable/ddc.dart' show TypeSchemaEnvironment;
 import 'package:kernel/class_hierarchy.dart';
 import 'package:kernel/core_types.dart';
-import 'package:kernel/kernel.dart';
+import 'package:kernel/kernel.dart' hide MapEntry;
 import 'package:kernel/library_index.dart';
 import 'package:kernel/type_algebra.dart';
 import 'package:kernel/type_environment.dart';
@@ -1808,7 +1808,7 @@ class ProgramCompiler extends Object
 
   /// Emits a Dart factory constructor to a JS static method.
   JS.Method _emitFactoryConstructor(Procedure node) {
-    if (isUnsupportedFactoryConstructor(node)) return null;
+    if (node.isExternal || isUnsupportedFactoryConstructor(node)) return null;
 
     var function = node.function;
 
@@ -1983,9 +1983,6 @@ class ProgramCompiler extends Object
       var lazyFields = <Field>[];
       var savedUri = _currentUri;
       for (var field in fields) {
-        // Skip our magic undefined constant.
-        if (field.name.name == 'undefined') continue;
-
         var init = field.initializer;
         if (init == null ||
             init is BasicLiteral ||
@@ -2954,23 +2951,15 @@ class ProgramCompiler extends Object
   }
 
   JS.Expression _defaultParamValue(VariableDeclaration p) {
-    if (p.initializer != null) {
-      var value = p.initializer;
-      return _isJSUndefined(value) ? null : _visitExpression(value);
+    if (p.annotations.any(isUndefinedAnnotation)) {
+      return null;
+    } else if (p.initializer != null) {
+      return _visitExpression(p.initializer);
     } else {
       return JS.LiteralNull();
     }
   }
 
-  bool _isJSUndefined(Expression expr) {
-    expr = expr is AsExpression ? expr.operand : expr;
-    if (expr is StaticGet) {
-      var t = expr.target;
-      return isSdkInternalRuntime(getLibrary(t)) && t.name.name == 'undefined';
-    }
-    return false;
-  }
-
   void _emitCovarianceBoundsCheck(
       List<TypeParameter> typeFormals, List<JS.Statement> body) {
     for (var t in typeFormals) {
@@ -3786,15 +3775,11 @@ class ProgramCompiler extends Object
   }
 
   @override
-  visitStaticGet(StaticGet node) {
-    var target = node.target;
+  visitStaticGet(StaticGet node) => _emitStaticGet(node.target);
 
+  JS.Expression _emitStaticGet(Member target) {
     // TODO(vsm): Re-inline constants.  See:
     // https://github.com/dart-lang/sdk/issues/36285
-
-    // TODO(markzipan): reifyTearOff check can be removed when we enable
-    // front-end constant evaluation because static tear-offs will be
-    // treated as constants and handled by visitTearOffConstant.
     var result = _emitStaticTarget(target);
     if (_reifyTearoff(target)) {
       // TODO(jmesserly): we could tag static/top-level function types once
@@ -5092,13 +5077,8 @@ class ProgramCompiler extends Object
         isBuiltinAnnotation(a, '_js_helper', 'ReifyFunctionTypes');
     while (parent != null) {
       var a = findAnnotation(parent, reifyFunctionTypes);
-      if (a is ConstructorInvocation) {
-        var args = a.arguments.positional;
-        if (args.length == 1) {
-          var arg = args[0];
-          if (arg is BoolLiteral) return arg.value;
-        }
-      }
+      var value = _constants.getFieldValueFromAnnotation(a, 'value');
+      if (value is bool) return value;
       parent = parent.parent;
     }
     return true;
@@ -5128,7 +5108,8 @@ class ProgramCompiler extends Object
   ///
   /// Calls [findAnnotation] followed by [getNameFromAnnotation].
   String getAnnotationName(NamedNode node, bool test(Expression value)) {
-    return _constants.getNameFromAnnotation(findAnnotation(node, test));
+    return _constants.getFieldValueFromAnnotation(
+        findAnnotation(node, test), 'name');
   }
 
   JS.Expression visitConstant(Constant node) => node.accept(this);
@@ -5139,7 +5120,23 @@ class ProgramCompiler extends Object
   @override
   visitIntConstant(IntConstant node) => js.number(node.value);
   @override
-  visitDoubleConstant(DoubleConstant node) => js.number(node.value);
+  visitDoubleConstant(DoubleConstant node) {
+    var value = node.value;
+
+    // Emit the constant as an integer, if possible.
+    if (value.isFinite) {
+      var intValue = value.toInt();
+      const int _MIN_INT32 = -0x80000000;
+      const int _MAX_INT32 = 0x7FFFFFFF;
+      if (intValue.toDouble() == value &&
+          intValue >= _MIN_INT32 &&
+          intValue <= _MAX_INT32) {
+        return js.number(intValue);
+      }
+    }
+    return js.number(value);
+  }
+
   @override
   visitStringConstant(StringConstant node) => js.escapedString(node.value, '"');
 
@@ -5174,13 +5171,11 @@ class ProgramCompiler extends Object
 
   @override
   visitInstanceConstant(node) {
-    entryToProperty(entry) {
-      var field = entry.key.asField.name.name;
+    entryToProperty(MapEntry<Reference, Constant> entry) {
       var constant = entry.value.accept(this);
       var member = entry.key.asField;
-      var result =
-          JS.Property(_emitMemberName(field, member: member), constant);
-      return result;
+      return JS.Property(
+          _emitMemberName(member.name.name, member: member), constant);
     }
 
     var type = visitInterfaceType(node.getType(types) as InterfaceType);
@@ -5192,14 +5187,7 @@ class ProgramCompiler extends Object
   }
 
   @override
-  visitTearOffConstant(node) {
-    var target = node.procedure;
-    var result = _emitStaticTarget(target);
-    // TODO(jmesserly): we could tag static/top-level function types once
-    // in the module initialization, rather than at the point where they
-    // escape.
-    return _emitFunctionTagged(result, target.function.functionType);
-  }
+  visitTearOffConstant(node) => _emitStaticGet(node.procedure);
 
   @override
   visitTypeLiteralConstant(node) => _emitTypeLiteral(node.type);

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

@@ -41,10 +41,10 @@ class DevCompilerConstants {
     return result is UnevaluatedConstant ? null : result;
   }
 
-  /// If [node] is an annotation with a field named `name`, returns that field's
+  /// If [node] is an annotation with a field named [name], returns that field's
   /// value.
   ///
-  /// This assumes the `name` field is populated from a named argument `name:`
+  /// This assumes the field is populated from a named argument with that name,
   /// or from the first positional argument.
   ///
   /// For example:
@@ -59,36 +59,56 @@ class DevCompilerConstants {
   ///     main() { ... }
   ///
   /// Given the node for `@MyAnnotation('FooBar')` this will return `'FooBar'`.
-  String getNameFromAnnotation(ConstructorInvocation node) {
-    if (node == null) return null;
+  Object getFieldValueFromAnnotation(Expression node, String name) {
+    node = unwrapUnevaluatedConstant(node);
+    if (node is ConstantExpression) {
+      var constant = node.constant;
+      if (constant is InstanceConstant) {
+        var value = constant.fieldValues.entries
+            .firstWhere((e) => e.key.asField.name.name == name,
+                orElse: () => null)
+            ?.value;
+        if (value is PrimitiveConstant) return value.value;
+        if (value is UnevaluatedConstant) {
+          return _evaluateAnnotationArgument(value.expression);
+        }
+        return null;
+      }
+    }
 
     // TODO(jmesserly): this does not use the normal evaluation engine, because
     // it won't work if we don't have the const constructor body available.
     //
     // We may need to address this in the kernel outline files.
-    Expression first;
-    var named = node.arguments.named;
-    if (named.isNotEmpty) {
-      first =
-          named.firstWhere((n) => n.name == 'name', orElse: () => null)?.value;
-    }
-    var positional = node.arguments.positional;
-    if (positional.isNotEmpty) first ??= positional[0];
-    if (first != null) {
-      first = _followConstFields(first);
-      if (first is StringLiteral) return first.value;
+    if (node is ConstructorInvocation) {
+      Expression first;
+      var named = node.arguments.named;
+      if (named.isNotEmpty) {
+        first =
+            named.firstWhere((n) => n.name == name, orElse: () => null)?.value;
+      }
+      var positional = node.arguments.positional;
+      if (positional.isNotEmpty) first ??= positional[0];
+      if (first != null) {
+        return _evaluateAnnotationArgument(first);
+      }
     }
     return null;
   }
 
-  Expression _followConstFields(Expression expr) {
-    if (expr is StaticGet) {
-      var target = expr.target;
+  Object _evaluateAnnotationArgument(Expression node) {
+    node = unwrapUnevaluatedConstant(node);
+    if (node is ConstantExpression) {
+      var constant = node.constant;
+      if (constant is PrimitiveConstant) return constant.value;
+    }
+    if (node is StaticGet) {
+      var target = node.target;
       if (target is Field) {
-        return _followConstFields(target.initializer);
+        return _evaluateAnnotationArgument(target.initializer);
       }
     }
-    return expr;
+    return node is BasicLiteral ? node.value : null;
   }
 }
 

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

@@ -17,13 +17,7 @@ bool _isJSLibrary(Library library) {
 }
 
 bool _annotationIsFromJSLibrary(String expectedName, Expression value) {
-  Class c;
-  if (value is ConstructorInvocation) {
-    c = value.target.enclosingClass;
-  } else if (value is StaticGet) {
-    var type = value.target.getterType;
-    if (type is InterfaceType) c = type.classNode;
-  }
+  var c = getAnnotationClass(value);
   return c != null &&
       c.name == expectedName &&
       _isJSLibrary(c.enclosingLibrary);
@@ -48,42 +42,35 @@ bool isPublicJSAnnotation(Expression value) =>
 bool _isJSAnonymousAnnotation(Expression value) =>
     _annotationIsFromJSLibrary('_Anonymous', value);
 
-bool _isBuiltinAnnotation(
-    Expression value, String libraryName, String annotationName) {
-  if (value is ConstructorInvocation) {
-    var c = value.target.enclosingClass;
-    if (c.name == annotationName) {
-      var uri = c.enclosingLibrary.importUri;
-      return uri.scheme == 'dart' && uri.pathSegments[0] == libraryName;
-    }
-  }
-  return false;
-}
-
 /// Whether [value] is a `@JSExportName` (internal annotation used in SDK
 /// instead of `@JS` from `package:js`).
 bool isJSExportNameAnnotation(Expression value) =>
-    _isBuiltinAnnotation(value, '_foreign_helper', 'JSExportName');
+    isBuiltinAnnotation(value, '_foreign_helper', 'JSExportName');
 
 /// Whether [i] is a `spread` invocation (to be used on function arguments
 /// to have them compiled as `...` spread args in ES6 outputs).
 bool isJSSpreadInvocation(Procedure target) =>
     target.name.name == 'spread' && _isJSLibrary(target.enclosingLibrary);
 
 bool isJSName(Expression value) =>
-    _isBuiltinAnnotation(value, '_js_helper', 'JSName');
+    isBuiltinAnnotation(value, '_js_helper', 'JSName');
 
 bool isJsPeerInterface(Expression value) =>
-    _isBuiltinAnnotation(value, '_js_helper', 'JsPeerInterface');
+    isBuiltinAnnotation(value, '_js_helper', 'JsPeerInterface');
 
 bool isNativeAnnotation(Expression value) =>
-    _isBuiltinAnnotation(value, '_js_helper', 'Native');
+    isBuiltinAnnotation(value, '_js_helper', 'Native');
 
 bool isJSAnonymousType(Class namedClass) {
-  return hasJSInteropAnnotation(namedClass) &&
+  var hasJSInterop = hasJSInteropAnnotation(namedClass);
+  var isAnonymous =
       findAnnotation(namedClass, _isJSAnonymousAnnotation) != null;
+  return hasJSInterop && isAnonymous;
 }
 
+bool isUndefinedAnnotation(Expression value) =>
+    isBuiltinAnnotation(value, '_js_helper', '_Undefined');
+
 /// Returns true iff the class has an `@JS(...)` annotation from `package:js`.
 ///
 /// Note: usually [_usesJSInterop] should be used instead of this.