Skip to content

Fix #498: Optimize rest arguments #19330

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13248,6 +13248,14 @@ namespace ts {
// or parameter. The flow container is the innermost function starting with which we analyze the control
// flow graph to determine the control flow based type.
const isParameter = getRootDeclaration(declaration).kind === SyntaxKind.Parameter;

if (isParameter) {
const parameterDeclaration = getRootDeclaration(declaration) as ParameterDeclaration;
if (parameterDeclaration.dotDotDotToken) {
getNodeLinks(parameterDeclaration).flags |= NodeCheckFlags.EmitRestParam;
}
}

const declarationContainer = getControlFlowContainer(declaration);
let flowContainer = getControlFlowContainer(node);
const isOuterVariable = flowContainer !== declarationContainer;
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/transformers/es2015.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1341,7 +1341,7 @@ namespace ts {
* synthesized call to `super`
*/
function shouldAddRestParameter(node: ParameterDeclaration, inConstructorWithSynthesizedSuper: boolean) {
return node && node.dotDotDotToken && node.name.kind === SyntaxKind.Identifier && !inConstructorWithSynthesizedSuper;
return node && node.dotDotDotToken && node.name.kind === SyntaxKind.Identifier && !inConstructorWithSynthesizedSuper && resolver.getNodeCheckFlags(node) & NodeCheckFlags.EmitRestParam;
}

/**
Expand Down
1 change: 1 addition & 0 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3313,6 +3313,7 @@ namespace ts {
AssignmentsMarked = 0x00400000, // Parameter assignments have been marked
ClassWithConstructorReference = 0x00800000, // Class that contains a binding to its constructor inside of the class body.
ConstructorReferenceInClass = 0x01000000, // Binding to a class constructor inside of the class's body.
EmitRestParam = 0x02000000, // Rest parameter used in body.
}

/* @internal */
Expand Down
14 changes: 2 additions & 12 deletions tests/baselines/reference/accessorWithRestParam.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,22 +9,12 @@ var C = /** @class */ (function () {
function C() {
}
Object.defineProperty(C.prototype, "X", {
set: function () {
var v = [];
for (var _i = 0; _i < arguments.length; _i++) {
v[_i] = arguments[_i];
}
},
set: function () { },
Copy link

@Jessidhia Jessidhia Oct 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks wrong.

This test file is wrong.

class C { set X(...v) {} }, which this file is testing, is invalid syntax. set methods, on either classes or objects, only accept exactly one valid FormalParameter, which does not allow for FunctionRestParameter.

See https://tc39.github.io/ecma262/#prod-PropertySetParameterList

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, thanks.

Yet TypeScript does not allow set to have a rest parameter anyway (i.e. the test code won't compile).

So I guess I'll revert this baseline and add it to the list above, where I ask @mhegazy about whether or not to apply the optimization in error cases.

Copy link

@Jessidhia Jessidhia Oct 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, the transform looks correct -- the test file having a valid-looking output at all is wrong. Invalid syntax should either throw or stay as invalid syntax 😄

This is probably out of scope of this PR, though, but the change to that file helped notice it.

enumerable: true,
configurable: true
});
Object.defineProperty(C, "X", {
set: function () {
var v2 = [];
for (var _i = 0; _i < arguments.length; _i++) {
v2[_i] = arguments[_i];
}
},
set: function () { },
enumerable: true,
configurable: true
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,5 @@ panic([], 'one', 'two');


//// [arrayLiteralInNonVarArgParameter.js]
function panic(val) {
var opt = [];
for (var _i = 1; _i < arguments.length; _i++) {
opt[_i - 1] = arguments[_i];
}
}
function panic(val) { }
panic([], 'one', 'two');
Original file line number Diff line number Diff line change
Expand Up @@ -49,49 +49,19 @@ var a4: (x?: number, y?: string, ...z: number[]) => number;
// call signatures in derived types must have the same or fewer optional parameters as the target for assignment
var a; // ok, same number of required params
a = function () { return 1; }; // ok, same number of required params
a = function () {
var args = [];
for (var _i = 0; _i < arguments.length; _i++) {
args[_i] = arguments[_i];
}
return 1;
}; // ok, same number of required params
a = function () {
var args = [];
for (var _i = 0; _i < arguments.length; _i++) {
args[_i] = arguments[_i];
}
return 1;
}; // error, type mismatch
a = function () { return 1; }; // ok, same number of required params
a = function () { return 1; }; // error, type mismatch
a = function (x) { return 1; }; // ok, same number of required params
a = function (x, y, z) { return 1; }; // ok, same number of required params
a = function (x) { return 1; }; // ok, rest param corresponds to infinite number of params
a = function (x) { return 1; }; // error, incompatible type
var a2;
a2 = function () { return 1; }; // ok, fewer required params
a2 = function () {
var args = [];
for (var _i = 0; _i < arguments.length; _i++) {
args[_i] = arguments[_i];
}
return 1;
}; // ok, fewer required params
a2 = function () { return 1; }; // ok, fewer required params
a2 = function (x) { return 1; }; // ok, fewer required params
a2 = function (x) { return 1; }; // ok, same number of required params
a2 = function (x) {
var args = [];
for (var _i = 1; _i < arguments.length; _i++) {
args[_i - 1] = arguments[_i];
}
return 1;
}; // ok, same number of required params
a2 = function (x) {
var args = [];
for (var _i = 1; _i < arguments.length; _i++) {
args[_i - 1] = arguments[_i];
}
return 1;
}; // should be type mismatch error
a2 = function (x) { return 1; }; // ok, same number of required params
a2 = function (x) { return 1; }; // should be type mismatch error
a2 = function (x, y) { return 1; }; // ok, rest param corresponds to infinite number of params
a2 = function (x, y) { return 1; }; // ok, same number of required params
var a3;
Expand All @@ -100,24 +70,12 @@ a3 = function (x) { return 1; }; // ok, fewer required params
a3 = function (x) { return 1; }; // ok, same number of required params
a3 = function (x, y) { return 1; }; // ok, all present params match
a3 = function (x, y, z) { return 1; }; // error
a3 = function (x) {
var z = [];
for (var _i = 1; _i < arguments.length; _i++) {
z[_i - 1] = arguments[_i];
}
return 1;
}; // error
a3 = function (x) { return 1; }; // error
a3 = function (x, y, z) { return 1; }; // error
var a4;
a4 = function () { return 1; }; // ok, fewer required params
a4 = function (x, y) { return 1; }; // error, type mismatch
a4 = function (x) { return 1; }; // ok, all present params match
a4 = function (x, y) { return 1; }; // error, second param has type mismatch
a4 = function (x, y) { return 1; }; // ok, same number of required params with matching types
a4 = function (x) {
var args = [];
for (var _i = 1; _i < arguments.length; _i++) {
args[_i - 1] = arguments[_i];
}
return 1;
}; // error, rest params have type mismatch
a4 = function (x) { return 1; }; // error, rest params have type mismatch
7 changes: 1 addition & 6 deletions tests/baselines/reference/baseTypeAfterDerivedType.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,6 @@ interface Base2 {
var Derived2 = /** @class */ (function () {
function Derived2() {
}
Derived2.prototype.method = function () {
var args = [];
for (var _i = 0; _i < arguments.length; _i++) {
args[_i] = arguments[_i];
}
};
Derived2.prototype.method = function () { };
return Derived2;
}());
8 changes: 0 additions & 8 deletions tests/baselines/reference/callWithSpread.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,6 @@ var __extends = (this && this.__extends) || (function () {
};
})();
function foo(x, y) {
var z = [];
for (var _i = 2; _i < arguments.length; _i++) {
z[_i - 2] = arguments[_i];
}
}
var a;
var z;
Expand Down Expand Up @@ -108,10 +104,6 @@ var C = /** @class */ (function () {
this.foo.apply(this, [x, y].concat(z));
}
C.prototype.foo = function (x, y) {
var z = [];
for (var _i = 2; _i < arguments.length; _i++) {
z[_i - 2] = arguments[_i];
}
};
return C;
}());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,6 @@ var __extends = (this && this.__extends) || (function () {
})();
var Based = /** @class */ (function () {
function Based() {
var arg = [];
for (var _i = 0; _i < arguments.length; _i++) {
arg[_i] = arguments[_i];
}
}
return Based;
}());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,6 @@ var __extends = (this && this.__extends) || (function () {
})();
var Base = /** @class */ (function () {
function Base() {
var arg = [];
for (var _i = 0; _i < arguments.length; _i++) {
arg[_i] = arguments[_i];
}
}
return Base;
}());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,6 @@ var __extends = (this && this.__extends) || (function () {
})();
var Base = /** @class */ (function () {
function Base() {
var arg = [];
for (var _i = 0; _i < arguments.length; _i++) {
arg[_i] = arguments[_i];
}
}
return Base;
}());
Expand Down
12 changes: 0 additions & 12 deletions tests/baselines/reference/collisionArgumentsArrowFunctions.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,27 +18,15 @@ var f2NoError = () => {

//// [collisionArgumentsArrowFunctions.js]
var f1 = function (i) {
var arguments = [];
for (var _i = 1; _i < arguments.length; _i++) {
arguments[_i - 1] = arguments[_i];
}
var arguments; // no error
};
var f12 = function (arguments) {
var rest = [];
for (var _i = 1; _i < arguments.length; _i++) {
rest[_i - 1] = arguments[_i];
}
var arguments = 10; // no error
};
var f1NoError = function (arguments) {
var arguments = 10; // no error
};
var f2 = function () {
var restParameters = [];
for (var _i = 0; _i < arguments.length; _i++) {
restParameters[_i] = arguments[_i];
}
var arguments = 10; // No Error
};
var f2NoError = function () {
Expand Down
24 changes: 0 additions & 24 deletions tests/baselines/reference/collisionArgumentsClassConstructor.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,20 +90,12 @@ declare class c6NoError {
// Constructors
var c1 = /** @class */ (function () {
function c1(i) {
var arguments = [];
for (var _i = 1; _i < arguments.length; _i++) {
arguments[_i - 1] = arguments[_i];
}
var arguments; // no error
}
return c1;
}());
var c12 = /** @class */ (function () {
function c12(arguments) {
var rest = [];
for (var _i = 1; _i < arguments.length; _i++) {
rest[_i - 1] = arguments[_i];
}
var arguments = 10; // no error
}
return c12;
Expand All @@ -116,10 +108,6 @@ var c1NoError = /** @class */ (function () {
}());
var c2 = /** @class */ (function () {
function c2() {
var restParameters = [];
for (var _i = 0; _i < arguments.length; _i++) {
restParameters[_i] = arguments[_i];
}
var arguments = 10; // no error
}
return c2;
Expand All @@ -132,10 +120,6 @@ var c2NoError = /** @class */ (function () {
}());
var c3 = /** @class */ (function () {
function c3(arguments) {
var restParameters = [];
for (var _i = 1; _i < arguments.length; _i++) {
restParameters[_i - 1] = arguments[_i];
}
this.arguments = arguments;
var arguments = 10; // no error
}
Expand All @@ -150,20 +134,12 @@ var c3NoError = /** @class */ (function () {
}());
var c5 = /** @class */ (function () {
function c5(i) {
var arguments = [];
for (var _i = 1; _i < arguments.length; _i++) {
arguments[_i - 1] = arguments[_i];
}
var arguments; // no error
}
return c5;
}());
var c52 = /** @class */ (function () {
function c52(arguments) {
var rest = [];
for (var _i = 1; _i < arguments.length; _i++) {
rest[_i - 1] = arguments[_i];
}
var arguments; // no error
}
return c52;
Expand Down
20 changes: 0 additions & 20 deletions tests/baselines/reference/collisionArgumentsClassMethod.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,34 +53,18 @@ var c1 = /** @class */ (function () {
function c1() {
}
c1.prototype.foo = function (i) {
var arguments = [];
for (var _i = 1; _i < arguments.length; _i++) {
arguments[_i - 1] = arguments[_i];
}
var arguments; // no error
};
c1.prototype.foo1 = function (arguments) {
var rest = [];
for (var _i = 1; _i < arguments.length; _i++) {
rest[_i - 1] = arguments[_i];
}
var arguments = 10; // no error
};
c1.prototype.fooNoError = function (arguments) {
var arguments = 10; // no error
};
c1.prototype.f4 = function (i) {
var arguments = [];
for (var _i = 1; _i < arguments.length; _i++) {
arguments[_i - 1] = arguments[_i];
}
var arguments; // no error
};
c1.prototype.f41 = function (arguments) {
var rest = [];
for (var _i = 1; _i < arguments.length; _i++) {
rest[_i - 1] = arguments[_i];
}
var arguments; // no error
};
c1.prototype.f4NoError = function (arguments) {
Expand All @@ -92,10 +76,6 @@ var c3 = /** @class */ (function () {
function c3() {
}
c3.prototype.foo = function () {
var restParameters = [];
for (var _i = 0; _i < arguments.length; _i++) {
restParameters[_i] = arguments[_i];
}
var arguments = 10; // no error
};
c3.prototype.fooNoError = function () {
Expand Down
Loading