From 6a402120a11829b98fc1c5039be0dcb0bcf7a6a7 Mon Sep 17 00:00:00 2001 From: tbreisacher Date: Tue, 1 Mar 2016 15:10:51 -0800 Subject: [PATCH] Allow iteration over the 'arguments' object even if it escapes and cannot be detected. Note that this causes some incorrect code to "work" when transpiled. let obj = { 0: 'x', 1: 'y', length: 2, }; for (let x of obj) {} should throw a TypeError, but will not when transpiled. However, if typechecking is enabled there will be a compiler error. Fixes https://github.com/google/closure-compiler/issues/1303 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=116058485 --- externs/es6.js | 2 +- .../jscomp/Es6RewriteDestructuring.java | 2 +- .../jscomp/Es6RewriteGenerators.java | 2 +- .../javascript/jscomp/Es6ToEs3Converter.java | 33 +--- .../javascript/jscomp/js/es6/runtime.js | 23 +-- .../javascript/jscomp/js/es6_runtime.js | 16 +- .../jscomp/Es6RewriteDestructuringTest.java | 3 +- .../jscomp/Es6ToEs3ConverterTest.java | 142 ++++++++++++------ 8 files changed, 111 insertions(+), 112 deletions(-) diff --git a/externs/es6.js b/externs/es6.js index 9770309c4a0..e5f13e45641 100644 --- a/externs/es6.js +++ b/externs/es6.js @@ -142,7 +142,7 @@ Generator.prototype.return = function(value) {}; Generator.prototype.throw = function(exception) {}; -// TODO(johnlenz): Array should be Iterable. +// TODO(johnlenz): Array and Arguments should be Iterable. diff --git a/src/com/google/javascript/jscomp/Es6RewriteDestructuring.java b/src/com/google/javascript/jscomp/Es6RewriteDestructuring.java index 78741b218ce..fa32cca0f3a 100644 --- a/src/com/google/javascript/jscomp/Es6RewriteDestructuring.java +++ b/src/com/google/javascript/jscomp/Es6RewriteDestructuring.java @@ -294,7 +294,7 @@ private void visitArrayPattern(NodeTraversal t, Node arrayPattern, Node parent) String tempVarName = DESTRUCTURING_TEMP_VAR + (destructuringVarCounter++); Node tempDecl = IR.var( IR.name(tempVarName), - makeIterator(t, compiler, rhs.detachFromParent())); + makeIterator(compiler, rhs.detachFromParent())); tempDecl.useSourceInfoIfMissingFromForTree(arrayPattern); nodeToDetach.getParent().addChildBefore(tempDecl, nodeToDetach); diff --git a/src/com/google/javascript/jscomp/Es6RewriteGenerators.java b/src/com/google/javascript/jscomp/Es6RewriteGenerators.java index 2c5fab9f8a1..5e3c5babe17 100644 --- a/src/com/google/javascript/jscomp/Es6RewriteGenerators.java +++ b/src/com/google/javascript/jscomp/Es6RewriteGenerators.java @@ -179,7 +179,7 @@ private void visitYieldFor(NodeTraversal t, Node n, Node parent) { Node enclosingStatement = NodeUtil.getEnclosingStatement(n); Node generator = IR.var( IR.name(GENERATOR_YIELD_ALL_NAME), - makeIterator(t, compiler, n.removeFirstChild())); + makeIterator(compiler, n.removeFirstChild())); Node entryDecl = IR.var(IR.name(GENERATOR_YIELD_ALL_ENTRY)); Node assignIterResult = IR.assign( diff --git a/src/com/google/javascript/jscomp/Es6ToEs3Converter.java b/src/com/google/javascript/jscomp/Es6ToEs3Converter.java index 98226fac9e8..992ec75431e 100644 --- a/src/com/google/javascript/jscomp/Es6ToEs3Converter.java +++ b/src/com/google/javascript/jscomp/Es6ToEs3Converter.java @@ -259,7 +259,7 @@ private void visitForOf(NodeTraversal t, Node node, Node parent) { Node iterResult = IR.name(ITER_RESULT + variableName); iterResult.makeNonIndexable(); - Node init = IR.var(iterName.cloneTree(), makeIterator(t, compiler, iterable)); + Node init = IR.var(iterName.cloneTree(), makeIterator(compiler, iterable)); Node initIterResult = iterResult.cloneTree(); initIterResult.addChildToFront(getNext.cloneTree()); init.addChildToBack(initIterResult); @@ -397,7 +397,7 @@ private void visitArrayLitOrCallWithSpread(NodeTraversal t, Node node, Node pare currGroup = null; } compiler.needsEs6Runtime = true; - groups.add(arrayFromIterable(t, compiler, currElement.removeFirstChild())); + groups.add(arrayFromIterable(compiler, currElement.removeFirstChild())); } else { if (currGroup == null) { currGroup = IR.arraylit(); @@ -937,36 +937,17 @@ private void cannotConvertYet(Node n, String feature) { } /** - * Returns a call to {@code $jscomp.makeIterator} with {@code iterable} as its argument, unless - * {@code iterable} is the special {@code arguments} variable, in which case the returned Node is: - * {@code $jscomp.makeIterator($jscomp.arrayFromArguments(iterable))}. + * Returns a call to {@code $jscomp.makeIterator} with {@code iterable} as its argument. */ - static Node makeIterator(NodeTraversal t, AbstractCompiler compiler, Node iterable) { - if (iterable.isName()) { - Var var = t.getScope().getVar(iterable.getString()); - if (var != null && var.isArguments()) { - iterable = IR.call( - NodeUtil.newQName(compiler, "$jscomp.arrayFromArguments"), - iterable); - } - } + static Node makeIterator(AbstractCompiler compiler, Node iterable) { return callEs6RuntimeFunction(compiler, iterable, "makeIterator"); } /** - * Returns a call to $jscomp.arrayFromIterable with {@code iterable} as its argument, unless - * {@code iterable} is the special {@code arguments} variable, in which case - * {@code $jscomp.arrayFromArguments} is called instead. + * Returns a call to $jscomp.arrayFromIterable with {@code iterable} as its argument. */ - private static Node arrayFromIterable(NodeTraversal t, AbstractCompiler compiler, Node iterable) { - String fnName = "arrayFromIterable"; - if (iterable.isName()) { - Var var = t.getScope().getVar(iterable.getString()); - if (var != null && var.isArguments()) { - fnName = "arrayFromArguments"; - } - } - return callEs6RuntimeFunction(compiler, iterable, fnName); + private static Node arrayFromIterable(AbstractCompiler compiler, Node iterable) { + return callEs6RuntimeFunction(compiler, iterable, "arrayFromIterable"); } private static Node callEs6RuntimeFunction( diff --git a/src/com/google/javascript/jscomp/js/es6/runtime.js b/src/com/google/javascript/jscomp/js/es6/runtime.js index 5306036c2ed..44f885f9944 100644 --- a/src/com/google/javascript/jscomp/js/es6/runtime.js +++ b/src/com/google/javascript/jscomp/js/es6/runtime.js @@ -90,7 +90,7 @@ $jscomp.initSymbolIterator = function() { /** * Creates an iterator for the given iterable. * - * @param {string|!Array|!Iterable|!Iterator} iterable + * @param {string|!Array|!Iterable|!Iterator|!Arguments} iterable * @return {!Iterator} * @template T * @suppress {reportUnknownTypes} @@ -101,10 +101,7 @@ $jscomp.makeIterator = function(iterable) { if (iterable[$jscomp.global.Symbol.iterator]) { return iterable[$jscomp.global.Symbol.iterator](); } - if (!(iterable instanceof Array) && typeof iterable != 'string' && - !(iterable instanceof String)) { - throw new TypeError(iterable + ' is not iterable'); - } + let index = 0; return /** @type {!Iterator} */ ({ next() { @@ -142,7 +139,7 @@ $jscomp.arrayFromIterator = function(iterator) { /** * Copies the values from an Iterable into an Array. - * @param {string|!Array|!Iterable} iterable + * @param {string|!Array|!Iterable|!Arguments} iterable * @return {!Array} * @template T */ @@ -155,20 +152,6 @@ $jscomp.arrayFromIterable = function(iterable) { }; -/** - * Copies the values from an Arguments object into an Array. - * @param {!Arguments} args - * @return {!Array} - */ -$jscomp.arrayFromArguments = function(args) { - const result = []; - for (let i = 0; i < args.length; i++) { - result.push(args[i]); - } - return result; -}; - - /** * Inherit the prototype methods and static methods from one constructor * into another. diff --git a/src/com/google/javascript/jscomp/js/es6_runtime.js b/src/com/google/javascript/jscomp/js/es6_runtime.js index 6726387edd4..31961212fcd 100644 --- a/src/com/google/javascript/jscomp/js/es6_runtime.js +++ b/src/com/google/javascript/jscomp/js/es6_runtime.js @@ -50,15 +50,12 @@ /**@suppress {reportUnknownTypes} @template T -@param {(string|!Array|!Iterable|!Iterator)} iterable +@param {(string|!Array|!Iterable|!Iterator|!Arguments)} iterable @return {!Iterator} */$jscomp.makeIterator = function(iterable) { $jscomp.initSymbolIterator(); if (iterable[$jscomp.global.Symbol.iterator]) { return iterable[$jscomp.global.Symbol.iterator](); } - if (!(iterable instanceof Array) && typeof iterable != "string" && !(iterable instanceof String)) { - throw new TypeError(iterable + " is not iterable"); - } var index = 0; return /**@type {!Iterator} */({next:function() { if (index == iterable.length) { @@ -81,7 +78,7 @@ }; /**@template T -@param {(string|!Array|!Iterable)} iterable +@param {(string|!Array|!Iterable|!Arguments)} iterable @return {!Array} */$jscomp.arrayFromIterable = function(iterable) { if (iterable instanceof Array) { return iterable; @@ -90,15 +87,6 @@ } }; /** -@param {!Arguments} args -@return {!Array} */$jscomp.arrayFromArguments = function(args) { - /**@const */var result = []; - for (var i = 0;i < args.length;i++) { - result.push(args[i]); - } - return result; -}; -/** @param {!Function} childCtor @param {!Function} parentCtor */$jscomp.inherits = function(childCtor, parentCtor) { /**@constructor */function tempCtor() { diff --git a/test/com/google/javascript/jscomp/Es6RewriteDestructuringTest.java b/test/com/google/javascript/jscomp/Es6RewriteDestructuringTest.java index e2c92a06541..7920a356802 100644 --- a/test/com/google/javascript/jscomp/Es6RewriteDestructuringTest.java +++ b/test/com/google/javascript/jscomp/Es6RewriteDestructuringTest.java @@ -362,8 +362,7 @@ public void testArrayDestructuringArguments() { "function f() { var [x, y] = arguments; }", LINE_JOINER.join( "function f() {", - " var $jscomp$destructuring$var0 = $jscomp.makeIterator(", - " $jscomp.arrayFromArguments(arguments));", + " var $jscomp$destructuring$var0 = $jscomp.makeIterator(arguments);", " var x = $jscomp$destructuring$var0.next().value;", " var y = $jscomp$destructuring$var0.next().value;", "}")); diff --git a/test/com/google/javascript/jscomp/Es6ToEs3ConverterTest.java b/test/com/google/javascript/jscomp/Es6ToEs3ConverterTest.java index c6bebb1f5bc..93d66976575 100644 --- a/test/com/google/javascript/jscomp/Es6ToEs3ConverterTest.java +++ b/test/com/google/javascript/jscomp/Es6ToEs3ConverterTest.java @@ -29,6 +29,37 @@ public final class Es6ToEs3ConverterTest extends CompilerTestCase { private static final String EXTERNS_BASE = LINE_JOINER.join( + "/** @constructor @template T */", + "function Arguments() {}", + "", + "/**", + " * @constructor", + " * @param {...*} var_args", + " * @return {!Array}", + " * @template T", + " */", + "function Array(var_args) {}", + "", + "/** @constructor @template T */", + "function Iterable() {}", + "", + "/** @constructor @template T */", + "function Iterator() {}", + "", + "Iterator.prototype.next = function() {};", + "", + "/**", + " * @record", + " * @template VALUE", + " */", + "function IIterableResult() {}", + "", + "/** @type {boolean} */", + "IIterableResult.prototype.done;", + "", + "/** @type {VALUE} */", + "IIterableResult.prototype.value;", + "", "/**", " * @param {...*} var_args", " * @return {*}", @@ -47,8 +78,29 @@ public final class Es6ToEs3ConverterTest extends CompilerTestCase { // Stub out just enough of es6_runtime.js to satisfy the typechecker. // In a real compilation, the entire library will be loaded by // the InjectEs6RuntimeLibrary pass. - "$jscomp.inherits = function(x,y) {};", - "$jscomp.arrayFromIterable = function(x) {};"); + "/**", + " * @param {function(new: ?)} subclass", + " * @param {function(new: ?)} superclass", + " */", + "$jscomp.inherits = function(subclass, superclass) {};", + "", + "/**", + " * @param {string|!Array|!Iterable|!Iterator|!Arguments} iterable", + " * @return {!Iterator}", + " * @template T", + " */", + "$jscomp.makeIterator = function(iterable) {};", + "", + "/**", + " * @param {string|!Array|!Iterable|!Iterator|!Arguments} iterable", + " * @return {!Array}", + " * @template T", + " */", + "$jscomp.arrayFromIterable = function(iterable) {};"); + + public Es6ToEs3ConverterTest() { + super(EXTERNS_BASE); + } @Override public void setUp() { @@ -849,7 +901,6 @@ public void testInvalidClassUse() { enableTypeCheck(); test( - EXTERNS_BASE, LINE_JOINER.join( "/** @constructor @struct */", "function Foo() {}", @@ -863,24 +914,18 @@ public void testInvalidClassUse() { "/** @constructor @struct @extends {Foo} */", "var Sub=function(var_args) { Foo.apply(this, arguments); }", "$jscomp.inherits(Sub, Foo);", - "(new Sub).f();"), - null, - null); + "(new Sub).f();")); - test( - EXTERNS_BASE, + testWarning( LINE_JOINER.join( "/** @constructor @struct */", "function Foo() {}", "Foo.f = function() {};", "class Sub extends Foo {}", "Sub.f();"), - null, - null, TypeCheck.INEXISTENT_PROPERTY); test( - EXTERNS_BASE, LINE_JOINER.join( "/** @constructor */", "function Foo() {}", @@ -892,9 +937,7 @@ public void testInvalidClassUse() { "Foo.f = function() {};", "/** @constructor @struct @extends {Foo} */", "var Sub = function(var_args) { Foo.apply(this, arguments); };", - "$jscomp.inherits(Sub, Foo);"), - null, - null); + "$jscomp.inherits(Sub, Foo);")); } /** @@ -1109,7 +1152,6 @@ public void testClassEs5GetterSetterIncorrectTypes() { // Using @type instead of @return on a getter. test( - EXTERNS_BASE, "class C { /** @type {string} */ get value() { } }", LINE_JOINER.join( "/** @constructor @struct */", @@ -1129,7 +1171,6 @@ public void testClassEs5GetterSetterIncorrectTypes() { // Using @type instead of @param on a setter. test( - EXTERNS_BASE, "class C { /** @type {string} */ set value(v) { } }", LINE_JOINER.join( "/** @constructor @struct */", @@ -1559,20 +1600,17 @@ public void testSpreadArray() { "function f() { return [...arguments]; };", LINE_JOINER.join( "function f() {", - " return [].concat($jscomp.arrayFromArguments(arguments));", + " return [].concat($jscomp.arrayFromIterable(arguments));", "};")); test( "function f() { return [...arguments, 2]; };", LINE_JOINER.join( "function f() {", - " return [].concat($jscomp.arrayFromArguments(arguments), [2]);", + " return [].concat($jscomp.arrayFromIterable(arguments), [2]);", "};")); } - public void testUnsupportedUsesOfArguments() { - // In this case, we don't recognize that 'x' is be an 'Arguments' object, so we produce - // code that will ultimately fail with a TypeError at runtime. However, this code fails the - // CheckArguments lint check so it should happen rarely. + public void testArgumentsEscaped() { test( LINE_JOINER.join( "function g(x) {", @@ -1590,6 +1628,19 @@ public void testUnsupportedUsesOfArguments() { "}")); } + public void testForOfOnNonIterable() { + enableTypeCheck(); + testWarning( + LINE_JOINER.join( + "var arrayLike = {", + " 0: 'x',", + " 1: 'y',", + " length: 2,", + "};", + "for (var x of arrayLike) {}"), + TypeValidator.TYPE_MISMATCH_WARNING); + } + public void testSpreadCall() { test("f(...arr);", "f.apply(null, [].concat($jscomp.arrayFromIterable(arr)));"); test("f(0, ...g());", "f.apply(null, [].concat([0], $jscomp.arrayFromIterable(g())));"); @@ -1627,8 +1678,7 @@ public void testSpreadCall() { enableTypeCheck(); - test( - EXTERNS_BASE, + testWarning( LINE_JOINER.join( "class C {}", "class Factory {", @@ -1637,31 +1687,29 @@ public void testSpreadCall() { "}", "var arr = [1,2]", "Factory.create().m(...arr);"), - null, - null, TypeCheck.INEXISTENT_PROPERTY); - test(EXTERNS_BASE, LINE_JOINER.join( - "class C { m(a) {} }", - "class Factory {", - " /** @return {C} */", - " static create() {return new C()}", - "}", - "var arr = [1,2]", - "Factory.create().m(...arr);" - ), LINE_JOINER.join( - "/** @constructor @struct */", - "var C = function() {};", - "C.prototype.m = function(a) {};", - "/** @constructor @struct */", - "var Factory = function() {};", - "/** @return {C} */", - "Factory.create = function() {return new C()};", - "var arr = [1,2]", - "var $jscomp$spread$args0;", - "($jscomp$spread$args0 = Factory.create()).m.apply(", - " $jscomp$spread$args0, [].concat($jscomp.arrayFromIterable(arr)));" - ), null, null); + test( + LINE_JOINER.join( + "class C { m(a) {} }", + "class Factory {", + " /** @return {C} */", + " static create() {return new C()}", + "}", + "var arr = [1,2]", + "Factory.create().m(...arr);"), + LINE_JOINER.join( + "/** @constructor @struct */", + "var C = function() {};", + "C.prototype.m = function(a) {};", + "/** @constructor @struct */", + "var Factory = function() {};", + "/** @return {C} */", + "Factory.create = function() {return new C()};", + "var arr = [1,2]", + "var $jscomp$spread$args0;", + "($jscomp$spread$args0 = Factory.create()).m.apply(", + " $jscomp$spread$args0, [].concat($jscomp.arrayFromIterable(arr)));")); } public void testSpreadNew() {