Skip to content

Reduce rest parameter if not referenced #39188

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.

Sign up for GitHub

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 8 commits into from
Closed

Reduce rest parameter if not referenced #39188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
37de252
Reduce rest parameter reduce if not referenced
Kingwl Jun 22, 2020
564ece6
make linter happy
Kingwl Jun 22, 2020
ff4ac8f
Emit node before using
Kingwl Jun 22, 2020
1c1537c
update baseline
Kingwl Jun 22, 2020
d05a052
fix super call break
Kingwl Jun 22, 2020
360e696
Merge branch 'master' into rest_parameter_opt
Kingwl Jul 23, 2020
30cfc5d
Fix some case
Kingwl Jul 23, 2020
4112a7b
accept baseline
Kingwl Jul 23, 2020
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
43 changes: 39 additions & 4 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22101,6 +22101,8 @@ namespace ts {
return getTypeOfSymbol(symbol);
}

checkIdentifierForRestParameter(node, symbol);

// We should only mark aliases as referenced if there isn't a local value declaration
// for the symbol. Also, don't mark any property access expression LHS - checkPropertyAccessExpression will handle that
if (!(node.parent && isPropertyAccessExpression(node.parent) && node.parent.expression === node)) {
Expand Down Expand Up @@ -22194,7 +22196,7 @@ namespace ts {
// The declaration container is the innermost function that encloses the declaration of the variable
// 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;
const isParameterDecl = getRootDeclaration(declaration).kind === SyntaxKind.Parameter;
const declarationContainer = getControlFlowContainer(declaration);
let flowContainer = getControlFlowContainer(node);
const isOuterVariable = flowContainer !== declarationContainer;
Expand All @@ -22205,19 +22207,19 @@ namespace ts {
// analysis to include the immediately enclosing function.
while (flowContainer !== declarationContainer && (flowContainer.kind === SyntaxKind.FunctionExpression ||
flowContainer.kind === SyntaxKind.ArrowFunction || isObjectLiteralOrClassExpressionMethod(flowContainer)) &&
(isConstVariable(localOrExportSymbol) || isParameter && !isParameterAssigned(localOrExportSymbol))) {
(isConstVariable(localOrExportSymbol) || isParameterDecl && !isParameterAssigned(localOrExportSymbol))) {
flowContainer = getControlFlowContainer(flowContainer);
}
// We only look for uninitialized variables in strict null checking mode, and only when we can analyze
// the entire control flow graph from the variable's declaration (i.e. when the flow container and
// declaration container are the same).
const assumeInitialized = isParameter || isAlias || isOuterVariable || isSpreadDestructuringAssignmentTarget || isModuleExports || isBindingElement(declaration) ||
const assumeInitialized = isParameterDecl || isAlias || isOuterVariable || isSpreadDestructuringAssignmentTarget || isModuleExports || isBindingElement(declaration) ||
type !== autoType && type !== autoArrayType && (!strictNullChecks || (type.flags & (TypeFlags.AnyOrUnknown | TypeFlags.Void)) !== 0 ||
isInTypeQuery(node) || node.parent.kind === SyntaxKind.ExportSpecifier) ||
node.parent.kind === SyntaxKind.NonNullExpression ||
declaration.kind === SyntaxKind.VariableDeclaration && (<VariableDeclaration>declaration).exclamationToken ||
declaration.flags & NodeFlags.Ambient;
const initialType = assumeInitialized ? (isParameter ? removeOptionalityFromDeclaredType(type, declaration as VariableLikeDeclaration) : type) :
const initialType = assumeInitialized ? (isParameterDecl ? removeOptionalityFromDeclaredType(type, declaration as VariableLikeDeclaration) : type) :
type === autoType || type === autoArrayType ? undefinedType :
getOptionalType(type);
const flowType = getFlowTypeOfReference(node, type, initialType, flowContainer, !assumeInitialized);
Expand All @@ -22241,6 +22243,36 @@ namespace ts {
return assignmentKind ? getBaseTypeOfLiteralType(flowType) : flowType;
}

function checkIdentifierForRestParameter(node: Identifier, symbol: Symbol) {
if ((symbol.flags & SymbolFlags.Value) && symbol.valueDeclaration && isParameter(symbol.valueDeclaration) &&
(symbol.valueDeclaration !== node.parent || isParameterPropertyDeclaration(symbol.valueDeclaration, symbol.valueDeclaration.parent)) &&
isRestParameter(symbol.valueDeclaration) && !(symbol.valueDeclaration.flags & NodeFlags.RestParameterMustEmitAtTop)) {
const containerFunctionLikeDeclaration = findAncestor(symbol.valueDeclaration, isFunctionLikeDeclaration);
if (containerFunctionLikeDeclaration) {
const mutableValueDeclaration = <Mutable<Node>>symbol.valueDeclaration;
if (isParameterPropertyDeclaration(symbol.valueDeclaration, symbol.valueDeclaration.parent)) {
mutableValueDeclaration.flags |= NodeFlags.RestParameterMustEmitAtTop;
}
else {
let insideOtherFunctionLikeScope = false;
const topLevelStatementInContainer = findAncestor(node, n => {
if (isFunctionLikeDeclaration(n)) {
insideOtherFunctionLikeScope = true;
return "quit";
}
return n.parent && (isBlock(n.parent) ? n.parent.parent === containerFunctionLikeDeclaration : n.parent === containerFunctionLikeDeclaration);
});
if (insideOtherFunctionLikeScope) {
mutableValueDeclaration.flags |= NodeFlags.RestParameterMustEmitAtTop;
}
else if(topLevelStatementInContainer) {
(<Mutable<Node>>topLevelStatementInContainer).flags |= NodeFlags.ContainsRestParameterReference;
}
}
}
}
}

function isInsideFunction(node: Node, threshold: Node): boolean {
return !!findAncestor(node, n => n === threshold ? "quit" : isFunctionLike(n));
}
Expand Down Expand Up @@ -30304,6 +30336,9 @@ namespace ts {
checkVariableLikeDeclaration(node);
const func = getContainingFunction(node)!;
if (hasSyntacticModifier(node, ModifierFlags.ParameterPropertyModifier)) {
if (isIdentifier(node.name)) {
checkIdentifierForRestParameter(node.name, getResolvedSymbol(node.name));
}
if (!(func.kind === SyntaxKind.Constructor && nodeIsPresent(func.body))) {
error(node, Diagnostics.A_parameter_property_is_only_allowed_in_a_constructor_implementation);
}
Expand Down
38 changes: 29 additions & 9 deletions src/compiler/transformers/es2015.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -976,17 +976,18 @@ namespace ts {
let statementOffset = 0;
if (!hasSynthesizedSuper) statementOffset = factory.copyStandardPrologue(constructor.body.statements, prologue, /*ensureUseStrict*/ false);
addDefaultValueAssignmentsIfNeeded(statements, constructor);
addRestParameterIfNeeded(statements, constructor, hasSynthesizedSuper);
if (!hasSynthesizedSuper) statementOffset = factory.copyCustomPrologue(constructor.body.statements, statements, statementOffset, visitor);

// If the first statement is a call to `super()`, visit the statement directly
let superCallExpression: Expression | undefined;
let superCallStatement: Statement | undefined;
if (hasSynthesizedSuper) {
superCallExpression = createDefaultSuperCallOrThis();
}
else if (isDerivedClass && statementOffset < constructor.body.statements.length) {
const firstStatement = constructor.body.statements[statementOffset];
if (isExpressionStatement(firstStatement) && isSuperCall(firstStatement.expression)) {
superCallStatement = firstStatement;
superCallExpression = visitImmediateSuperCallInBody(firstStatement.expression);
}
}
Expand All @@ -1003,7 +1004,7 @@ namespace ts {
insertCaptureNewTargetIfNeeded(prologue, constructor, /*copyOnWrite*/ false);

if (isDerivedClass) {
if (superCallExpression && statementOffset === constructor.body.statements.length && !(constructor.body.transformFlags & TransformFlags.ContainsLexicalThis)) {
if (superCallExpression && superCallStatement && statementOffset === constructor.body.statements.length && !(constructor.body.transformFlags & TransformFlags.ContainsLexicalThis)) {
// If the subclass constructor does *not* contain `this` and *ends* with a `super()` call, we will use the
// following representation:
//
Expand All @@ -1025,6 +1026,7 @@ namespace ts {
// ```
const superCall = cast(cast(superCallExpression, isBinaryExpression).left, isCallExpression);
const returnStatement = factory.createReturnStatement(superCallExpression);
setOriginalNode(returnStatement, superCallStatement);
setCommentRange(returnStatement, getCommentRange(superCall));
setEmitFlags(superCall, EmitFlags.NoComments);
statements.push(returnStatement);
Expand Down Expand Up @@ -1054,8 +1056,7 @@ namespace ts {

// Since the `super()` call was the first statement, we insert the `this` capturing call to
// `super()` at the top of the list of `statements` (after any pre-existing custom prologues).
insertCaptureThisForNode(statements, constructor, superCallExpression || createActualThis());

insertCaptureThisForNode(statements, constructor, superCallExpression || createActualThis(), superCallStatement);
if (!isSufficientlyCoveredByReturnStatements(constructor.body)) {
statements.push(factory.createReturnStatement(factory.createUniqueName("_this", GeneratedIdentifierFlags.Optimistic | GeneratedIdentifierFlags.FileLevel)));
}
Expand All @@ -1079,6 +1080,7 @@ namespace ts {
insertCaptureThisForNodeIfNeeded(prologue, constructor);
}

addRestParameterIfNeeded(statements, constructor, hasSynthesizedSuper);
const block = factory.createBlock(
setTextRange(
factory.createNodeArray(
Expand Down Expand Up @@ -1354,6 +1356,11 @@ namespace ts {
return false;
}

const firstStatementContainsRestParameter = find(statements, stmt => !!(getOriginalNode(stmt).flags & NodeFlags.ContainsRestParameterReference));
if (!firstStatementContainsRestParameter && !((getOriginalNode(parameter).flags & NodeFlags.RestParameterMustEmitAtTop))) {
return false;
}

// `declarationName` is the name of the local declaration for the parameter.
// TODO(rbuckton): Does this need to be parented?
const declarationName = parameter.name.kind === SyntaxKind.Identifier ? setParent(setTextRange(factory.cloneNode(parameter.name), parameter.name), parameter.name.parent) : factory.createTempVariable(/*recordTempVariable*/ undefined);
Expand Down Expand Up @@ -1445,7 +1452,14 @@ namespace ts {
);
}

insertStatementsAfterCustomPrologue(statements, prologueStatements);
if (!firstStatementContainsRestParameter || (getOriginalNode(parameter).flags & NodeFlags.RestParameterMustEmitAtTop) || !node.body || isExpression(node.body)) {
insertStatementsAfterCustomPrologue(statements, prologueStatements);
}
else {
const idx = statements.indexOf(firstStatementContainsRestParameter);
Debug.assert(idx > -1);
statements.splice(idx, 0, ...prologueStatements);
}
return true;
}

Expand All @@ -1458,13 +1472,13 @@ namespace ts {
*/
function insertCaptureThisForNodeIfNeeded(statements: Statement[], node: Node): boolean {
if (hierarchyFacts & HierarchyFacts.CapturedLexicalThis && node.kind !== SyntaxKind.ArrowFunction) {
insertCaptureThisForNode(statements, node, factory.createThis());
insertCaptureThisForNode(statements, node, factory.createThis(), /* originalNode */ undefined);
return true;
}
return false;
}

function insertCaptureThisForNode(statements: Statement[], node: Node, initializer: Expression | undefined): void {
function insertCaptureThisForNode(statements: Statement[], node: Node, initializer: Expression | undefined, originalNode: Node | undefined): void {
enableSubstitutionsForCapturedThis();
const captureThisStatement = factory.createVariableStatement(
/*modifiers*/ undefined,
Expand All @@ -1478,6 +1492,7 @@ namespace ts {
])
);
setEmitFlags(captureThisStatement, EmitFlags.NoComments | EmitFlags.CustomPrologue);
setOriginalNode(captureThisStatement, originalNode);
setSourceMapRange(captureThisStatement, node);
insertStatementAfterCustomPrologue(statements, captureThisStatement);
}
Expand Down Expand Up @@ -1881,7 +1896,6 @@ namespace ts {
}

multiLine = addDefaultValueAssignmentsIfNeeded(statements, node) || multiLine;
multiLine = addRestParameterIfNeeded(statements, node, /*inConstructorWithSynthesizedSuper*/ false) || multiLine;

if (isBlock(body)) {
// addCustomPrologue puts already-existing directives at the beginning of the target statement-array
Expand Down Expand Up @@ -1916,6 +1930,7 @@ namespace ts {

const expression = visitNode(body, visitor, isExpression);
const returnStatement = factory.createReturnStatement(expression);
setOriginalNode(returnStatement, expression);
setTextRange(returnStatement, body);
moveSyntheticComments(returnStatement, body);
setEmitFlags(returnStatement, EmitFlags.NoTokenSourceMaps | EmitFlags.NoTrailingSourceMap | EmitFlags.NoTrailingComments);
Expand All @@ -1926,6 +1941,8 @@ namespace ts {
closeBraceLocation = body;
}

multiLine = addRestParameterIfNeeded(statements, node, /*inConstructorWithSynthesizedSuper*/ false) || multiLine;

factory.mergeLexicalEnvironment(prologue, endLexicalEnvironment());
insertCaptureNewTargetIfNeeded(prologue, node, /*copyOnWrite*/ false);
insertCaptureThisForNodeIfNeeded(prologue, node);
Expand Down Expand Up @@ -2466,6 +2483,7 @@ namespace ts {
),
/*location*/ node
);
setOriginalNode(forStatement, node);

// Disable trailing source maps for the OpenParenToken to align source map emit with the old emitter.
setEmitFlags(forStatement, EmitFlags.NoTokenTrailingSourceMaps);
Expand Down Expand Up @@ -2517,7 +2535,7 @@ namespace ts {
EmitFlags.NoTokenTrailingSourceMaps
);

return factory.createTryStatement(
const stmt = factory.createTryStatement(
factory.createBlock([
factory.restoreEnclosingLabel(
forStatement,
Expand Down Expand Up @@ -2582,6 +2600,8 @@ namespace ts {
)
])
);
setOriginalNode(stmt, node);
return stmt;
}

/**
Expand Down
22 changes: 13 additions & 9 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -746,15 +746,19 @@ namespace ts {
// removal, it is likely that users will add the import anyway.
// The advantage of this approach is its simplicity. For the case of batch compilation,
// we guarantee that users won't have to pay the price of walking the tree if a dynamic import isn't used.
/* @internal */ PossiblyContainsDynamicImport = 1 << 20,
/* @internal */ PossiblyContainsImportMeta = 1 << 21,

JSDoc = 1 << 22, // If node was parsed inside jsdoc
/* @internal */ Ambient = 1 << 23, // If node was inside an ambient context -- a declaration file, or inside something with the `declare` modifier.
/* @internal */ InWithStatement = 1 << 24, // If any ancestor of node was the `statement` of a WithStatement (not the `expression`)
JsonFile = 1 << 25, // If node was parsed in a Json
/* @internal */ TypeCached = 1 << 26, // If a type was cached for node at any point
/* @internal */ Deprecated = 1 << 27, // If has '@deprecated' JSDoc tag
/* @internal */ PossiblyContainsDynamicImport = 1 << 20,
/* @internal */ PossiblyContainsImportMeta = 1 << 21,

JSDoc = 1 << 22, // If node was parsed inside jsdoc
/* @internal */ Ambient = 1 << 23, // If node was inside an ambient context -- a declaration file, or inside something with the `declare` modifier.
/* @internal */ InWithStatement = 1 << 24, // If any ancestor of node was the `statement` of a WithStatement (not the `expression`)
JsonFile = 1 << 25, // If node was parsed in a Json
/* @internal */ TypeCached = 1 << 26, // If a type was cached for node at any point
/* @internal */ Deprecated = 1 << 27, // If has '@deprecated' JSDoc tag

/* @internal */ ContainsRestParameterReference = 1 << 28, // If contains rest parameter reference

/* @internal */ RestParameterMustEmitAtTop = 1 << 29, // If rest parameter must emit at top of function

BlockScoped = Let | Const,

Expand Down
14 changes: 2 additions & 12 deletions tests/baselines/reference/accessorWithRestParam.js
View file
Open in desktop
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 () { },
enumerable: false,
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: false,
configurable: true
});
Expand Down
7 changes: 1 addition & 6 deletions tests/baselines/reference/arrayLiteralInNonVarArgParameter.js
View file
Open in desktop
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');
Loading