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

No type error on length-mutating methods with readonly length #56685

Closed
rotu opened this issue Dec 6, 2023 · 14 comments
Closed

No type error on length-mutating methods with readonly length #56685

rotu opened this issue Dec 6, 2023 · 14 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@rotu
Copy link

rotu commented Dec 6, 2023

🔎 Search Terms

readonly length, array

🕗 Version & Regression Information

  • This is a crash
  • This changed between versions ______ and _______
  • This changed in commit or PR _______
  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about _________
  • I was unable to test this on prior versions because _______

⏯ Playground Link

https://www.typescriptlang.org/play?target=99&jsx=0&ts=5.4.0-dev.20231205#code/GYVwdgxgLglg9mABAdxlAFgMQE5wF4CmYAMkQOYYA8AgogQB5REAmAzouANZhzJgDaAXQB8ACgCG2AFzUAlAFgAUAG8liRBASsoibAVYgANjoC8iatmziAngDpguALYTsCxeoDyAIwBWBaLbMBMAwYAQACrgADgTYUNaiegbGADQARIbkGGkpysjYaOJemVLA4oasBAC+bup6UCDYSElGOuLstABkiMq6BOLMCIbWiJlgFOhS5vwA5GMTM4KIVUorikqaYNqIjtYWViNmqBg4+ESk4xii-ACMKYgATPcAzIJuu-s2tlEgrOiiABZZIgAPQgxAAOTgiHiMToljg2FsiAASuBYI4CPDcEigA

💻 Code

function withFrozenLength<A extends unknown[]>(ar:A)
{
  const result = Array.from(ar)
  Object.defineProperty(result,"length",{writable:false})
  return result as A & { readonly length: A['length'] }
}

const myArray = withFrozenLength([1, 2, 3])
myArray.push(4) // No type error. Runtime error.

🙁 Actual behavior

Typescript accepts the above code. The code crashes at runtime with a runtime TypeError like:

Uncaught TypeError: Cannot assign to read only property 'length' of object '[object Array]'

🙂 Expected behavior

The push method should be rejected by the type checker.

Additional information about the issue

Partial overlap with a similar issue involving tuple types #40316

@MartinJohns
Copy link
Contributor

MartinJohns commented Dec 6, 2023

The push method should be rejected by the type checker.

Based on what? Your type clearly states it has such a method. The existence of a read-only length property does not change the rest of the type.

@rotu
Copy link
Author

rotu commented Dec 6, 2023

Based on what? Your type clearly states it has such a method. The existence of a read-only length property does not change the rest of the type.

The push method assigns the length property. If I were to do the effective equivalent, myArray[myArray.length++]=4, it would be rightly rejected as mutating a readonly property.

Relatedly, making a readonly Array type makes TypeScript think it doesn’t have a push method at all, which seems like it will lead to incorrect flow analysis.

@MartinJohns
Copy link
Contributor

The push method assigns the length property.

But that's an implementation detail of your method. The type system is completely unaware of this. Besides that, using a type assertion as you tell the compiler "trust me, this is the type", and when your implementation does not match this type you're eventually going to have a bad time.

Relatedly, making a readonly Array type makes TypeScript think it doesn’t have a push method at all

Writing readonly Array<T> will make it a ReadonlyArray<T>, which does not include any mutating methods. That's the correct type to use when you want to provide a read-only array.


I don't see anything wrong with this behavior.

@rotu
Copy link
Author

rotu commented Dec 6, 2023

The ECMAScript spec has a note:

This method is intentionally generic; it does not require that its this value be an Array. Therefore it can be transferred to other kinds of objects for use as a method.

It’s not merely an implementation detail - it’s a feature of the push method.

@fatcerberus
Copy link

TypeScript doesn't know what push does beyond the types of its parameters and return type - it's just another method call, like any other. The compiler doesn't look at the implementation of functions when you call them, it only checks whether the argument types are compatible. It's no different to this:

const foo: { readonly foo: string } = { foo: "bar" };
fooey(foo);  // no error
console.log(foo.foo);  // prints "qux"

function fooey(foo: { foo: string }) {
    foo.foo = "qux";
}

readonly is not as strong a guarantee as you think it is. It just means you can't mutate the property locally.

@rotu
Copy link
Author

rotu commented Dec 6, 2023

Writing readonly Array<T> will make it a ReadonlyArray<T>, which does not include any mutating methods. That's the correct type to use when you want to provide a read-only array.

The absence of a method is unfortunately different from saying that calling the method is illegal. For instance, it can be implicitly re-introduced by narrowing:

let x = [] as readonly number[]
if ('push' in x && typeof x.push === 'function'){
    x.push(1)
}

The lack of those properties does prevent accidentally treating a ReadonlyArray as an Array, which is good. But it doesn't do much for preventing accidental mutation by generic methods.

TypeScript doesn't know what push does beyond the types of its parameters and return type - it's just another method call, like any other.

The type of push can also depend on the type of this, just as map does.

readonly is not as strong a guarantee as you think it is. It just means you can't mutate the property locally.

I'm realizing this, and I think it might be a limitation of the TypeScript type system that you can't meaningfully encode a readonly property in a way that it can't be implicitly forgotten. My attempt below by declaring getters and setters does not achieve the desired result:

// @target: ES2022

type T = symbol;
function Make<T>(): T{ throw new Error("unimplementable") }

// Array versus readonly Array
Make<ReadonlyArray<T>>() satisfies Array<T>;
Make<Array<T>>() satisfies ReadonlyArray<T>;
// ReadonlyArray < Array

// property versus readonly property
Make<{readonly a: T}>() satisfies {a:T}
Make<{a: T}>() satisfies {readonly a:T}
// property and readonly property are freely interconvertible

// property implementation
type GetX = {
  get x(): T
  set x(value: never)
}
type SetX = {
  set x(value: T)
  get x(): unknown
}
type GetSetX = {x: T}

Make<GetSetX>() satisfies GetX;
Make<GetX>() satisfies GetSetX;
// GetX and GetSetX are freely interconvertible,

Make<GetX>() satisfies SetX;
Make<SetX>() satisfies GetX;
// GetX < SetX

Make<GetSetX>() satisfies SetX;
Make<SetX>() satisfies GetSetX;
// GetSetX < SetX

@MartinJohns
Copy link
Contributor

MartinJohns commented Dec 6, 2023

from saying that calling the method is illegal.

You can't represent "illegal methods" in TypeScripts type-system. And you can't represent immutability in TypeScripts type-system.

it can be implicitly re-introduced by narrowing:

With that argument everything is pointless, because any lets you do all kind of shenanigans.

@fatcerberus
Copy link

The type of push can also depend on the type of this, just as map does.

Indeed; I was implicitly including this as a parameter when I said “parameters and return type”

@rotu
Copy link
Author

rotu commented Dec 8, 2023

You can't represent "illegal methods" in TypeScripts type-system.

I would think you could by making the function signature require an argument of type never. Or by having some failing extends constraint.

And you can't represent immutability in TypeScripts type-system.

Yeah. I don’t understand why my above implementation of GetX decays into GetSetX given that function types should be contravariant in their argument types.

With that argument everything is pointless, because any lets you do all kind of shenanigans.

Emphasis on “implicitly”. Yes, any is the “hold my beer” of TypeScript 🥲.

———-

At any rate, I think this issue is a design limitation (at least unless/until #13347 is resolved)

@MartinJohns
Copy link
Contributor

I would think you could by making the function signature require an argument of type never.

It's valid to call functions expecting an argument of never when you have a value typed never. This is useful e.g. for a panic function.

(at least unless/until #13347 is resolved)

#13347 wouldn't change anything, because, again, your type says it has a push method, so it's possible to call the push method. Having a read-only length property does not magically make that method disappear.

@rotu
Copy link
Author

rotu commented Dec 8, 2023

It's valid to call functions expecting an argument of never when you have a value typed never. This is useful e.g. for a panic function.

Yes. Such a function is callable only by constructing a value of type never (or any). That is, only by using unsound parts of the type system.

#13347 wouldn't change anything, because, again, your type says it has a push method, so it's possible to call the push method. Having a read-only length property does not magically make that method disappear.

It wouldn’t fix the issue directly, but it would allow either declaring a function push({length:number}, /*…*/) so that calling it with a {readonly length:number} would require first explicitly narrowing away the readonly modifier.

To make a push method safe, you’d do something similar with a constraint on the type of this.

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Dec 8, 2023
@RyanCavanaugh
Copy link
Member

readonly doesn't mean immutable. A readonly property is one that you don't have write access to. It doesn't mean that the property value itself cannot change. There's nothing wrong, inconsistent, or unsound with an object having a readonly property that changes as a side effect of some other operation.

@rotu
Copy link
Author

rotu commented Dec 9, 2023

readonly doesn't mean immutable. A readonly property is one that you don't have write access to. It doesn't mean that the property value itself cannot change. There's nothing wrong, inconsistent, or unsound with an object having a readonly property that changes as a side effect of some other operation.

That’s true. A readonly property may change as the side effect of another operation. That is expressly not what happens here.

The Array.prototype.push function has no special access to the private underlying length. Per spec, it goes through the public length setter. That’s why making the property unwritable with Object.defineProperty results in a runtime TypeError.

I fail to see how an operation which calls the setter could possibly be in semantic agreement with something declared readonly. Design limitation, sure. But also semantically wrong.

@typescript-bot
Copy link
Collaborator

This issue has been marked as "Working as Intended" and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@typescript-bot typescript-bot closed this as not planned Won't fix, can't repro, duplicate, stale Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

5 participants