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

Wrong field value in inherited constructor's submethod and after #9455

Closed
mikolaj-milewski opened this Issue Jun 30, 2016 · 10 comments

Comments

Projects
None yet
3 participants
@mikolaj-milewski

mikolaj-milewski commented Jun 30, 2016

TypeScript Version: 1.8.10

Code

// A *self-contained* demonstration of the problem follows...
abstract class A {
    constructor() {
        this.onCreate();
    }

    abstract onCreate();
}

class B extends A {
    private field = 'foo';

    onCreate() {
        console.log(this.field);
        this.field = 'bar';
    }

    echo () {
        console.log(this.field);
    }
}

let b: B = new B();
b.echo();

Expected behavior:
Console prints out:

foo
bar

Actual behavior:
Console prints out:

undefined
foo
@mhegazy

This comment has been minimized.

Show comment
Hide comment
@mhegazy

mhegazy Jun 30, 2016

Contributor

Please see my comment in #9060 (comment)

Duplicate of #9060, #7724 and #9209

Contributor

mhegazy commented Jun 30, 2016

Please see my comment in #9060 (comment)

Duplicate of #9060, #7724 and #9209

@mhegazy mhegazy closed this Jun 30, 2016

@mhegazy mhegazy added the Duplicate label Jun 30, 2016

@mikolaj-milewski

This comment has been minimized.

Show comment
Hide comment
@mikolaj-milewski

mikolaj-milewski Jun 30, 2016

I agree that calling virtual methods from constructor of a base class is risky because specific order of calling constructors chain - but here I have an issue with field initializers. C# treats them well:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace ConsoleApplication1
{
    public abstract class A
    {
        public A()
        {
            onCreate();
        }

        protected abstract void onCreate();
    }

    class B : A
    {
        private string field = "foo";

        protected override void onCreate()
        {
            Console.Write(field);
            field = "bar";
        }

        public void echo()
        {
            Console.Write(field);
        }
    }

    class Program
    {
        static void Main(string[] args)
        {
            B b = new B();
            b.echo();
            Console.ReadKey();
        }
    }
}

C# code from above gives expected output: foobar. TypeScript treats field initializers differently - in generated JS code it just puts them to the constructor, between super() call and rest of constructor's code:

B = (function (_super) {
    __extends(B, _super);
    function B() {
        _super.apply(this, arguments);
        this.field = 'foo'; // <------------- field initializer is inserted here
    }
    B.prototype.onCreate = function () {
        console.log(this.field);
        this.field = 'bar';
    };
    B.prototype.echo = function () {
        console.log(this.field);
    };
    return B;
}(A));

Is that a problem just to reverse order in generated construtor - put field initializers before super() call? That resolves the problem I have and adds consistency with C# behavior to the language.

mikolaj-milewski commented Jun 30, 2016

I agree that calling virtual methods from constructor of a base class is risky because specific order of calling constructors chain - but here I have an issue with field initializers. C# treats them well:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace ConsoleApplication1
{
    public abstract class A
    {
        public A()
        {
            onCreate();
        }

        protected abstract void onCreate();
    }

    class B : A
    {
        private string field = "foo";

        protected override void onCreate()
        {
            Console.Write(field);
            field = "bar";
        }

        public void echo()
        {
            Console.Write(field);
        }
    }

    class Program
    {
        static void Main(string[] args)
        {
            B b = new B();
            b.echo();
            Console.ReadKey();
        }
    }
}

C# code from above gives expected output: foobar. TypeScript treats field initializers differently - in generated JS code it just puts them to the constructor, between super() call and rest of constructor's code:

B = (function (_super) {
    __extends(B, _super);
    function B() {
        _super.apply(this, arguments);
        this.field = 'foo'; // <------------- field initializer is inserted here
    }
    B.prototype.onCreate = function () {
        console.log(this.field);
        this.field = 'bar';
    };
    B.prototype.echo = function () {
        console.log(this.field);
    };
    return B;
}(A));

Is that a problem just to reverse order in generated construtor - put field initializers before super() call? That resolves the problem I have and adds consistency with C# behavior to the language.

@RyanCavanaugh

This comment has been minimized.

Show comment
Hide comment
@RyanCavanaugh

RyanCavanaugh Jun 30, 2016

Member

Putting field initializers before the super call is even worse because base class field initialization occurs during the super call. This results in derived class initializations getting overwritten by base class initializations. In other words:

class Base {
  myName = 'the default name';
}
class Derived extends Base {
  myName = 'derived name';
}
var x = new Derived();
console.log(x.myName); // should not print "the default name" !
Member

RyanCavanaugh commented Jun 30, 2016

Putting field initializers before the super call is even worse because base class field initialization occurs during the super call. This results in derived class initializations getting overwritten by base class initializations. In other words:

class Base {
  myName = 'the default name';
}
class Derived extends Base {
  myName = 'derived name';
}
var x = new Derived();
console.log(x.myName); // should not print "the default name" !
@RyanCavanaugh

This comment has been minimized.

Show comment
Hide comment
@RyanCavanaugh

RyanCavanaugh Jun 30, 2016

Member

See also #1617

Member

RyanCavanaugh commented Jun 30, 2016

See also #1617

@mikolaj-milewski

This comment has been minimized.

Show comment
Hide comment
@mikolaj-milewski

mikolaj-milewski Jun 30, 2016

Also agree, wrong idea. What about this one?

A = (function () {
    function A() {
        this.__initializers();
        this.onCreate();
    }
    A.prototype.__initializers = function () {
        // class A initializers here
    }
    return A;
}());
B = (function (_super) {
    __extends(B, _super);
    function B() {
        _super.apply(this, arguments);
    }
    B.prototype.__initializers = function () {
        _super.prototype.__initializers.call(this);
        // class B initializers here
        this.field = 'foo';
    }
    B.prototype.onCreate = function () {
        console.log(this.field);
        this.field = 'bar';
    };
    B.prototype.echo = function () {
        console.log(this.field);
    };
    return B;
}(A));

I've moved field initializers from constructor to the dedicated virtual method. It works well in my scenario, does not break yours and also fixes #1617.

mikolaj-milewski commented Jun 30, 2016

Also agree, wrong idea. What about this one?

A = (function () {
    function A() {
        this.__initializers();
        this.onCreate();
    }
    A.prototype.__initializers = function () {
        // class A initializers here
    }
    return A;
}());
B = (function (_super) {
    __extends(B, _super);
    function B() {
        _super.apply(this, arguments);
    }
    B.prototype.__initializers = function () {
        _super.prototype.__initializers.call(this);
        // class B initializers here
        this.field = 'foo';
    }
    B.prototype.onCreate = function () {
        console.log(this.field);
        this.field = 'bar';
    };
    B.prototype.echo = function () {
        console.log(this.field);
    };
    return B;
}(A));

I've moved field initializers from constructor to the dedicated virtual method. It works well in my scenario, does not break yours and also fixes #1617.

@RyanCavanaugh

This comment has been minimized.

Show comment
Hide comment
@RyanCavanaugh

RyanCavanaugh Jul 1, 2016

Member

Breaking change concerns aside, in general we don't know whether a given base class is a TypeScript-generated class or not, so we wouldn't know whether to put initializers in __initializers or the constructor body.

Member

RyanCavanaugh commented Jul 1, 2016

Breaking change concerns aside, in general we don't know whether a given base class is a TypeScript-generated class or not, so we wouldn't know whether to put initializers in __initializers or the constructor body.

@RyanCavanaugh

This comment has been minimized.

Show comment
Hide comment
@RyanCavanaugh

RyanCavanaugh Jul 1, 2016

Member

I should also add that the current initialization order is very likely to be the spec'd behavior of ES7+ class property initializers.

Member

RyanCavanaugh commented Jul 1, 2016

I should also add that the current initialization order is very likely to be the spec'd behavior of ES7+ class property initializers.

@mikolaj-milewski

This comment has been minimized.

Show comment
Hide comment
@mikolaj-milewski

mikolaj-milewski Jul 1, 2016

Well, with non-TS base classes it is indeed a problem - I haven't taken that into consideration. What do you think about compiler warning on reading fields in constructor and constructor's submethods? That would save much of a developer time in all mentioned scenarios, I think.

mikolaj-milewski commented Jul 1, 2016

Well, with non-TS base classes it is indeed a problem - I haven't taken that into consideration. What do you think about compiler warning on reading fields in constructor and constructor's submethods? That would save much of a developer time in all mentioned scenarios, I think.

@RyanCavanaugh

This comment has been minimized.

Show comment
Hide comment
@RyanCavanaugh

RyanCavanaugh Jul 1, 2016

Member

Literally you'd want a warning for this code?

class A {
  size = 10;
  constructor() {
    console.log('size = ' + this.size);
  }
}
Member

RyanCavanaugh commented Jul 1, 2016

Literally you'd want a warning for this code?

class A {
  size = 10;
  constructor() {
    console.log('size = ' + this.size);
  }
}
@mikolaj-milewski

This comment has been minimized.

Show comment
Hide comment
@mikolaj-milewski

mikolaj-milewski Jul 1, 2016

Yeah. This initialization order is specific to TypeScript, so warning would help beginers like me to get the very idea, without hitting the wall, searching forums, even starting discussions like this one.

mikolaj-milewski commented Jul 1, 2016

Yeah. This initialization order is specific to TypeScript, so warning would help beginers like me to get the very idea, without hitting the wall, searching forums, even starting discussions like this one.

@Microsoft Microsoft locked and limited conversation to collaborators Jun 19, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.