From a052d8e0dca2cb55ccd61af6327b9e4d8c718d62 Mon Sep 17 00:00:00 2001 From: Jenny Messerly Date: Wed, 17 Jan 2018 02:53:22 +0000 Subject: [PATCH] fix some issues with compiling the kernel SDK - fix all usage of `.name` when `.name.name` is required - fix handling of inline-JS in the SDK - fix casts on SDK nodes Change-Id: Ic48e0b77e6e03515f16b8c30d3e274abbe2ed272 Reviewed-on: https://dart-review.googlesource.com/34540 Reviewed-by: Vijay Menon Commit-Queue: Jenny Messerly --- .../lib/src/analyzer/code_generator.dart | 152 +++++++++--------- pkg/dev_compiler/lib/src/kernel/compiler.dart | 89 +++++----- .../lib/src/kernel/kernel_helpers.dart | 2 +- .../lib/src/kernel/nullable_inference.dart | 7 - .../input_sdk/private/ddc_runtime/types.dart | 2 +- .../input_sdk/private/foreign_helper.dart | 2 +- .../tool/input_sdk/private/js_number.dart | 2 +- 7 files changed, 124 insertions(+), 132 deletions(-) diff --git a/pkg/dev_compiler/lib/src/analyzer/code_generator.dart b/pkg/dev_compiler/lib/src/analyzer/code_generator.dart index bb2c7658e8d..d08f1e3fddd 100644 --- a/pkg/dev_compiler/lib/src/analyzer/code_generator.dart +++ b/pkg/dev_compiler/lib/src/analyzer/code_generator.dart @@ -3898,93 +3898,91 @@ class CodeGenerator extends Object /// Emits code for the `JS(...)` macro. JS.Node _emitForeignJS(MethodInvocation node, Element e) { - if (isInlineJS(e)) { - var args = node.argumentList.arguments; - // arg[0] is static return type, used in `RestrictedStaticTypeAnalyzer` - var code = args[1]; - List templateArgs; - String source; - if (code is StringInterpolation) { - if (args.length > 2) { - throw new ArgumentError( - "Can't mix template args and string interpolation in JS calls."); - } - templateArgs = []; - source = code.elements.map((element) { - if (element is InterpolationExpression) { - templateArgs.add(element.expression); - return '#'; - } else { - return (element as InterpolationString).value; - } - }).join(); - } else { - templateArgs = args.skip(2).toList(); - source = (code as StringLiteral).stringValue; - } + if (!isInlineJS(e)) return null; - // TODO(vsm): Constructors in dart:html and friends are trying to - // allocate a type defined on window/self, but this often conflicts a - // with the generated extension class in scope. We really should - // qualify explicitly in dart:html itself. - var constructorPattern = new RegExp("new [A-Z][A-Za-z]+\\("); - if (constructorPattern.matchAsPrefix(source) != null) { - var containingClass = node.parent; - while ( - containingClass != null && containingClass is! ClassDeclaration) { - containingClass = containingClass.parent; + var args = node.argumentList.arguments; + // arg[0] is static return type, used in `RestrictedStaticTypeAnalyzer` + var code = args[1]; + List templateArgs; + String source; + if (code is StringInterpolation) { + if (args.length > 2) { + throw new ArgumentError( + "Can't mix template args and string interpolation in JS calls."); + } + templateArgs = []; + source = code.elements.map((element) { + if (element is InterpolationExpression) { + templateArgs.add(element.expression); + return '#'; + } else { + return (element as InterpolationString).value; } - if (containingClass is ClassDeclaration && - _extensionTypes.isNativeClass(containingClass.element)) { - var constructorName = source.substring(4, source.indexOf('(')); - var className = containingClass.name.name; - if (className == constructorName) { - source = - source.replaceFirst('new $className(', 'new self.$className('); - } + }).join(); + } else { + templateArgs = args.skip(2).toList(); + source = (code as StringLiteral).stringValue; + } + + // TODO(vsm): Constructors in dart:html and friends are trying to + // allocate a type defined on window/self, but this often conflicts a + // with the generated extension class in scope. We really should + // qualify explicitly in dart:html itself. + var constructorPattern = new RegExp("new [A-Z][A-Za-z]+\\("); + if (constructorPattern.matchAsPrefix(source) != null) { + var containingClass = node.parent; + while (containingClass != null && containingClass is! ClassDeclaration) { + containingClass = containingClass.parent; + } + if (containingClass is ClassDeclaration && + _extensionTypes.isNativeClass(containingClass.element)) { + var constructorName = source.substring(4, source.indexOf('(')); + var className = containingClass.name.name; + if (className == constructorName) { + source = + source.replaceFirst('new $className(', 'new self.$className('); } } + } - JS.Expression visitTemplateArg(Expression arg) { - if (arg is InvocationExpression) { - var e = arg is MethodInvocation - ? arg.methodName.staticElement - : (arg as FunctionExpressionInvocation).staticElement; - if (e?.name == 'getGenericClass' && - e.library.name == 'dart._runtime' && - arg.argumentList.arguments.length == 1) { - var typeArg = arg.argumentList.arguments[0]; - if (typeArg is SimpleIdentifier) { - var typeElem = typeArg.staticElement; - if (typeElem is TypeDefiningElement && - typeElem.type is ParameterizedType) { - return _emitTopLevelNameNoInterop(typeElem, suffix: '\$'); - } + JS.Expression visitTemplateArg(Expression arg) { + if (arg is InvocationExpression) { + var e = arg is MethodInvocation + ? arg.methodName.staticElement + : (arg as FunctionExpressionInvocation).staticElement; + if (e?.name == 'getGenericClass' && + e.library.name == 'dart._runtime' && + arg.argumentList.arguments.length == 1) { + var typeArg = arg.argumentList.arguments[0]; + if (typeArg is SimpleIdentifier) { + var typeElem = typeArg.staticElement; + if (typeElem is TypeDefiningElement && + typeElem.type is ParameterizedType) { + return _emitTopLevelNameNoInterop(typeElem, suffix: '\$'); } } } - return _visitExpression(arg); } - - // TODO(rnystrom): The JS() calls are almost never nested, and probably - // really shouldn't be, but there are at least a couple of calls in the - // HTML library where an argument to JS() is itself a JS() call. If those - // go away, this can just assert(!_isInForeignJS). - // Inside JS(), type names evaluate to the raw runtime type, not the - // wrapped Type object. - var wasInForeignJS = _isInForeignJS; - _isInForeignJS = true; - var jsArgs = templateArgs.map(visitTemplateArg).toList(); - _isInForeignJS = wasInForeignJS; - - var result = js.parseForeignJS(source).instantiate(jsArgs); - - // `throw` is emitted as a statement by `parseForeignJS`. - assert(result is JS.Expression || - result is JS.Throw && node.parent is ExpressionStatement); - return result; + return _visitExpression(arg); } - return null; + + // TODO(rnystrom): The JS() calls are almost never nested, and probably + // really shouldn't be, but there are at least a couple of calls in the + // HTML library where an argument to JS() is itself a JS() call. If those + // go away, this can just assert(!_isInForeignJS). + // Inside JS(), type names evaluate to the raw runtime type, not the + // wrapped Type object. + var wasInForeignJS = _isInForeignJS; + _isInForeignJS = true; + var jsArgs = templateArgs.map(visitTemplateArg).toList(); + _isInForeignJS = wasInForeignJS; + + var result = js.parseForeignJS(source).instantiate(jsArgs); + + // `throw` is emitted as a statement by `parseForeignJS`. + assert(result is JS.Expression || + result is JS.Throw && node.parent is ExpressionStatement); + return result; } @override diff --git a/pkg/dev_compiler/lib/src/kernel/compiler.dart b/pkg/dev_compiler/lib/src/kernel/compiler.dart index cdaa0be4c5c..8668672d6cc 100644 --- a/pkg/dev_compiler/lib/src/kernel/compiler.dart +++ b/pkg/dev_compiler/lib/src/kernel/compiler.dart @@ -658,7 +658,7 @@ class ProgramCompiler var jsParams = _emitFormalParameters(ctor.function); var ctorBody = []; if (mixinCtor != null) ctorBody.add(mixinCtor); - if (ctor.name != '' || hasUnnamedSuper) { + if (ctor.name.name != '' || hasUnnamedSuper) { ctorBody.add( _emitSuperConstructorCall(className, ctor.name.name, jsParams)); } @@ -2029,12 +2029,12 @@ class ProgramCompiler var lazyFields = []; for (var field in fields) { // Skip our magic undefined constant. - if (field.name == 'undefined') continue; + if (field.name.name == 'undefined') continue; var init = field.initializer; if (init == null || init is BasicLiteral || - _isJSInvocation(init) || + _isInlineJSCall(init) || init is ConstructorInvocation && isSdkInternalRuntime(init.target.enclosingLibrary)) { _moduleItems.add(js.statement('# = #;', [ @@ -2048,9 +2048,6 @@ class ProgramCompiler return _emitLazyFields(_currentLibrary, lazyFields); } - bool _isJSInvocation(Expression expr) => - expr is StaticInvocation && isInlineJS(expr.target); - JS.Statement _emitLazyFields(NamedNode target, Iterable fields) { var accessors = []; for (var field in fields) { @@ -3032,8 +3029,14 @@ class ProgramCompiler defaultStatement(Statement node) => _emitInvalidNode(node).toStatement(); @override - visitExpressionStatement(ExpressionStatement node) => - _visitAndMarkExpression(node.expression).toStatement(); + visitExpressionStatement(ExpressionStatement node) { + var expr = node.expression; + if (_isInlineJSCall(expr)) { + var inlineJS = _emitInlineJSCode(expr); + return inlineJS is JS.Expression ? inlineJS.toStatement() : inlineJS; + } + return _visitAndMarkExpression(expr).toStatement(); + } @override visitBlock(Block node) => @@ -4096,13 +4099,11 @@ class ProgramCompiler @override visitStaticInvocation(StaticInvocation node) { - var result = _emitForeignJS(node); - if (result != null) return result; - if (node.target.isFactory) { - return _emitFactoryInvocation(node); - } var target = node.target; - if (target?.name == 'extensionSymbol' && + if (isInlineJS(target)) return _emitInlineJSCode(node); + if (target.isFactory) return _emitFactoryInvocation(node); + + if (target.name.name == 'extensionSymbol' && isSdkInternalRuntime(target.enclosingLibrary)) { var args = node.arguments; var firstArg = args.positional.length == 1 ? args.positional[0] : null; @@ -4158,27 +4159,32 @@ class ProgramCompiler } /// Emits code for the `JS(...)` macro. - JS.Expression _emitForeignJS(StaticInvocation node) { - if (!isInlineJS(node.target)) return null; + JS.Node _emitInlineJSCode(StaticInvocation node) { var args = node.arguments.positional; // arg[0] is static return type, used in `RestrictedStaticTypeAnalyzer` var code = args[1]; List templateArgs; String source; if (code is StringConcatenation) { - if (args.length > 2) { - throw new ArgumentError( - "Can't mix template args and string interpolation in JS calls."); - } - templateArgs = []; - source = code.expressions.map((expression) { - if (expression is StringLiteral) { - return expression.value; - } else { - templateArgs.add(expression); - return '#'; + if (code.expressions.every((e) => e is StringLiteral)) { + templateArgs = args.skip(2).toList(); + source = code.expressions.map((e) => (e as StringLiteral).value).join(); + } else { + if (args.length > 2) { + throw new ArgumentError( + "Can't mix template args and string interpolation in JS calls: " + "`$node`"); } - }).join(); + templateArgs = []; + source = code.expressions.map((expression) { + if (expression is StringLiteral) { + return expression.value; + } else { + templateArgs.add(expression); + return '#'; + } + }).join(); + } } else { templateArgs = args.skip(2).toList(); source = (code as StringLiteral).value; @@ -4209,7 +4215,7 @@ class ProgramCompiler if (arg is StaticInvocation) { var target = arg.target; var positional = arg.arguments.positional; - if (target.name == 'getGenericClass' && + if (target.name.name == 'getGenericClass' && isSdkInternalRuntime(target.enclosingLibrary) && positional.length == 1) { var typeArg = positional[0]; @@ -4364,9 +4370,9 @@ class ProgramCompiler case 'Map': case 'HashMap': case 'LinkedHashMap': - if (ctor.name == '') { + if (ctor.name.name == '') { return js.call('new #.new()', _emitMapImplType(type)); - } else if (ctor.name == 'identity') { + } else if (ctor.name.name == 'identity') { return js.call( 'new #.new()', _emitMapImplType(type, identity: true)); } @@ -4374,15 +4380,15 @@ class ProgramCompiler case 'Set': case 'HashSet': case 'LinkedHashSet': - if (ctor.name == '') { + if (ctor.name.name == '') { return js.call('new #.new()', _emitSetImplType(type)); - } else if (ctor.name == 'identity') { + } else if (ctor.name.name == 'identity') { return js.call( 'new #.new()', _emitSetImplType(type, identity: true)); } break; case 'List': - if (ctor.name == '' && type is InterfaceType) { + if (ctor.name.name == '' && type is InterfaceType) { return _emitList(type.typeArguments[0], []); } break; @@ -4515,9 +4521,9 @@ class ProgramCompiler @override visitAsExpression(AsExpression node) { Expression fromExpr = node.operand; - var from = fromExpr.getStaticType(types); var to = node.type; var jsFrom = _visitAndMarkExpression(fromExpr); + var from = fromExpr.getStaticType(types); // If the check was put here by static analysis to ensure soundness, we // can't skip it. For example, one could implement covariant generic caller @@ -4536,10 +4542,6 @@ class ProgramCompiler // c.add('hi); // } // - // NOTE: due to implementation details, we do not currently reify the the - // `C.add` check in CoercionReifier, so it does not reach this point; - // rather we check for it explicitly when emitting methods and fields. - // However we do reify the `c.f` check, so we must not eliminate it. var isTypeError = node.isTypeError; if (!isTypeError && types.isSubtypeOf(from, to)) return jsFrom; @@ -4865,13 +4867,12 @@ bool _isInlineJSFunction(Statement body) { if (statements.length != 1) return false; body = statements[0]; } - if (body is ReturnStatement) { - var e = body.expression; - return e is MethodInvocation && isInlineJS(e.interfaceTarget); - } - return false; + return body is ReturnStatement && _isInlineJSCall(body.expression); } +bool _isInlineJSCall(Expression expr) => + expr is StaticInvocation && isInlineJS(expr.target); + /// Return true if this is one of the methods/properties on all Dart Objects /// (toString, hashCode, noSuchMethod, runtimeType). /// diff --git a/pkg/dev_compiler/lib/src/kernel/kernel_helpers.dart b/pkg/dev_compiler/lib/src/kernel/kernel_helpers.dart index c555f217591..7fc8071052e 100644 --- a/pkg/dev_compiler/lib/src/kernel/kernel_helpers.dart +++ b/pkg/dev_compiler/lib/src/kernel/kernel_helpers.dart @@ -248,5 +248,5 @@ Expression getInvocationReceiver(InvocationExpression node) => bool isInlineJS(Member e) => e is Procedure && - e.name == 'JS' && + e.name.name == 'JS' && e.enclosingLibrary.importUri.toString() == 'dart:_foreign_helper'; diff --git a/pkg/dev_compiler/lib/src/kernel/nullable_inference.dart b/pkg/dev_compiler/lib/src/kernel/nullable_inference.dart index 26ab417acda..bdfd687c42e 100644 --- a/pkg/dev_compiler/lib/src/kernel/nullable_inference.dart +++ b/pkg/dev_compiler/lib/src/kernel/nullable_inference.dart @@ -164,13 +164,6 @@ class NullableInference extends ExpressionVisitor { var target = node.target; if (target == types.coreTypes.identicalProcedure) return false; if (isInlineJS(target)) { - // Fix types for JS builtin calls. - // - // This code was taken from analyzer. It's not super sophisticated: - // only looks for the type name in dart:core, so we just copy it here. - // - // TODO(jmesserly): we'll likely need something that can handle a wider - // variety of types, especially when we get to JS interop. var args = node.arguments.positional; var first = args.isNotEmpty ? args.first : null; if (first is StringLiteral) { diff --git a/pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart b/pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart index 41af78b2ec5..7e044918a44 100644 --- a/pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart +++ b/pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/types.dart @@ -577,7 +577,7 @@ class GenericFunctionType extends AbstractFunctionType { var typeBounds = instantiateTypeBounds(typeFormals); for (int i = 0, n = typeFormals.length; i < n; i++) { if (i != 0) s += ", "; - s += JS('String', '#[#].name', typeFormals, i); + s += JS('!', '#[#].name', typeFormals, i); var typeBound = typeBounds[i]; if (!identical(typeBound, _dynamic)) { s += " extends $typeBound"; diff --git a/pkg/dev_compiler/tool/input_sdk/private/foreign_helper.dart b/pkg/dev_compiler/tool/input_sdk/private/foreign_helper.dart index e3e72138828..c5e77b72737 100644 --- a/pkg/dev_compiler/tool/input_sdk/private/foreign_helper.dart +++ b/pkg/dev_compiler/tool/input_sdk/private/foreign_helper.dart @@ -106,7 +106,7 @@ library dart._foreign_helper; */ // Add additional optional arguments if needed. The method is treated internally // as a variable argument method. -JS(String typeDescription, String codeTemplate, +T JS(String typeDescription, String codeTemplate, [arg0, arg1, arg2, diff --git a/pkg/dev_compiler/tool/input_sdk/private/js_number.dart b/pkg/dev_compiler/tool/input_sdk/private/js_number.dart index 29b43ecc395..fa8a5d8e86c 100644 --- a/pkg/dev_compiler/tool/input_sdk/private/js_number.dart +++ b/pkg/dev_compiler/tool/input_sdk/private/js_number.dart @@ -202,7 +202,7 @@ class JSNumber extends Interceptor implements int, double { int exponent = JS("int", "+#", match[3]); if (match[2] != null) { result = JS('String', '# + #', result, match[2]); - exponent -= JS('int', '#.length', match[2]); + exponent -= JS('!', '#.length', match[2]); } return result + "0" * exponent; }