Skip to content

Commit

Permalink
Allowed non-this, non-super code before super call in derived classes…
Browse files Browse the repository at this point in the history 
… with property initializers (#29374)

* Allowed non-this, non-super code before super call in derived classes

Fixes #8277.

It feels wrong to put a new `forEachChild` loop in the checker, though in the vast majority of user files this will be a very quick one. Is there a better way to check for a reference to `super` or `this`?

* Used new isNewThisScope utility and dummy prop in baselines

* Accounted for parameters and get/set accessor bodies

* Accounted for class extensions

* (node|statement)RefersToSuperOrThis wasn't checking root level scope boundaries

```ts
function () {
    return this;
}
```

It was immediately going to `ts.forEachChild` so the statement itself wasn't being counted as a new `this` scope.

* Better 'references' name and comments; accounted for more method edge case

* Limited super calls to root-level statements in constructor bodies

As per discussion in the issue, it would be ideal to consider any block that always ends up calling to super() the equivalent of a root-level super() statement. This would be valid:

```ts
foo = 1;
constructor() {
    condition() ? super(1) : super(0);
    this.foo;
}
```

...as it would compile to the equivalent of:
```ts
function () {
    condition() ? super(1) : super(0);
    this.foo = 1;
    this.foo;
}

That change would a bit more intense and I'm very timid, so leaving it out of this PR. In the meantime the requirement is that the super() statement must itself be root-level.

* Fixed error number following 'master' merge

* Added decorator test cases

* Again allowed arrow functions

* Accepted new baselines

* Added allowance for (super())

* Reworked emitter transforms for ES this binding semantics

In trying to adjust to rbuckton's PR feedback, this orders derived class constructor bodies into five sections:

1. Pre-`super()` code
2. The `super()` call itself
3. Class properties with initializers
4. Parameter properties
5. The rest of the constructor

I've looked through the updated baselines and it looks like they're generally better than before. Within the existing test cases that result in semantic errors for `this` access before `super`, several previously resulted in a global `_this` being created; now, they correctly defer referring to `_this` until it's assigned to `_super.call(this) || this`.

* Used skipOuterExpressions when diving for super()s; fix prop ordering

* Allow direct var _this = super() when no pre-super() statements; lint fixes

* Always with the TSLint

* One last touchup: skipOuterExpressions in es2015 transformer

* Fixed new lint complaint in utilities.ts

* Again added a falls-through; it'd be swell if I could run linting locally

* This time I think I got it

* Well at least the error is a different one

* Undid irrelevant whitespace changes

* Mostly addressed private/field issues

* Accepted derivedClassSuperProperties baseline

* Lint fix, lovely

* Remove now-unnecesary comment

* First round of feedback

* Moved prologue statements to start of statements

* Added consideration for super statements in loops and the like

* Ordering and a _this_1 test

* Missed the one change I needed...

* First round of feedback corrections

* Feedback round two: statements

* Feedback: used more direct statements

* Fixed classFields emit to not duplicate temp variables

* Refactored es2015 helper to be less overloaded

* Accounted for parentheses

* Simpler feedback: -1, and emptyArray

* Next feedback: superStatementIndex

* Feedback: simplified to no longer create slice arrays

* Adjusted for default and rest parameters

* Added test case for commas

* Corrected comment ranges

* Handled comments after super, with tests

* Fixed Bad/Late super baselines

* Remove unused param and unnecessary baseline comments

Co-authored-by: Orta Therox <orta.therox@gmail.com>
Co-authored-by: Ron Buckton <ron.buckton@microsoft.com>
  • Loading branch information
@orta @rbuckton
3 people committed Jan 14, 2022
1 parent 70097c4 commit b7fee7f
Show file tree
Hide file tree
Showing 74 changed files with 5,561 additions and 317 deletions.
59 changes: 42 additions & 17 deletions src/compiler/checker.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34731,34 +34731,42 @@ namespace ts {
error(superCall, Diagnostics.A_constructor_cannot_contain_a_super_call_when_its_class_extends_null);
}

// The first statement in the body of a constructor (excluding prologue directives) must be a super call
// if both of the following are true:
// A super call must be root-level in a constructor if both of the following are true:
// - The containing class is a derived class.
// - The constructor declares parameter properties
// or the containing class declares instance member variables with initializers.
const superCallShouldBeFirst =

const superCallShouldBeRootLevel =
(getEmitScriptTarget(compilerOptions) !== ScriptTarget.ESNext || !useDefineForClassFields) &&
(some((node.parent as ClassDeclaration).members, isInstancePropertyWithInitializerOrPrivateIdentifierProperty) ||
some(node.parameters, p => hasSyntacticModifier(p, ModifierFlags.ParameterPropertyModifier)));

// Skip past any prologue directives to find the first statement
// to ensure that it was a super call.
if (superCallShouldBeFirst) {
const statements = node.body!.statements;
let superCallStatement: ExpressionStatement | undefined;
if (superCallShouldBeRootLevel) {
// Until we have better flow analysis, it is an error to place the super call within any kind of block or conditional
// See GH #8277
if (!superCallIsRootLevelInConstructor(superCall, node.body!)) {
error(superCall, Diagnostics.A_super_call_must_be_a_root_level_statement_within_a_constructor_of_a_derived_class_that_contains_initialized_properties_parameter_properties_or_private_identifiers);
}
// Skip past any prologue directives to check statements for referring to 'super' or 'this' before a super call
else {
let superCallStatement: ExpressionStatement | undefined;

for (const statement of statements) {
if (statement.kind === SyntaxKind.ExpressionStatement && isSuperCall((statement as ExpressionStatement).expression)) {
superCallStatement = statement as ExpressionStatement;
break;
for (const statement of node.body!.statements) {
if (isExpressionStatement(statement) && isSuperCall(skipOuterExpressions(statement.expression))) {
superCallStatement = statement;
break;
}
if (!isPrologueDirective(statement) && nodeImmediatelyReferencesSuperOrThis(statement)) {
break;
}
}
if (!isPrologueDirective(statement)) {
break;

// Until we have better flow analysis, it is an error to place the super call within any kind of block or conditional
// See GH #8277
if (superCallStatement === undefined) {
error(node, Diagnostics.A_super_call_must_be_the_first_statement_in_the_constructor_to_refer_to_super_or_this_when_a_derived_class_contains_initialized_properties_parameter_properties_or_private_identifiers);
}
}
if (!superCallStatement) {
error(node, Diagnostics.A_super_call_must_be_the_first_statement_in_the_constructor_when_a_class_contains_initialized_properties_parameter_properties_or_private_identifiers);
}
}
}
else if (!classExtendsNull) {
Expand All @@ -34767,6 +34775,23 @@ namespace ts {
}
}

function superCallIsRootLevelInConstructor(superCall: Node, body: Block) {
const superCallParent = walkUpParenthesizedExpressions(superCall.parent);
return isExpressionStatement(superCallParent) && superCallParent.parent === body;
}

function nodeImmediatelyReferencesSuperOrThis(node: Node): boolean {
if (node.kind === SyntaxKind.SuperKeyword || node.kind === SyntaxKind.ThisKeyword) {
return true;
}

if (isThisContainerOrFunctionBlock(node)) {
return false;
}

return !!forEachChild(node, nodeImmediatelyReferencesSuperOrThis);
}

function checkAccessorDeclaration(node: AccessorDeclaration) {
if (produceDiagnostics) {
// Grammar checking accessors
Expand Down
6 changes: 5 additions & 1 deletion src/compiler/diagnosticMessages.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1747,7 +1747,7 @@
"category": "Error",
"code": 2375
},
"A 'super' call must be the first statement in the constructor when a class contains initialized properties, parameter properties, or private identifiers.": {
"A 'super' call must be the first statement in the constructor to refer to 'super' or 'this' when a derived class contains initialized properties, parameter properties, or private identifiers.": {
"category": "Error",
"code": 2376
},
Expand Down Expand Up @@ -1839,6 +1839,10 @@
"category": "Error",
"code": 2400
},
"A 'super' call must be a root-level statement within a constructor of a derived class that contains initialized properties, parameter properties, or private identifiers.": {
"category": "Error",
"code": 2401
},
"Expression resolves to '_super' that compiler uses to capture base class reference.": {
"category": "Error",
"code": 2402
Expand Down
7 changes: 4 additions & 3 deletions src/compiler/factory/nodeFactory.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5885,7 +5885,7 @@ namespace ts {
* @param visitor Optional callback used to visit any custom prologue directives.
*/
function copyPrologue(source: readonly Statement[], target: Push<Statement>, ensureUseStrict?: boolean, visitor?: (node: Node) => VisitResult<Node>): number {
const offset = copyStandardPrologue(source, target, ensureUseStrict);
const offset = copyStandardPrologue(source, target, 0, ensureUseStrict);
return copyCustomPrologue(source, target, offset, visitor);
}

Expand All @@ -5901,12 +5901,13 @@ namespace ts {
* Copies only the standard (string-expression) prologue-directives into the target statement-array.
* @param source origin statements array
* @param target result statements array
* @param statementOffset The offset at which to begin the copy.
* @param ensureUseStrict boolean determining whether the function need to add prologue-directives
* @returns Count of how many directive statements were copied.
*/
function copyStandardPrologue(source: readonly Statement[], target: Push<Statement>, ensureUseStrict?: boolean): number {
function copyStandardPrologue(source: readonly Statement[], target: Push<Statement>, statementOffset = 0, ensureUseStrict?: boolean): number {
Debug.assert(target.length === 0, "Prologue directives should be at the first statement in the target statements array");
let foundUseStrict = false;
let statementOffset = 0;
const numStatements = source.length;
while (statementOffset < numStatements) {
const statement = source[statementOffset];
Expand Down
79 changes: 65 additions & 14 deletions src/compiler/transformers/classFields.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1249,10 +1249,28 @@ namespace ts {

resumeLexicalEnvironment();

let indexOfFirstStatement = 0;
const needsSyntheticConstructor = !constructor && isDerivedClass;
let indexOfFirstStatementAfterSuper = 0;
let prologueStatementCount = 0;
let superStatementIndex = -1;
let statements: Statement[] = [];

if (!constructor && isDerivedClass) {
if (constructor?.body?.statements) {
prologueStatementCount = factory.copyPrologue(constructor.body.statements, statements, /*ensureUseStrict*/ false, visitor);
superStatementIndex = findSuperStatementIndex(constructor.body.statements, prologueStatementCount);

// If there was a super call, visit existing statements up to and including it
if (superStatementIndex >= 0) {
indexOfFirstStatementAfterSuper = superStatementIndex + 1;
statements = [
...statements.slice(0, prologueStatementCount),
...visitNodes(constructor.body.statements, visitor, isStatement, prologueStatementCount, indexOfFirstStatementAfterSuper - prologueStatementCount),
...statements.slice(prologueStatementCount),
];
}
}

if (needsSyntheticConstructor) {
// Add a synthetic `super` call:
//
// super(...arguments);
Expand All @@ -1268,9 +1286,6 @@ namespace ts {
);
}

if (constructor) {
indexOfFirstStatement = addPrologueDirectivesAndInitialSuperCall(factory, constructor, statements, visitor);
}
// Add the property initializers. Transforms this:
//
// public x = 1;
Expand All @@ -1281,26 +1296,52 @@ namespace ts {
// this.x = 1;
// }
//
// If we do useDefineForClassFields, they'll be converted elsewhere.
// We instead *remove* them from the transformed output at this stage.
let parameterPropertyDeclarationCount = 0;
if (constructor?.body) {
let afterParameterProperties = findIndex(constructor.body.statements, s => !isParameterPropertyDeclaration(getOriginalNode(s), constructor), indexOfFirstStatement);
if (afterParameterProperties === -1) {
afterParameterProperties = constructor.body.statements.length;
if (useDefineForClassFields) {
statements = statements.filter(statement => !isParameterPropertyDeclaration(getOriginalNode(statement), constructor));
}
if (afterParameterProperties > indexOfFirstStatement) {
if (!useDefineForClassFields) {
addRange(statements, visitNodes(constructor.body.statements, visitor, isStatement, indexOfFirstStatement, afterParameterProperties - indexOfFirstStatement));
else {
for (const statement of constructor.body.statements) {
if (isParameterPropertyDeclaration(getOriginalNode(statement), constructor)) {
parameterPropertyDeclarationCount++;
}
}
if (parameterPropertyDeclarationCount > 0) {
const parameterProperties = visitNodes(constructor.body.statements, visitor, isStatement, indexOfFirstStatementAfterSuper, parameterPropertyDeclarationCount);

// If there was a super() call found, add parameter properties immediately after it
if (superStatementIndex >= 0) {
addRange(statements, parameterProperties);
}
// If a synthetic super() call was added, add them just after it
else if (needsSyntheticConstructor) {
statements = [
statements[0],
...parameterProperties,
...statements.slice(1),
];
}
// Since there wasn't a super() call, add them to the top of the constructor
else {
statements = [...parameterProperties, ...statements];
}

indexOfFirstStatementAfterSuper += parameterPropertyDeclarationCount;
}
indexOfFirstStatement = afterParameterProperties;
}
}

const receiver = factory.createThis();
// private methods can be called in property initializers, they should execute first.
addMethodStatements(statements, privateMethodsAndAccessors, receiver);
addPropertyOrClassStaticBlockStatements(statements, properties, receiver);

// Add existing statements, skipping the initial super call.
// Add existing statements after the initial prologues and super call
if (constructor) {
addRange(statements, visitNodes(constructor.body!.statements, visitor, isStatement, indexOfFirstStatement));
addRange(statements, visitNodes(constructor.body!.statements, visitBodyStatement, isStatement, indexOfFirstStatementAfterSuper + prologueStatementCount));
}

statements = factory.mergeLexicalEnvironment(statements, endLexicalEnvironment());
Expand All @@ -1315,6 +1356,14 @@ namespace ts {
),
/*location*/ constructor ? constructor.body : undefined
);

function visitBodyStatement(statement: Node) {
if (useDefineForClassFields && isParameterPropertyDeclaration(getOriginalNode(statement), constructor!)) {
return undefined;
}

return visitor(statement);
}
}

/**
Expand All @@ -1335,11 +1384,13 @@ namespace ts {
setSourceMapRange(statement, moveRangePastModifiers(property));
setCommentRange(statement, property);
setOriginalNode(statement, property);

// `setOriginalNode` *copies* the `emitNode` from `property`, so now both
// `statement` and `expression` have a copy of the synthesized comments.
// Drop the comments from expression to avoid printing them twice.
setSyntheticLeadingComments(expression, undefined);
setSyntheticTrailingComments(expression, undefined);

statements.push(statement);
}
}
Expand Down

0 comments on commit b7fee7f

Please sign in to comment.