Readonly properties can be modified in derived classes

[Mcfloy]

This issue is created even if there was a pretty complete issue about the purpose of the readonly keyword ( #8496 (comment) ) and I'll just quote @RyanCavanaugh :

Basically we see properties as "possibly readonly" or "definitely readonly", and allow assignments from definitely-readonly things to possibly-readonly things.

I totally agree that we must prevent codes from libraries and/or node modules, as still today there's modules that doesn't include d.ts files and the DefinitelyTyped repo doesn't have 100% of updated d.ts files.

BUT as the main purpose was to do a compromise, let's add a compromise over the compromise, see the code below to understand why we should make the user more careful about mutating a readonly property when deriving a class.

TypeScript Version: 3.3.0-dev.201xxxxx

Search Terms: final readonly properties derived inherits

Code

abstract class Pet {
    protected abstract race: string;

    protected abstract sayMyRace(): void;
}

class Dog extends Pet {
    protected readonly race: string = "Dog";

    sayMyRace() {
        console.log(this.race);
    }
}

class NotADog extends Dog {
    protected readonly race: string = "Robot";
}

const scooby = new Dog();
scooby.sayMyRace();

const marvin = new NotADog();
marvin.sayMyRace();

Expected behavior:
Error saying that race in NotADog cannot be modified as it was already declared in class Dog and we set the same attributes in both cases, meaning we do want to override a constant.
Actual behavior:
Property is overwritten, ignoring the readonly property from the already existing parent's property.

Playground Link:
here

Note that in the documentation the example shows an error when trying to mutate the property created in the class.

I'm not asking to put a check rule on every keyword in every line of code, but to accept that if a class have a readonly property, nobody can mutate it from outside, I'm not talking about the interfaces and/or declaration file to help to make the compromise, but the Typescript documentation state:

The easiest way to remember whether to use readonly or const is to ask whether you’re using it on a variable or a property. Variables use const whereas properties use readonly.

If a property with readonly is a variable with const, then we must keep this in line and prevent any mutation from a class property to another class property, at least that would allow to add a safety check in the code.

Also the documentation regarding the readonly properties in Interfaces state that

Some properties should only be modifiable when an object is first created.

If the team says that a mutation between an explicitly typed readonly property from a class to a subclass is normal as the compromise was done and that changing the rules would break the declaration files (even though two years had past)

So to sum up :
I open this issue as a bug because the documentation states that a readonly property is a constant variable, and that based on the few informations about it (still from the documentation), this is not the expected behavior.

I also considered the previous discussion and the compromise made to prevent any blocking backward compatibility, and asking only to use the readonly check rule from class to class by default, and allow more in a tsconfig rule later.

[YassinMahmoudEl-Zouhery]

is there is any issue now

[Mcfloy]

Depends on if you want to prevent someone to overwrite anything by just deriving its main class. It's not a crash bug, so it's not a fatal warning, but people should be aware that in TS nothing can avoid the rewrite of a constant property.

In a JS way, we could use the Object.freeze() "way to do" to prevent this problem. Just asking to have a syntactic sugar version of this freeze method when declaring a readonly property in a class and prevent any modification when deriving the class and attempting to mutate the said property.

Edit: As freezing an object will prevent it to be deleted, using Object.defineProperty() could be another way to avoid any mutation on a property, in the runtime and the compile time.

[YassinMahmoudEl-Zouhery]

let's use the Object.defineProperty() way

[Mcfloy]

Okay so here's a decorator that performs a better check than Typescript unfortunately :

function readonly(target: any, key: keyof any) {
    let _value = target[key];
    Object.defineProperty(target, key, {
        get: () => _value,
        set: value => {
            if (_value) {
                throw new Error(`Cannot assign to '${String(key)}' because it is a read-only property.`);
            }
            _value = value;
        }
    })
}

class MyClass {

    @readonly
    readonly props: string = "toto";

    constructor() {}

    readProps() {
        console.log(this.props);
    }
}

class B extends MyClass {
    props = "redefined";

    constructor() {
        super();
    }
}

const classInstance = new B();
classInstance.readProps();

If you launch this, you'll get an Error for attempting to redefine a property.

If you remove the decorator from the property, compiler will still say yes, but in runtime you'd be able to redefine again from the constructor the readonly property, and this case shouldn't be allowed.

The decorators lock any further mutation, but allows you to do

class MyClass {
    @readonly
    readonly props: string;

    constructor() {
        this.props = "toto";
    }
}

As there's no mutation, only initialization.

Now we only need the checker to know if it's readonly (isReadonly) when attempting to mutate the value.

[amitbeck]

Funnily enough, unlike the example above, this doesn't work:

abstract class Pet {
    protected abstract race: string;
}

class Dog extends Pet {
    protected readonly race: string = "Dog";
}

class NotADog extends Dog {
    constructor() {
        this.race = "Robot"; // error TS2540: Cannot assign to 'race' because it is a read-only property
        //   ~~~~
    }
}

But, like the example above shows, there's a workaround for this which is to redefine the readonly property in the derived class. Very weird behavior IMO.

[vicary]

Despite multiple closing attempts in the past years, if people keep coming back to this it means industry standards or deeply rooted habits that a language should pay attention to.

I would love the readonly property to be inherited.