New --enforceReadonly compiler option to enforce read-only semantics in type relations
#58296
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
|
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
src/compiler/diagnosticMessages.json
Outdated
| "category": "Message", | ||
| "code": 6718 | ||
| }, | ||
| "Ensure that 'readonly' properties remain read-only in type relationships.": { |
There was a problem hiding this comment.
Just a drive-by comment that as an everyday user of TS, I don't really know what a "type relationship" is. Maybe this could be worded more explicitly as:
| "Ensure that 'readonly' properties remain read-only in type relationships.": { | |
| "Enforce that mutable properties cannot satisfy 'readonly' requirements in assignment or subtype relationships.": { |
# Conflicts: # src/compiler/diagnosticMessages.json # tests/baselines/reference/es2018IntlAPIs.types # tests/baselines/reference/es2020IntlAPIs.types # tests/baselines/reference/inferenceOptionalPropertiesToIndexSignatures.types # tests/baselines/reference/instantiationExpressions.types # tests/baselines/reference/localesObjectArgument.types # tests/baselines/reference/mappedTypeConstraints2.types # tests/baselines/reference/mappedTypeRecursiveInference.types # tests/baselines/reference/unionTypeInference.types # tests/baselines/reference/useObjectValuesAndEntries1.types # tests/baselines/reference/useObjectValuesAndEntries4.types
|
@typescript-bot test it |
|
Hey @ahejlsberg, the results of running the DT tests are ready. Everything looks the same! |
|
@ahejlsberg Here are the results of running the user tests comparing Everything looks good! |
|
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot pack this |
|
Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@ahejlsberg Here are the results of running the top 400 repos comparing Everything looks good! |
|
Can you update |
There was a problem hiding this comment.
Mapped type relationships should also check this:
function test<T>(a: T) {
let obj1: { [K in keyof T]: T[K] } = null as any;
let obj2: { readonly [K in keyof T]: T[K] } = null as any;
obj1 = obj2; // should error
obj2 = obj1;
}Currently both assignments are OK: TS playground (using this PR)
|
nit: In the PR description the second code example does not match the preceding wording - it doesn't use |
|
Any reason why this isn't called |
|
Alternatively, perhaps this should just be an |
This not the current effect of "strict". For example both "noUncheckedIndexedAccess" and "useDefineForClassFields" improve safety. That said some sort of versioned "strict" does seem reasonable. Name-wise I don't see a problem with "enforce" because it is enforcing readonly at the type level, which is where TS operates. If users don't understand that TS is a typechecker, not something that enforces runtime constraints, this is an issue with fundamental knowledge of TS that is really a pre-requisite. "Enforce" is not incorrect within the context. The name could be a little confusing however to those who don't understand the issue, as TS appears to already enforce (at the type level) the readonly modifier. The issue technically applies only to type relationships and manifests in values with types that are considered subtypes of the original type with a readonly property. Something like "consistentReadonly" might better communicate that the change helps to rationalise type relationships, and preserve readonly status within subtypes. "preserveReadonly" is also an option, that I like a little less. |
| new (): Map<any, any>; | ||
| new <K, V>(entries?: readonly (readonly [K, V])[] | null): Map<K, V>; | ||
| readonly prototype: Map<any, any>; | ||
| prototype: Map<any, any>; |
There was a problem hiding this comment.
I'm concerned with this change as prototype is actually read-only on Map, Set, WeakMap, WeakSet, etc.:
@ahejlsberg I'm confused, how does it's name imply it's significance or expected significance as a breaking change? Also, why would me implying that it's part of the As for flag name suggestions, if Side Note: I'm interpreting this from a more surface level understanding of the concepts, since this level of discussion is generally outside my realm of expertise. But I felt the need to contribute since, I just recently encountered a use-case where not having this flag could prove to be dangerous/fragile. Because I don't have a means of gracefully handling this risk other than hoping the dev reads the jsdocs on the function in question. Edit: Oh I think I may have figured it out, my initial interpretation was that |
"strict" enables a variety of flags, some prefixed with strict. It doesn't enable all safety/correctness features ("noUncheckedIndexedAccess", "useDefineForClassFields") |
|
The Perhaps we might need |
|
@jakebailey would it be much effort to rebase and pack this branch? I briefly tried to resolve the conflicts but felt I didn't have enough context around the changes. |
|
@HansBrende you could have proposed that edit yourself. I did so here. |
|
Is there something currently blocking this PR? This would be an amazing feature to have — I often have to refactor codebases that use a lot of mutability to try and reduce overall mutations, and sadly the current behavior of |
|
Apparently I have gaslit myself. For years, I thought this was already enforced behavior and recently discovered it wasn't. It's hard to believe that this is valid in typescript: interface ImmutablePoint {
readonly x: number;
readonly y: number;
}
class FixedPoint {
get x() { return 5; }
get y() { return 10; }
}
function move(mutablePoint: { x: number, y: number }) {
mutablePoint.x += 1;
mutablePoint.y += 1;
}
const point: ImmutablePoint = { x: 1, y: 2 };
move(point);
move(new FixedPoint());Let's get this merged, please. |
|
I too came across this problem, are there any news on this? |
TypeScript is in ~feature freeze until Corsa lands, so we've got to wait at least another year 😢 |
|
Quite unfortunate In the meantime, would this pull request change the behaviour of this code? function f<K extends PropertyKey, T extends { [k in K]: number }>(obj: T, k: K) {
// Type 'number' is not assignable to type 'T[K]'.
// 'number' is assignable to the constraint of type 'T[K]', but 'T[K]' could be instantiated with a different subtype of constraint 'number'.
obj[k] = 1;
return obj[k];
}
const k = f({ a: 1 }, "a");
// ^? const k: numberMaybe I'm wrong, but I think that |
I don't see why it would. There are no
TypeScript's assignability rules act as if all objects are readonly, so uses covariance rules. This makes most mutable objects unsafe as reading does not support contravariance and writing does not support covariance, so only objects of exactly the same type should be allowed. E.g the follow code is broken: const o = { a: 1 }
function set(o: { a: string | number }) {
o.a = 'foo'
}
set(o)
console.log(o.a.toPrecision(2)) // err: o.a.toPrecision is not a function In your example it is analyzing a bit more deeply than normal and is actually behaving correctly. (Removing the generic T and just using To be safe your code needs to specify that The solution would be something along the lines of this: #9252 (expanded to apply to object properties, and with support for exact match only as well as supertype. |
|
What I meant was that it seems that TypeScript assignability rules are based on readonly checks not being strict. |
TS could move to exact match only for mutable objects, and require readonly props for covariant assignability. With a writeonly flag they could allow contravariant props. But this is separate to this change, and could happen without this. With that change and not this ticket you'd be able to assign The above though is unlikely to happen due to the issues it would cause in existing codebases. Calls to libraries that expect objects and only read, that rely on covariant types being allowed (mega common) would break, unless those libs were updated with readonly keywords. A more likely change would be a strict flag that made objects received by functions readonly by default, with a mutable flag to enable writing and stricter assignability, because this would move to issue upstream into the libraries with nothing breaking unless a lib identified that it was writing to an object and needed to add the mutable keyword, and of course downstream projects wouldn't break until they updated those libs. Plus breaks would only occur where writing was actually happening, rather than many false positives. |
This PR introduces a new
--enforceReadonlycompiler option to enforce read-only semantics in type relations. Currently, thereadonlymodifier prohibits assignments to properties so marked, but it still considers areadonlyproperty to be a match for a mutable property in subtyping and assignability type relationships. With this PR,readonlyproperties are required to remainreadonlyacross type relationships:When compiled with
--enforceReadonlythe assignment ofimmutabletomutablebecomes an error because thevalueproperty isreadonlyin the source type but not in the target type.The
--enforceReadonlyoption also validates that derived classes and interfaces don't violate inheritedreadonlyconstraints. For example, an error is reported in the example below because it isn't possible for a derived class to remove mutability from an inherited property (a getter-only declaration is effectivelyreadonly, yet assignments are allowed when treating an instance as the base type).In type relationships involving generic mapped types,
--enforceReadonlyensures that properties of the target type are not more mutable than properties of the source type:The
--enforceReadonlyoption slightly modifies the effect ofas constassertions andconsttype parameters to mean "as const as possible" without violating constraints. For example, the following compiles successfully with--enforceReadonly:whereas the following does not:
Some examples using
consttype parameters:Stricter enforcement of
readonlyhas been debated ever since the modifier was introduced eight years ago in #6532. Our rationale for the current design is outlined here. Given the huge body of existing type definitions that were written without deeper consideration of read-only vs. read-write semantics, it would be a significant breaking change to strictly enforcereadonlysemantics across type relationships. For this reason, the--enforceReadonlycompiler option defaults tofalse. However, by introducing the option now, it becomes possible to gradually update code bases to correctreadonlysemantics in anticipation of the option possibly becoming part of the--strictfamily in the future.Fixes #13347.