From d15da7d9000381ed74520995d32168db389eaa9c Mon Sep 17 00:00:00 2001 From: Stephen Adams Date: Thu, 13 Apr 2017 12:45:31 -0700 Subject: [PATCH] dart2js: Capture typedef arguments once The type Map>> contains one type variable referenced twice, so there are two inputs into the HTypeInfoExpression instruction. If Foo is a typedef, T can be reused, e.g. typedef E Foo(E a, E b); As the typedef is expanded (to Function(Set, Set) => Set) it should not consume additional types from the to-level input. We prevent this by capturing the types and using the captured type expressions inside the typedef expansion. TODO: We should make the type subexpression Foo<...> be a second HTypeInfoExpression, with Set as its input (a third HTypeInfoExpression). This would share all the Set subexpressions instead of duplicating them. This would require HTypeInfoExpression inputs to correspond to type variables AND typedefs. BUG= https://github.com/dart-lang/sdk/issues/28749 R=efortuna@google.com Review-Url: https://codereview.chromium.org/2812393003 . --- .../lib/src/js_backend/runtime_types.dart | 47 ++++++++++++++- pkg/js_ast/lib/src/nodes.dart | 2 + pkg/js_ast/lib/src/printer.dart | 9 +++ tests/compiler/dart2js_extra/28749_test.dart | 57 +++++++++++++++++++ 4 files changed, 113 insertions(+), 2 deletions(-) create mode 100644 tests/compiler/dart2js_extra/28749_test.dart diff --git a/pkg/compiler/lib/src/js_backend/runtime_types.dart b/pkg/compiler/lib/src/js_backend/runtime_types.dart index 923148ad7a4..e9c66980cc5 100644 --- a/pkg/compiler/lib/src/js_backend/runtime_types.dart +++ b/pkg/compiler/lib/src/js_backend/runtime_types.dart @@ -878,6 +878,8 @@ class TypeRepresentationGenerator implements DartTypeVisitor { OnVariableCallback onVariable; ShouldEncodeTypedefCallback shouldEncodeTypedef; + Map typedefBindings; + TypeRepresentationGenerator(this.namer, this.emitter); /** @@ -888,6 +890,7 @@ class TypeRepresentationGenerator implements DartTypeVisitor { ResolutionDartType type, OnVariableCallback onVariable, ShouldEncodeTypedefCallback encodeTypedef) { + assert(typedefBindings == null); this.onVariable = onVariable; this.shouldEncodeTypedef = (encodeTypedef != null) ? encodeTypedef @@ -906,6 +909,10 @@ class TypeRepresentationGenerator implements DartTypeVisitor { visit(ResolutionDartType type, [_]) => type.accept(this, null); visitTypeVariableType(ResolutionTypeVariableType type, _) { + if (typedefBindings != null) { + assert(typedefBindings[type] != null); + return typedefBindings[type]; + } return onVariable(type); } @@ -1001,6 +1008,42 @@ class TypeRepresentationGenerator implements DartTypeVisitor { visitTypedefType(ResolutionTypedefType type, _) { bool shouldEncode = shouldEncodeTypedef(type); ResolutionDartType unaliasedType = type.unaliased; + + var oldBindings = typedefBindings; + if (typedefBindings == null) { + // First level typedef - capture arguments for re-use within typedef body. + // + // The type `Map>>` contains one type variable referenced + // twice, so there are two inputs into the HTypeInfoExpression + // instruction. + // + // If Foo is a typedef, T can be reused, e.g. + // + // typedef E Foo(E a, E b); + // + // As the typedef is expanded (to (Set, Set) => Set) it should + // not consume additional types from the to-level input. We prevent this + // by capturing the types and using the captured type expressions inside + // the typedef expansion. + // + // TODO(sra): We should make the type subexpression Foo<...> be a second + // HTypeInfoExpression, with Set as its input (a third + // HTypeInfoExpression). This would share all the Set subexpressions + // instead of duplicating them. This would require HTypeInfoExpression + // inputs to correspond to type variables AND typedefs. + typedefBindings = {}; + type.forEachTypeVariable((variable) { + if (variable is! MethodTypeVariableType) { + typedefBindings[variable] = onVariable(variable); + } + }); + } + + jsAst.Expression finish(jsAst.Expression result) { + typedefBindings = oldBindings; + return result; + } + if (shouldEncode) { jsAst.ObjectInitializer initializer = unaliasedType.accept(this, null); // We have to encode the aliased type. @@ -1011,9 +1054,9 @@ class TypeRepresentationGenerator implements DartTypeVisitor { // Add it to the function-type object. jsAst.LiteralString tag = js.string(namer.typedefTag); initializer.properties.add(new jsAst.Property(tag, encodedTypedef)); - return initializer; + return finish(initializer); } else { - return unaliasedType.accept(this, null); + return finish(unaliasedType.accept(this, null)); } } } diff --git a/pkg/js_ast/lib/src/nodes.dart b/pkg/js_ast/lib/src/nodes.dart index c43a2519687..3ab8e6bb992 100644 --- a/pkg/js_ast/lib/src/nodes.dart +++ b/pkg/js_ast/lib/src/nodes.dart @@ -233,6 +233,8 @@ abstract class Node { Statement toStatement() { throw new UnsupportedError('toStatement'); } + + String debugPrint() => DebugPrint(this); } class Program extends Node { diff --git a/pkg/js_ast/lib/src/printer.dart b/pkg/js_ast/lib/src/printer.dart index d4b09377f16..041b2dc5b8b 100644 --- a/pkg/js_ast/lib/src/printer.dart +++ b/pkg/js_ast/lib/src/printer.dart @@ -61,6 +61,15 @@ class SimpleJavaScriptPrintingContext extends JavaScriptPrintingContext { String getText() => buffer.toString(); } +String DebugPrint(Node node) { + JavaScriptPrintingOptions options = new JavaScriptPrintingOptions(); + SimpleJavaScriptPrintingContext context = + new SimpleJavaScriptPrintingContext(); + Printer printer = new Printer(options, context); + printer.visit(node); + return context.getText(); +} + class Printer implements NodeVisitor { final JavaScriptPrintingOptions options; final JavaScriptPrintingContext context; diff --git a/tests/compiler/dart2js_extra/28749_test.dart b/tests/compiler/dart2js_extra/28749_test.dart new file mode 100644 index 00000000000..b75dd271a37 --- /dev/null +++ b/tests/compiler/dart2js_extra/28749_test.dart @@ -0,0 +1,57 @@ +// Copyright (c) 2017, 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. + +// Regression test for http://dartbug.com/28749. +// +// This would crash at compile time because inner typedefs remain after calling +// [type.unalias]. Expanding the typedef causes the inputs to be used multiple +// types, breaking the invariant of HTypeInfoExpression that the type variable +// occurrences correspond to inputs. + +import 'package:expect/expect.dart'; + +typedef void F(T value); +typedef F Converter(F function); +typedef Converter ConvertFactory(int input); + +class B { + final field = new Wrap>(); + @NoInline() + B(); +} + +class Wrap { + @NoInline() + Wrap(); +} + +foo(int x) { + if (x == 0) + return new Wrap>().runtimeType; + else + return new B().field.runtimeType; +} + +void main() { + var name = '${Wrap}'; + if (name.length < 4) return; // minified. + + Expect.equals( + 'Wrap<(int) => ((int) => void) => (int) => void>', + '${new B().field.runtimeType}', + ); + Expect.equals( + 'Wrap<(int) => ((bool) => void) => (bool) => void>', + '${new B().field.runtimeType}', + ); + + Expect.equals( + 'Wrap<(int) => ((dynamic) => void) => (dynamic) => void>', + '${foo(0)}', + ); + Expect.equals( + 'Wrap<(int) => ((dynamic) => void) => (dynamic) => void>', + '${foo(1)}', + ); +}