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

add enforceReadonlyAssignability flag for smoother transition to true readonly modifiers #13002

Closed
zpdDG4gta8XKpMCd opened this issue Dec 17, 2016 · 27 comments
Labels
Duplicate An existing issue was already created

Comments

@zpdDG4gta8XKpMCd
Copy link

zpdDG4gta8XKpMCd commented Dec 17, 2016

this can be seen in the nightly build as of Dec 17 (as well as at playground)

interface ReadOnlyData {
    readonly value: number;
}

interface WritableData {
    value: number;
}

function erase(data: WritableData): void {
    data.value = 0;
}

const readOnly: ReadOnlyData = { value: 1 };

erase(readOnly); // expected a type error, actual: no problem

this can't be serious, can it?
why is it called readonly?

please note there are no type assertions or any other attempts to trick the type system, it just simply doesn't work

50+ errors left uncaught

UPDATE


constructive discussion starts here: #13002 (comment)

@RyanCavanaugh RyanCavanaugh added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Dec 17, 2016
@RyanCavanaugh
Copy link
Member

This was described at #6532 (comment)

@dead-claudia
Copy link

@Aleksey-Bykov readonly basically means you may not modify it. It guarantees no actual immutability of the binding (heck, even Scala doesn't provide facilities that deep - val name = value and def name = value carry the same signature, and toString is a property). Consider it a case of "we're all consenting adults here", not "you shall not modify this value ever" (the latter is what Object.freeze is for).

@zpdDG4gta8XKpMCd
Copy link
Author

zpdDG4gta8XKpMCd commented Dec 18, 2016

@iashmeadows next time you are buying a car and you read "a sport BMW coupe", you say sweet, just what I wanted, you write a check, get your car and drive it home, next day you are all excited to show it to your friends, you stop at a store to get a 6 pack on the way there, you open the trunk and put your beer in it, when you close it you BMW turns into Honda Odyssey... you are like what? you are all pissed and stuff, you take the beer out and it turns into BMW again, you are like what!!! you call the dealer who sold it to you, they drop on you first, it's normal, then they pick up and say you know it's accoring to issue #6532 where some housewife complained that the trunk of BMW isn't big enough for shopping at Costco and taking kids to the beach, and that's exactly why we made a hard decision to turn iT into Honda Odyssey every time anything is put in its trunk, you are like wtf!!! So you go there and you are like: nothing was said about this shit anywhere! And what you hear back is: well we never said we sell true BMW''s why are you surprised? And people around who saw it are like: hey let's be adults here. F**k what?

@DanielRosenwasser
Copy link
Member

Regardless of whether or not I agree on that perspective of readonly, I have to admit I enjoyed the analogy. 😄

@dead-claudia
Copy link

dead-claudia commented Dec 18, 2016

@Aleksey-Bykov In case you missed it, the "we're all consenting adults here" was a reference to a common saying in the Python ecosystem, regarding member privacy, types, etc. 😉

(Basically, conventions are usually sufficient to imply "you're on your own" for diving into internals, and there's only so much you can do to prevent them from doing it, especially without the use of closures.)

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Dec 18, 2016

@isiahmeadows

In case you missed it, the "we're all consenting >adults here" was a reference to a common >saying in the Python ecosystem, regarding >member privacy, types, etc. 😉

Python gets a lot of things wrong. I never understood the appeal and popularity of that language. Irregardless that doesn't make it a good basis for anything.

It guarantees no actual immutability of the >binding (heck, even Scala doesn't provide >facilities that deep - val name = value and def >name = value carry the same signature, and >toString is a property).

I don't see the relevance, val and def both mean the reference, to a property method or variable, may not be reassigned. They differ in eagerness vs laziness and some other characteristics, type inference characteristics for example, but they don't have different mutability. With respect to deep immutability, which is not what is asked for here, Scala does allow it, you just have to use the right modifier, val or def, all the way down and not var. Scala does enforce this at the type system level.

Regardless, Scala has a nominal type system with support for optional explicit structural subtyping. Furthermore it doesn't permit these types of readonly violations. I don't think it is the right analogy.

I'm not saying TypeScript made a bad decision here, but personally I think it would have been great if readonly had resulted in code emit using defineProperty({ writable: false, ... }), at least it would have made classes useful. But that's a whole other argument.

@masaeedu
Copy link
Contributor

Type driven emit would be nice in many scenarios, but even if we never get that, a type error here would be useful. Do readonly properties only get checked for reassignment within class members right now?

@danielearwicker
Copy link

Re: comments above, the issue here is not to do with emit, or the fact that readonly doesn't mean const. It's just the looseness of type checking between the interfaces with mismatched properties. If a property could be explicitly marked mutable, it would not be satisfied by a readonly property and the conversion disallowed.

@zpdDG4gta8XKpMCd
Copy link
Author

here is a dichotomy: a property is either readonly or writable, there is no other outcome

when we say { x: number } we mean writable x
when we say { readonly x: number } we mean x is only for reading

what else do we need? what is the third state implied by mutablem

@danielearwicker
Copy link

That would be the ideal if the readonly modifier had existed forever. The problem is backward compatibility. To quote @ahejlsberg in the comment linked to above:

Specifically, we can't interpret the absence of a readonly modifier to mean read-write, we can only say that we don't know. So, if an interface differs from another interface only in the readonly modifiers on its properties, we have to say that the two interfaces are compatible. Anything else would be a massive breaking change.

So now we have a trichotomy:

{ readonly x: number } - x is definitely not writable
{ mutable x: number } - x is definitely writable
{ x: number } - x may or may not be writable - coder has not specified

If mutable was added to the language then your:

interface WritableData {
    mutable value: number;
}

would no longer be compatible with ReadableData. It would allow us to specify definite mutability wherever we needed that safety.

I think of this as similar to the bivariance compromise in TS today. Any fix to it in the future will allow us to improve the situation where we ask for it, but the default will have to continue to be bivariance, to avoid breaking so much existing user code.

(OTOH as TS 2.0 was a major version bump, maybe a massive breaking change would be acceptable to some, but TS is more cautious than that, which is a good thing IMO).

@aluanhaddad
Copy link
Contributor

@Aleksey-Bykov Indeed a mutable modifier is effectively meaningless. Everything is mutable unless otherwise indicated.

@danielearwicker specifically for that reason there should not be new syntax because the old syntax will have to mean what the new syntax means implicitly and indefinitely. All this will do is create cognitive load

@abenhamdine
Copy link

Specifically, we can't interpret the absence of a readonly modifier to mean read-write, we can only say that we don't know. So, if an interface differs from another interface only in the readonly modifiers on its properties, we have to say that the two interfaces are compatible. Anything else would be a massive breaking change.

It could be behind a flag : usually it's a convenient way to handle these kind of breaking changes.

@danielearwicker
Copy link

@aluanhaddad

because the old syntax will have to mean what the new syntax means implicitly and indefinitely.

But that isn't what the old syntax meant. { name: string; } didn't mean that name was definitely mutable, and so it cannot be changed to mean that now. The world is full of TS 1.x code that uses such declarations for things that should not (or cannot) be modified because that code was written before TS 2. You can't change the meaning of all that code now.

All widely used languages are products of evolution and contain some scars of that process. Only academic niche languages that no one uses are totally spotless.

Look at it this way: suppose functional programming takes over the world and everyone's properties are all now readonly. Now it looks like readonly should have been the default, to remove all the noise of everything being marked readonly. But that change would break most existing apps utterly.

@danielearwicker
Copy link

@abenhamdine - yes though I expect there would be implications for maintaining type definition files so they can be used either way. If we have readonly and mutable then all type definitions must explicitly say which their properties are, so they are not changed by the compiler switch.

So even then, we'd still need mutable.

@aluanhaddad
Copy link
Contributor

But that isn't what the old syntax meant. { name: string; } didn't mean that name was definitely mutable, and so it cannot be changed to mean that now. The world is full of TS 1.x code that uses such declarations for things that should not (or cannot) be modified because that code was written before TS 2. You can't change the meaning of all that code now.

It depends on how you look at it. TypeScript already changed the meaning of a lot of code by introducing the readonly modifier. Recall that TypeScript could not previously model readonly properties like the name property of a function. That meant that it was legal to write

function f() {}
f.name = 'g';

which has no effect under sloppy mode and throws in strict mode. Either way the code was wrong but there was no way to model that.

This also implies the prevailing assumption was mutability which makes perfect sense given the nature of JavaScript.

@zpdDG4gta8XKpMCd
Copy link
Author

zpdDG4gta8XKpMCd commented Jan 9, 2017

@danielearwicker

{ name: string; } didn't mean that name was definitely mutable

no, this is exactly what it meant as far as expressive power of TS at that time, everything used to be modeled with mutable properties (even things that were not supposed to be mutable by design)

You can't change the meaning of all that code now.

exactly, leave the old code alone, readonly is an opt-in thing, if you want it only then you use it

how can it break anything if it's not in your code?

there is no single reason for delivering this feature halfbaked and use "breaking change" as an excuse

whoever stuck in 3 years behind:

  • is free to choose to use old lib.d.ts before readonly
  • or accept the fact that in order to move forward the code needs some maintenance, bite a bullet and embrace readonly
  • or get the latest and greatest shiny lib.2017.d.ts with readonly and remove them wherever they feel to tight
  • or ... you name it

but regardless of how you go about delivering a new "opt-in" feature, it's not an excuse to cut it short in favor of easier transition

@danielearwicker
Copy link

@aluanhaddad

Recall that TypeScript could not previously model readonly properties like the name property of a function... which has no effect under sloppy mode and throws in strict mode. Either way the code was wrong but there was no way to model that.

But that's a great use of a breaking change - to turn a runtime error into a compile time error. Like you say, the code was wrong, and now that can be statically discovered.

@RyanCavanaugh
Copy link
Member

Is anything being discussed here that wasn't already covered in #12 or #6532?

@danielearwicker
Copy link

@Aleksey-Bykov

how can it break anything if it's not in your code?

Because it's in type definitions your code uses.

@RyanCavanaugh - It seems not.

@masaeedu
Copy link
Contributor

masaeedu commented Jan 9, 2017

how can it break anything if it's not in your code?

@Aleksey-Bykov I believe this is because readonly applies implicitly in some scenarios. Specifically, get accessors without matching setters. Existing TypeScript code now behaves as though certain properties were readonly, regardless of whether you ever add readonly to your code.

@zpdDG4gta8XKpMCd
Copy link
Author

{ x: number } - x may or may not be writable - coder has not specified

you cracked me up :) so when you see { x: number } and you need to assign x it basically means the end of the work day for you because you can't make a decision based on how you defined it

@zpdDG4gta8XKpMCd
Copy link
Author

@danielearwicker @masaeedu

let me share a secret:

  • type definitions evolve naturally to keep up with latest features of TS
  • --noLib allows you to ignore new type definitions and use the old ones
  • in the old definitions there is no readonly

problem solved

@danielearwicker
Copy link

Cool, issue closed then!

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Jan 9, 2017

@RyanCavanaugh I suppose not.

An off by default, option in the vein of --enforceReadonlyAssignability, would be a very nice thing to have. It is of course a lot of time and effort, and I do not take that lightly, but it would be very beneficial. It seems as relevant, but of course less broadly valuable, as --strictNullChecks from a code correctness point of view.

@zpdDG4gta8XKpMCd zpdDG4gta8XKpMCd changed the title readonly modifiers are a joke add enforceReadonlyAssignability flag for smoother transition to true readonly modifiers Jan 9, 2017
@zpdDG4gta8XKpMCd
Copy link
Author

in support of the previous speaker, the topic was renamed

@RyanCavanaugh RyanCavanaugh added the In Discussion Not yet reached consensus label Jan 9, 2017
@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript Duplicate An existing issue was already created and removed Design Limitation Constraints of the existing architecture prevent this from being fixed In Discussion Not yet reached consensus Suggestion An idea for TypeScript labels Jan 9, 2017
@RyanCavanaugh
Copy link
Member

Tracking this at #13347

@gcnew
Copy link
Contributor

gcnew commented Jul 19, 2017

To be honest I miss the old title. It was so much more memorable. These days, when I get frustrated with readonly and subsequently try to search for this issue, I actually have hard time finding it.

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

9 participants