Skip to content

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

Merged
yuit merged 11 commits into
release-1.8from
checksuperbeforethislexically
Feb 11, 2016
Merged

Check using "super" before "this" lexically#6860
yuit merged 11 commits into
release-1.8from
checksuperbeforethislexically

Conversation

@yuit

@yuit yuit commented Feb 3, 2016

Copy link
Copy Markdown
Contributor

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
Comment thread src/compiler/checker.ts Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

getSuperCallInConstructor

@yuit

yuit commented Feb 8, 2016

Copy link
Copy Markdown
Contributor Author

@DanielRosenwasser @vladima Any more comments?

Comment thread src/compiler/checker.ts Outdated
}
}

function isSuperCallExpression(n: Node): boolean {

Copy link
Copy Markdown
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.

Comment thread src/compiler/checker.ts Outdated
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
Copy Markdown
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

mhegazy commented Feb 11, 2016

Copy link
Copy Markdown
Contributor

👍

@mhegazy

mhegazy commented Feb 11, 2016

Copy link
Copy Markdown
Contributor

Please port this to master as well.

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