Skip to content
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

Check using "super" before "this" lexically #6860

Merged
merged 11 commits into from
Feb 11, 2016

Conversation

yuit
Copy link
Contributor

@yuit yuit commented Feb 3, 2016

With previous approach @ahejlsberg pointed out, it fails in the following case

class Base {
       constructor(c) { }
 }
class D extends Base {
       private _t;
       constructor() {
            super(i);
            var s = {
               t: this._t  // incorrect reported an error
           } 
           var i = Factory.create(s);  // if this is changed to var  i = undefined; error no longer reported
       }
}

This behavior is due to the fact that when the compiler is in the middle of checking super, it will then go and try to resolve i which then try to go resolve s which cause checkThisExpression to be called before the NodeCheckFlags.HasSeenSuperCall is set.

@ahejlsberg suggests a new approach to check if super is called before this lexically. This approached will be more robust against type-checker diving down when type-checking particular statement.

The fix also includes not-erroring when extends null as pointed out by @rbuckton

Kanchalai Tanglertsampan added 3 commits February 2, 2016 16:31
add baselines

Update baseline
NodeCheckFlags

Remove "type-checking" way of checking if super is used before this.
Instead check using whether super occurs before this syntactically

Refactor the code

Dive down to get super call
*
* @param constructor constructor-function to look for super statement
*/
function getSuperStatementInConstructor(constructor: ConstructorDeclaration): ExpressionStatement {
Copy link
Member

Choose a reason for hiding this comment

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

getSuperCallInConstructor

@yuit
Copy link
Contributor Author

yuit commented Feb 8, 2016

@DanielRosenwasser @vladima Any more comments?

@@ -7306,6 +7306,50 @@ namespace ts {
}
}

function isSuperCallExpression(n: Node): boolean {
Copy link
Member

Choose a reason for hiding this comment

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

This function already exists elsewhere, so you can remove this one.

if (containsSuperCall(node.body)) {
if (baseConstructorType === nullType) {
if (getSuperCallInConstructor(node)) {
if (classExtendsNull) {
error(node, Diagnostics.A_constructor_cannot_contain_a_super_call_when_its_class_extends_null);
Copy link
Member

Choose a reason for hiding this comment

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

Currently you're erroring on the constructor node itself, which gives a pretty unpleasant experience (it errors on the entire body as well).

You should report an error on the super call that you were able to find.

@mhegazy
Copy link
Contributor

mhegazy commented Feb 11, 2016

👍

@mhegazy
Copy link
Contributor

mhegazy commented Feb 11, 2016

Please port this to master as well.

yuit added a commit that referenced this pull request Feb 11, 2016
Check using "super" before "this" lexically
@yuit yuit merged commit 2d4bc0c into release-1.8 Feb 11, 2016
yuit pushed a commit that referenced this pull request Feb 11, 2016
Update baselines

add baselines

Update baseline

Port PR #6860 lexically check calling super before this
Check using "super" before "this" lexically instead of using the
NodeCheckFlags

Remove "type-checking" way of checking if super is used before this.
Instead check using whether super occurs before this syntactically

Refactor the code

Dive down to get super call

Address PR

Address PR about tests

Add a flag so we don't repeatedly finding super call

rename function

Move tests into correct location

Address PR: report error on super call instead of entire constructor node

remove marge mark
@RyanCavanaugh RyanCavanaugh deleted the checksuperbeforethislexically branch February 12, 2016 18:14
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 61e954b on checksuperbeforethislexically into ** on release-1.8**.

@RyanCavanaugh
Copy link
Member

@coveralls why are you here?

@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants