microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
101.11k stars 12.5k forks source link

Allow assignment of uninitialized readonly base class properties in subclass constructor chain #25134

Closed ahamid closed 6 years ago

ahamid commented 6 years ago

This is an issue I run into often and forces me to choose between unnatural and convoluted initialization, redundant or weakened property declarations, or superfluous errors/warning noise I have to remember to ignore.

abstract class Base {
    readonly locomation: string // derived in subclass-specific way
}

class Bird extends Base {
    constructor() {
        super()
        this.locomation = 'fly' // cannot assign to constant or read-only property
    }
}

class Fish extends Base {
    constructor() {
        super()
        this.locomation = 'swim' // cannot assign to constant or read-only property
    }
}

It seems to make sense to me to relax the constraint to allow one-time initialization of a readonly property somewhere in the constructor chain. Subclass constructors are the most immediately statically analyzable locations, but the more general case might be private methods that are known only to be invoked during construction:

class ComplexInitialization {
  readonly derived: string
  constructor() {
    ...
    this.initialize()
    ...
  }
  private initialize() {
    this.derived = ...
  }
}

or even other initialization methods if they are known only to be invoked during construction:

// ComplexInitialization: broaden initialize visibility to `protected`
class DelegatedInitialization extends ComplexInitialization {
  protected initialize() {
    // still only called once during construction
    this.derived = ...
  }
}

Understood that the latter cases are harder to safely identify, but the simple case would go a long way. Absence of this ability essentially forces all values destined for readonly properties (and dependent context necessary to calculate them) to be hoisted up through the constructor call chain, or for readonly to be abandoned entirely.

I don't see this particular issue being reported (discussion on #8496 seems tangentially related, although in this issue I'm not suggesting the ability to ever re-assign a readonly property, and AFIAU I'm not asking to break existing readonly guarantees - I actually can't find readonly in the spec).

As far as I understand I think this suggestion meets these guidelines:

mhegazy commented 6 years ago

This can easily be written as:

abstract class Base {
    constructor(readonly locomation: string) { }
}

class Bird extends Base {
    constructor() {
        super('fly');
    }
}

class Fish extends Base {
    constructor() {
        super('swim');
    }
}
ahamid commented 6 years ago

Yes, true. Imagine 10 constructor arguments of complex objects, with data dependencies between them, and now a class hierarchy which derives parts of that data in other different complicated ways. It becomes much more convoluted than simply passing a couple primitive args. (i can produce less trivial examples, but for the sake of brevity those were the distilled cases)

mhegazy commented 6 years ago

readonly means constructor-only-write/read-everywhere-else. if you want to change the definition of an existing language concept, you need to provide some compelling use cases, and explain why the existing support is not sufficient.

ahamid commented 6 years ago

It means constructor in class-of-definition-only, not a subclass constructor? I couldn't find any mention of readonly in any specs I could find, so I didn't know that this was explicitly the case. If this feature isn't compelling, so be it.

Here's a slightly more elaborate example:

class Rec {
    readonly r1: any
    readonly r2: any
    readonly r3: any
    readonly r4: any

    constructor(readonly a: any, readonly b: any) {
    }
}

class SpecializedRec extends Rec {
    readonly s1: any
    readonly s2: any
    constructor(opts) {
        super(opts.a, opts.b)
        this.s1 = opts.s1
        this.s2 = opts.s2
        this.r2 = this.s1.c + this.s2.d // err
        this.r4 = opts.x                // err
    }
}

class EvenMoreSpecialized extends SpecializedRec {
    constructor(readonly e1: any) {
        super({ a: 'a', b: 'b', s1: { c: 1 }, s2: { d: 2 }, x: e1 })
        this.r3 = 123 // err
    }
}

to resolve this it's necessary to restructure to pass values through the constructor call chain:

class Rec {
    readonly r1: any

    constructor(readonly a: any, readonly b: any, readonly r2: any, readonly r3: any, readonly r4: any) {
    }
}

class SpecializedRec extends Rec {
    readonly s1: any
    readonly s2: any
    constructor(opts, r3) {
        super(opts.a, opts.b, opts.s1.c + opts.s2.d, r3 /* only passed thru */, opts.x)
        this.s1 = opts.s1
        this.s2 = opts.s2
    }

    updateSThing(val: any) {
        this.s1.foo = val
    }
}

class EvenMoreSpecialized2 extends SpecializedRec {
    constructor(readonly e1: any) {
        super({ a: 'a', b: 'b', s1: { c: 1 }, s2: { d: 2 }, x: e1 }, 123)
    }
}

There are not that many fields in play here, the more fields and more dependencies between them, the more the constructor args get out of hand.

mhegazy commented 6 years ago

It means constructor in class-of-definition-only, not a subclass constructor

Correct.

ferdaber commented 6 years ago

We had a recent PR for the React type definitions that made state to be readonly (where any component deriving from the base React component should not be modifying state directly), but it caused issues with existing code bases that used to initialize state in the constructor. https://github.com/DefinitelyTyped/DefinitelyTyped/pull/26813#issuecomment-400795486

For cases like these, what do you suggest is the best way to work around the issue? We have so far been recommending that they move the state initialization outside of the constructor and into a class field, but then unless they explicitly decorate it with readonly it once again becomes mutable.

mhegazy commented 6 years ago

For cases like these, what do you suggest is the best way to work around the issue?

I would say the change to the react declaration file on definitelyTyped was wrong. if it is allowed by the runtime to change state, then the type definitions should prohibit it. the intent of a declaration file is to model the runtime behavior of a JS library, and not to enforce a coding style or best practices. these should be done in a linter.

typescript-bot commented 6 years ago

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

thw0rted commented 6 years ago

I stumbled into a "fix" for this behavior, but I'm not sure if it's abusing the language or making some kind of pitfall I'll wind up tripping over later. Basically, it's this:

class A {
  public readonly foo: boolean = true;
}

class B extends A {
  public readonly foo: boolean;
  constructor(x: number) {
    super();
    this.foo = x >= 0;
  }
}

If you redeclare the readonly field from the superclass in the subclass, both classes can write to it in their constructor (or class definition). This seems to work... is there a downside?

Hashbrown777 commented 5 years ago

@thw0rted, you can also just put // @ts-ignore before the this.foo = instead of re-declaring (which may have issues when foo changes to boolean | somethingElse in class A later on without you realising).

@mhegazy, I disagree that it's a coding style and that linters should be handling this case. It's not "allowed by the runtime to change state"; the constructors (including those of derived classes) are initialising the state, you still want to deny changes in a constructed object. If you wanted to explicitly deny this behaviour in your code, you would make it readonly private and then have a getter for the variable for the subclasses.

I feel typescript doesn't account for that and we're making workarounds for how we reasonably expect TS to behave.

mcshaz commented 5 years ago

Yes, true. Imagine 10 constructor arguments of complex objects, with data dependencies between them, and now a class hierarchy which derives parts of that data in other different complicated ways. It becomes much more convoluted than simply passing a couple primitive args. (i can produce less trivial examples, but for the sake of brevity those were the distilled cases)

I came across this issue as I was having the same paradigm shift (I am pretty sure either C++ &/or C# let child classes assign in the constructor)

Thinking about working with what we have, the constructor must have a protected argument which declares an interface for the many readonly properties, and the subclass can then inject named arguments to be assigned to the readonly properties/fields. This is much better than the hacks listed above (// @ts-ignore or overwriting the property)

bogdan commented 4 years ago

My variant for this type of problem:

abstract class NamingService {
  readonly name: string;
}

class Eth extends NamingService {
  readonly name =  'ENS';
}

console.log(new Eth().name);

It is weird though that I can not set the property in a constructor of a subclass.