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

A readonly class property that overrides a non-readonly property can still be reassigned in the base class (and vice-versa) #8496

Closed
malibuzios opened this issue May 6, 2016 · 11 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@malibuzios
Copy link

malibuzios commented May 6, 2016

TypeScript Version:
1.9.0-dev.20160506

Code

class Base {
    prop: number;

    baseMethod() {
        this.prop = 4321;
    }
}

class Derived extends Base {
    readonly prop: number = 1234; // this could also be an accidental name collision

    derivedMethod() {
        this.baseMethod();
    }
}

let x = new Derived();
console.log(x.prop); // prints 1234
x.derivedMethod();
console.log(x.prop); // prints 4321

Expected behavior:
Trying to re-declare prop as readonly in Derived should error:

A writable property cannot be overriden by a readonly property

Actual behavior:
No error, prop can be reassigned in the base class.

@malibuzios
Copy link
Author

malibuzios commented May 6, 2016

It can also happen in the other direction, i.e. where in Base, prop is readonly and in Derived it isn't.

I think this case may be subtly different as a class cannot even 'know' if it is being used as a base class to anything. The contract here strongly and unambiguously suggests that prop should have the value 1234 forever:

class Base {
    readonly prop: number = 1234;

    sanityCheck() {
        if (this.prop !== 1234)
            throw new Error("Assertion failed, a readonly property has somehow been modified");
    }
}

class Derived extends Base {
    prop: number = 4321;
}

@malibuzios
Copy link
Author

malibuzios commented May 6, 2016

I think that for the first case, there may still be some utility in creating a 'read-only view' for the property in the base class. In that case the re-declared property must not be annotated with any specified type or reassigned in the derived class constructor. If non-nullability is enabled, the property in the base class must also be initialized with a value if it is not optional:

class Base {
    prop: number = 1234; // must be initialized if strict null checks are enabled
}

class Derived extends Base {
    readonly prop; // a 'read-only view' for the base class property

    constructor() {
        super();
        this.prop = 4321; // this would error
    }   
}

However, with this approach (I mean, without an explicit modifier), there could be some issues with noImplicitAny but I haven't really looked at that yet. I'm also considering as an alternative, what would be the best override-like modifier to use here in relation to #2000 and whether or not it should be mandatory.

In general, one major disadvantage I find for this is that it would break the association of readonly as a sign of the property being 'immutable' somehow (at least in the case of primitives) - both for humans, and also possibly for the compiler as well. Anyway, I only mentioned this possible 'feature' because I thought that team members might find it interesting, not because I personally wanted to promote it (I haven't really decided at this point, but I'm leaning towards the negative side).

@RyanCavanaugh
Copy link
Member

This was a compromise we had to make for the sake of backward compatibility.

More discussion about reasoning for this behavior is described here #6532 (comment)

A relevant thing for understanding that is that we structurally check whether a derived class is assignable to its base class. This is what causes it to be illegal to e.g. redeclare prop as a string when it was a number in the base class.

@malibuzios
Copy link
Author

@RyanCavanaugh

I don't agree with this compromise.

@RyanCavanaugh RyanCavanaugh added the Design Limitation Constraints of the existing architecture prevent this from being fixed label May 6, 2016
@malibuzios
Copy link
Author

malibuzios commented May 6, 2016

@RyanCavanaugh

I feel I don't have anything more to say here. Completely speechless.

The comment you linked doesn't mention this:

class Base {
    readonly prop: number = 1234;

    sanityCheck() {
        if (this.prop !== 1234)
            throw new Error("Assertion failed, a readonly property has somehow been modified");
    }
}

class Derived extends Base {
    prop: number = 4321;
}

which is unbelievably horrible. You should have seriously considered not to introduce this feature at all. This borders on insanity! I mean, now every class that anyone writes where they innocently annotate a property as readonly, could be violated by a derived class? That's just nuts!

I don't even understand why I spend my spare time trying to help you guys. I'm sorry, it is probably more effective that I put all this effort designing a language of my own.

@RyanCavanaugh
Copy link
Member

Again, it's a compromise (that means some people aren't going to think it's great, which is understandable).

The important use case here is that you might want to use readonly on one of your class fields, but you're inheriting from a base class definition from a definition file that was written before the readonly keyword existed (or one that was generated from a codebase that doesn't bother to use readonly annotations for whatever reason). This can often be the case for a class field that is in practice readonly but hasn't gotten the readonly annotation yet (for reasons listed above).

Basically we see properties as "possibly readonly" or "definitely readonly", and allow assignments from definitely-readonly things to possibly-readonly things. It was either this, or break every piece of code in the world, or add a new "writeable" (or something) keyword and force that keyword to be added to every declaration in every file everywhere.

@malibuzios
Copy link
Author

@RyanCavanaugh

I understand what you describe but I don't think I would classify that as 'backwards compatibility', as the introduction of the modifier is not a breaking change. It is more about trying to provide 'convenience', though at some very high cost.

Again, there is a contract here. When the programmer specifies readonly prop = 1234, they mean it would stay that way forever. If a derived class either intentionally or by mistake would violate that contract, it could lead to a disaster (both figuratively and in practice).

This is the contract that programmers mentally build for it. There's no point at trying to be overly-sophisticated here. If you try to tell them otherwise, they would simply disagree with you. They are right, and 'you' are 'wrong' because it's their contract, not yours.

I feel that eventually, these complex implicit mechanics of "possibly readonly" or "definitely readonly" would be something that must be understood and reasoned about by the programmers themselves, essentially putting more mental load on them rather than reducing it (which defeats the original intention of the feature).

I think perhaps that the team's eagerness to introduce this feature 'at any price' may have went so far, maybe to the point where you've lost 'contact' with the purpose of the thing itself.

@malibuzios
Copy link
Author

malibuzios commented May 7, 2016

@RyanCavanaugh

Several questions:

For the case where one wants to inherit from a base class defined in a declaration file:

  1. How often does that happen in practice, compared to only using the declarations for consumption? (ES6 is rather new, and ES5 doesn't really have a well-defined concept of 'classes').
  2. Why would anyone need to reassign a property that is supposed to be read-only? (but perhaps not marked that way). If the property is initialized in the base class, they probably shouldn't touch it! In any case, even if they did want to reassign that property during initialization, how would they know if the property is initialized in the base class at all?
  3. Even if they did want to reassign it with a different value during initialization, why would they need to override the declaration itself?
  4. If they only wanted to effectively render it 'read-only' despite the fact that it's not marked that way, why couldn't they just go ahead and mark it as readonly in the declaration file directly? or possibly use something like the 'read only view' idea I mentioned in a previous comment?

For the case where one wants to provide a base class through a declarations file:

  1. If one marks a property as readonly in the declaration file, why would the deriving class need to re-declare it at all? can't it just use the declaration as-is?

In general:

  1. How often are base class declarations for properties overridden at all? what is the benefit in doing that in a structurally typed language with implicit covariant casts? (I've never, ever done that myself, and I've demonstrated some of the risks involved in a different issue).

(Edits: rephrased a and expanded a bit)

@malibuzios malibuzios changed the title A readonly class property that overrides a non-readonly property can still be reassigned in the base class A readonly class property that overrides a non-readonly property can still be reassigned in the base class (and vice-versa) May 7, 2016
@kitsonk
Copy link
Contributor

kitsonk commented May 7, 2016

  1. How often does that happen in practice?

Quite a lot. Many project's distributables are transpiled code with an accompanying declaration file. Outside of that, there are many non-TypeScript projects where people are using existing community declaration files to integrate their code into a project, to build on something not written in typescript.

  1. Why would anyone need to 'override' a property that is supposed to be read-only? (and perhaps not marked that way). If the property is initialized in the base class, they probably shouldn't touch it! In any case, even if they did want to override that value during initialization, how would they know if the property is initialized in the base class at all?

Why should they, ¯_(ツ)_/¯ but this behaviour better matches the runtime behaviour of JavaScript, in the sense that it is quite easy to provide something that obscures what is further up the prototype chain. TypeScript is rightfully constrained by the behaviours JavaScript. The team for a long time avoided readonly because people might incorrectly assume it does more than it seems to say it does. It behaves like a non writable property, or a property with only a get accessor and flows through the types in that fashion, which again, matches the behaviour of JavaScript. Expecting it to do more means that it is likely to diverge.

I'm sorry, it is probably more effective that I put all this effort designing a language of my own.

Again, personal opinion, but I hope that you can respect that you, essentially coming out of the blue and unloading extremely well thought out, but rather intense and loquacious treatises can be a bit overwhelming. Again, personal opinion, but one of the biggest things I see you struggling with in understanding the "why" is that TypeScript is not a new language. Dart was a new language. Swift was a new language. TypeScript is trying to embrace ECMAScript and build on top of it and actually some of the early additional constructs that "added" to ECMAScript have caused all sorts of carnage down the road, especially when it isn't fully erasable. The module concept for TypeScript has made it challenging to embrace ES6 modules. I argue that both enum and tuples sort of don't ever quite fit in well, because they are things that don't exist in ECMAScript land (though I am thankful for them). So the TypeScript team has to make compromises all the time. They now have many many many projects out there that they can't break. The problem with "legacy" support also coupled with erasability. My opinion is that 9% of the time the TypeScript team make the right choices, even though some of them have greatly frustrated me and the patterns I seek out.

@AJamesPhillips
Copy link

( @kitsonk was that meant to read "99% of the time" ?

Thanks for everyone's contributions. This is a hard topic precisely because TypeScript is not a new language and it's building on top of a particularly characterful one.)

@malibuzios
Copy link
Author

@kitsonk

I don't feel the way I presented this neither my questions are "extremely well thought out" (I wish..). Anyway, I edited them again - tried to improve and reorganize a bit, but I still think I could do much better.

I'm trying not to make this thread very long (it isn't really at this point) so I'll avoid diverging into more personal or meta subjects. It may appear to be written in a 'torrent-like' way, but that's perhaps because I feel there's only a very small time window here for any change to happen (although I'm personally very pessimistic). In a few weeks, they would push this feature to production. At that point it would be about 100x more difficult to achieve anything meaningful. It would be virtually completely lost.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

4 participants