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

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

Closed
4 tasks done
ahamid opened this issue Jun 21, 2018 · 12 comments
Closed
4 tasks done
Labels
Declined The issue was declined as something which matches the TypeScript vision

Comments

@ahamid
Copy link

ahamid commented Jun 21, 2018

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:

  • This wouldn't be a breaking change in existing TypeScript / JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. new expression-level syntax)
@mhegazy
Copy link
Contributor

mhegazy commented Jun 21, 2018

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
Copy link
Author

ahamid commented Jun 21, 2018

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
Copy link
Contributor

mhegazy commented Jun 21, 2018

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
Copy link
Author

ahamid commented Jun 21, 2018

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
Copy link
Contributor

mhegazy commented Jun 21, 2018

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

Correct.

@ferdaber
Copy link

ferdaber commented Jun 28, 2018

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.
DefinitelyTyped/DefinitelyTyped#26813 (comment)

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
Copy link
Contributor

mhegazy commented Jun 28, 2018

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
Copy link
Collaborator

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

@thw0rted
Copy link

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

@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
Copy link

mcshaz commented Apr 16, 2019

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

bogdan commented Dec 4, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Declined The issue was declined as something which matches the TypeScript vision
Projects
None yet
Development

No branches or pull requests

8 participants