Skip to content

Assignment of immutable record into mutable record variable does not produce error #52225

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

Closed
MrWolfZ opened this issue Jan 13, 2023 · 9 comments
Labels
Duplicate An existing issue was already created

Comments

@MrWolfZ
Copy link

MrWolfZ commented Jan 13, 2023

Bug Report

🔎 Search Terms

readonly

🕗 Version & Regression Information

I saw this behavior on TypeScript 4.9.4.

⏯ Playground Link

bug workbench with relevant code

💻 Code

const immutableRecord: Readonly<Record<string, unknown>> = {};
immutableRecord.foo = 'bar'; // as expected, produces error
const mutableRecord: Record<string, unknown> = immutableRecord; // this should error, but does not
mutableRecord.foo = 'bar'; // as expected, no error

🙁 Actual behavior

I noticed that a readonly record can be assigned to a mutable record without a compile error. I assume this is because everything is assignable to the empty object (as described here) but it is very unintuitive, since we are not actively creating a class or type with no properties. If you ask any developer, they would assume this code to produce an error.

🙂 Expected behavior

Optimally, the assignment of an immutable record into a mutable record variable should not be allowed.

@fatcerberus
Copy link

This is not specific to Record. In general { readonly foo: T } is assignable to { foo: T }. It doesn’t really mean “immutable”, but rather a much weaker guarantee - only that it can’t be mutated through the readonly reference.

@fatcerberus
Copy link

fatcerberus commented Jan 13, 2023

More or less a duplicate of #11180/#13347. See also #11481, which sought to add a flag to fix this. #14150 is somewhat related, too

@MartinJohns
Copy link
Contributor

Hence why I always try to tell people: readonly does not mean immutable. It only means readonly via that interface.

@Fryuni
Copy link

Fryuni commented Jan 13, 2023

The behavior is inconsistent:

const immutableRecord: Readonly<Record<string, unknown>> = {};
// Doesn't fail
const mutableRecord: Record<string, unknown> = immutableRecord;

const immutableArray: ReadonlyArray<string> = [];
// This breaks, as it should
// The type 'readonly string[]' is 'readonly' and cannot be assigned to the mutable type 'string[]'
const mutableArray: Array<string> = immutableArray;

@fatcerberus
Copy link

fatcerberus commented Jan 13, 2023

It’s consistent w.r.t. to the structural typing rules: the readonly array is not assignable only as a side effect of missing methods, not directly due to its readonly-ness (despite what the error message says)

@Fryuni
Copy link

Fryuni commented Jan 13, 2023

It’s consistent w.r.t. to the structural typing rules: the readonly array is not assignable only as a side effect of missing methods, not directly due to its readonly-ness (despite what the error message says)

Aka, it is inconsistent. From the user's perspective, it doesn't really matter if the part that works as expected (the array) works because someone made it work or because of a happy coincidence.

@fatcerberus
Copy link

fatcerberus commented Jan 13, 2023

It’s consistent with normal assignability rules, given what types result in both cases. The fact that the readonly keyword is being used for two separate concepts is unfortunate, but overloading of keywords is hardly unique to TypeScript.

@fatcerberus
Copy link

fatcerberus commented Jan 13, 2023

fwiw the lines between “works by happy coincidence” and “someone made it work this way” tend to be blurred in a structural type system because the compiler only cares about the contents of your type and not what keywords were used to construct it. Higher-order expectations don’t really translate well in that environment, and you often have to trade one inconsistency for another.

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label Jan 13, 2023
@MrWolfZ
Copy link
Author

MrWolfZ commented Jan 16, 2023

well, you learn something new every day. this doesn't seem like it will be changed, so I'll close this issue

@MrWolfZ MrWolfZ closed this as completed Jan 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

5 participants