Skip to content

Dangerous Property Initializers in Classes #5987

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
kitsonk opened this issue Dec 8, 2015 · 9 comments · Fixed by #29395
Closed

Dangerous Property Initializers in Classes #5987

kitsonk opened this issue Dec 8, 2015 · 9 comments · Fixed by #29395
Labels
Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Help Wanted You can do this Suggestion An idea for TypeScript
Milestone

Comments

@kitsonk
Copy link
Contributor

kitsonk commented Dec 8, 2015

This is valid, but seem very dangerous:

class One {
    two() {};
}

class Foo {
    two = this.one.two();
    one = new One();
}

The emit goes in order of declaration which will cause a runtime error:

var One = (function () {
    function One() {
    }
    One.prototype.two = function () { };
    ;
    return One;
})();
var Foo = (function () {
    function Foo() {
        this.two = this.one.two();
        this.one = new One();
    }
    return Foo;
})();
@kitsonk
Copy link
Contributor Author

kitsonk commented Dec 30, 2015

I suspect this got lost in the pile. Any thoughts, or is it just and edge case that isn't worth bothering with?

@DanielRosenwasser DanielRosenwasser added the Suggestion An idea for TypeScript label Dec 30, 2015
@DanielRosenwasser
Copy link
Member

Sorry about that, this past month's been busy and naturally some people are out.

I wonder if this should fall under the umbrella of #2234.

@DanielRosenwasser DanielRosenwasser changed the title Dangerous Parameter Initializers in Classes Dangerous Property Initializers in Classes Dec 30, 2015
@kitsonk
Copy link
Contributor Author

kitsonk commented Dec 30, 2015

Clearly it is related to #2234, but I would suspect a lot of users would be "surprised" that they have to think about the ordering of a property initializers in a class, although I just checked the ES Stage 1 proposal for class properties and initializers and it appears that this would break there too, so the emit is valid. In that case, stupid is as stupid does and it would logically fall in the realm of use before initialization.

cc/@adidahiya

@RyanCavanaugh RyanCavanaugh added the Help Wanted You can do this label Jan 1, 2016
@RyanCavanaugh
Copy link
Member

"Order of emit is order of declaration" is a must-have behavior for ES6 compat ES7+ compat as that would almost certainly have the same semantics.

I don't see why we wouldn't error here, though. We can treat these as if they were let declarations in terms of TDZ rules.

@DanielRosenwasser DanielRosenwasser added the Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". label Jan 4, 2016
@mhegazy mhegazy added this to the Community milestone Jan 4, 2018
@theseyi
Copy link

theseyi commented Mar 15, 2018

Is this a similar issue to the ffg being allowed in TS?

class Test {
  x:any = this.x;
}

I can't find any documentation that says you can reference this in a class field / property but TS allows this to compile and emits js, but looking at it seems odd.

emit:

var Test = /** @class */ (function () {
    function Test() {
        this.x = this.x;
    }
    return Test;
}());

http://www.typescriptlang.org/play/index.html#src=class%20Test%20%7B%0A%20%20x%3Aany%20%3D%20this.x%3B%0A%7D

@theseyi
Copy link

theseyi commented Mar 16, 2018

@kitsonk Thoughts?

@kitsonk
Copy link
Contributor Author

kitsonk commented Mar 22, 2018

I don't know if it is similar. It is valid, though illogical. My opinion would be that it should be an error as well (sort of the same class as use before assigned).

@JoshuaKGoldberg
Copy link
Contributor

The original code now correctly gives a type checking error! ✨

class One {
    two() {};
}

class Foo {
    two = this.one.two();
               ~~~ // Property 'one' is used before its initialization.ts(2729)
    one = new One();
}

However, x: any = this.x; is still allowed but shouldn't be.

JoshuaKGoldberg pushed a commit to JoshuaKGoldberg/TypeScript that referenced this issue Jan 13, 2019
Fixes microsoft#5987.

Usages of a class property in a preceding property already gave an error, but the following doesn't yet:

```ts
class Test {
    x: number = this.x;
}
```

As with other use-before-declare checking, IIFEs are not treated as invalid uses.
@RyanCavanaugh RyanCavanaugh modified the milestones: Community, Backlog Mar 7, 2019
sandersn added a commit that referenced this issue Apr 5, 2019
…29395)

* Added error for class properties used within their own declaration

Fixes #5987.

Usages of a class property in a preceding property already gave an error, but the following doesn't yet:

```ts
class Test {
    x: number = this.x;
}
```

As with other use-before-declare checking, IIFEs are not treated as invalid uses.

* Accepted 'witness' baselines; removed unnecessary !==

* Addressed quick feedback items

* Accepted odd new baseline

* Fixed post-merge introduced lint errors

* Updated baselines again
@chriskrycho
Copy link

Hey folks, the Ember TS crowd just noted an issue with the PR that closed this; see my comment here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants