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

Overriding fields in child leads to broken class contracts and other unexpected behaviour #35049

Closed
goatfryed opened this issue Nov 11, 2019 · 4 comments

Comments

@goatfryed
Copy link

goatfryed commented Nov 11, 2019

Bug Report

Hello There,
I wanted to get into TypeScript for real for quite some time and finally started my first bigger side project using it. I really appreciate the great work 😊.

TypeScript Version: 3.8.0-dev.20191105

Search Terms: fields, inheritance, override optional type with mandatory, overridden field not accessible after super call

While carelessly abusing the type system, I ran into a case, where I would expect an type error:
When I extend a class with an optional, public field and narrow the field down to a mandatory field, typescript doesn't raise errors on the class or when consuming it, even though i'm breaking class contracts and i'm making my code error prone.
Also, typescript completely forgets all about the parent class, that's using the field as well.

interface SomeOptional {
    value?: number,
    notOverriden?: string;
}

class OtherClass {
    value: string = "Hello";
}

class Parent implements SomeOptional {

    value?: number;
    notOverriden?: string; 
    protected mandatory: {} = {};

    constructor({value}: SomeOptional) {
        this.value = value;
    }
}

class DefaultingChild extends Parent {
    value: number;
    mandatory: {}; // mandatory gets initialized in parent and is safe to access

    constructor(toCopy: SomeOptional) {
        super(toCopy);

        if (!this.value) { // super is called, so accessing it should be fine.
            this.value = 3;
        }
        console.log(this.notOverriden); // accessing notOverriden is obviously okay.
    }
}

function letsBreakContract() {
    const sample = new DefaultingChild({});

    if (sample.value === undefined) {
        throw new Error("actually unreachable due to class contract of DefaultingChild");
    }
    (sample as OtherClass).value = "Broke contract"; // raises an error, as it should.
    sample.value = undefined; // raises an error, as it should.
    sample.value = 3;
    (sample as SomeOptional).value = undefined // doesn't raise an error, but should;

    if (sample.value === undefined) {
        throw new Error("should be unreachable due to class contract of DefaultingChild, but isn't");
    }
    return 1;
}

Expected behavior:
In case of field "value", I would expect that typescript yields an error on the field, because trying to overriding the field with a different type - even though related - introduces possibility to break class contract (see line 42).
Even though I can ensure type safety in the child class, I can't ensure that the type contract isn't broken, when interacting with any parent or interface.

To be honest, I first saw it was an bug that typescript couldn't figure out that this.value was already set, but the more i thought about my use case, the more I saw my bad design approach in trying to override parent fields anyway

I think, it would be best to not allow any type changes in child fields. Also, typescript shouldn't forget that the parent class is using the field as well, when the field is overridden in order to make protected fields public or something.
I'm aware of #1617 and overridden initialisier, but that's not the case here.

Actual behavior:
Typescript doesn't know that overridden field gets initialized in the parent class.
The type system allows use cases, where class contract can be broken.

Playground Link

I could only find issues related to types and ovrrides that address method issues. e.g. #3402 #31470

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Nov 11, 2019

There's something about mutable properties and invariance somewhere on the issues page.

I gotta go look for it.


[EDIT]

#33234

#18770

@fatcerberus
Copy link

(sample as SomeOptional).value = undefined // doesn't raise an error, but should;

Why would this line be an error? SomeOptional says value can be undefined and you've explicitly cast sample to that. The as cast is just an upcast to a supertype so that's allowed. If there is to be an error here, it should be on the definition of DefaultingChild. Having an error on the upcast would just be closing the barn door after the horse already got out.

@goatfryed
Copy link
Author

goatfryed commented Nov 12, 2019 via email

@goatfryed
Copy link
Author

On a second thought, consider the following example that's not using inheritance

interface Optional {
    value?: Object
}

interface Mandatory {
    value: Object
}

const obj: Mandatory = { value: {} };
(obj as Optional).value = undefined;

@AnyhowStep thanks for linking the issues. I should have looked for the search word invariance. I guess this can be closed as a duplicate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants