-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Strawperson TypeScript integration for Record & Tuple #49243
Comments
In addition to the above. There is also downlevel transform to think of. I think this can be split into 3 potential levels of support: Downlevel support L0aka - no support. Any record/tuple syntax is directly preserved in the output. This is the same as the current support for // input
let rt = #{ a: #[1, 2, 3] };
// output
let rt = #{ a: #[1, 2, 3] }; Downlevel support L1aka - bring your own. The literal syntax is removed and replaced with the equivalent constructor calls. The equivalent level of support for Users of this mode would either need to be sure that their environment has these globals available, or install a polyfill themselves. This mode may also be useful where, even though the target environment has R/T support, other parts of their downstream toolchain may not yet have support for parsing the syntax (e.g. the bundler or minifier). // input
let rt = #{ a: #[1, 2, 3] };
// output
let rt = Record({ a: Tuple(1, 2, 3) }); Downlevel support L2aka - built-in polyfill. This mode would not only apply the L1 transform, but also include the necessary supporting library. Any R/T polyfill will have the caveats of:
// input
let rt = #{ a: #[1, 2, 3] };
// output
import { __Record, __Tuple } from 'tslib';
let rt = __Record({ a: __Tuple(1, 2, 3) }); |
Here's some top-of-mind feedback right now Intersection TaggingThe It does also feel like it's a bit of a step backwards in what we're expecting from users. Needing Narrowing BehaviorGiven that more kinds of types can be described by all- interface Opts {
readonly volume?: number;
readonly hue?: number;
}
function setHair(opts: Opts | number | string) {
let volume;
let hue;
if (typeof opts === "object") {
volume = opts.volume ?? 47;
hue = opts.hue ?? 0;
}
else {
// In this branch, 'opts' can still have the type 'Opts'
// because it might be a 'record'.
volume = typeof opts === "string" ? +opts : opts;
hue = 0;
}
if (typeof volume !== "number" || !Number.isFinite(volume)) {
throw new Error();
}
if (typeof hue !== "number" || !Number.isFinite(hue)) {
throw new Error();
}
// ...
}
// No call site errors, but won't work at run time.
setHair(#{ })
You could argue that the right way to write this narrowing code would be to check for the primitive types first and then assume an object-ish type in the final branch; however, someone has written code out there like this. Anyone who hasn't might, and often won't know better until they've run into this issue. I guess that that's going to be the case with any addition of a new primitive - existing code may stop working correctly, and new code may just forget to account for the new primitive. But when records are intended to be substitutable in enough places where objects are, there's some risk there. I also think a |
That narrowing behavior seems like a dealbreaker - it is exceedingly common and idiomatic for |
Ugh, the way I wrote it up was probably a mental lapse and I should clarify - the positive case will continue to narrow. In the second branch, we may still have to assume that Original text was
New text is now
|
ah gotcha, makes sense |
Thank you @DanielRosenwasser for the feedback on this. If I put my TS integrator at Bloomberg hat on; we do prefer Before I go into more details on how we could potentially solve those issues, I want to separate what choices are made in that strawperson TS proposal and why and what are core choices to the proposal. At the proposal level: at the moment we define R&T as primitives, that indeed means that Now on the TS level we are imagining the same experience as we intend to make with WebIDL, meaning that a Record can be used where an Object is accepted and a Tuple where an Array is accepted. Previous mutability exampleinterface Foo {
bar: string;
}
function anythingCanHappenToFoo(aFoo: Foo) {
// ...
aFoo.bar = "I can change anything!";
// ...
}
anythingCanHappenToFoo({ bar: "hello" }); // Typechecks OK, Runtime OK
anythingCanHappenToFoo(#{ bar: "hello" }); // Typechecks OK, Runtime TypeError
anythingCanHappenToFoo(Object.freeze({ bar: "hello" })); // Typechecks OK, Runtime TypeError In this scenario, TS could let you write code that would hit a runtime error with R&T. This is a trade-off and we understand if this could be problematic. With this angle in mind, let me go back to the issues you raised and see if we can mitigate them or even solve them... Intersection TaggingThe TS strawperson proposal is gradual: the tagging, For Interface extensionThis was suggested to me by @nicolo-ribaudo. Today it errors, but we could imagine tagging via the interface Foo extends object { ... }
interface Bar extends record { ... } This is only a proposition here, this might go against how the typechecker is designed today. More syntaxNow if we want to go another way, it would be possible to add more syntax out there to mark a record interface: interface Bar #{ ... } A downside to this is that it the inverse wouldn't have dedicated syntax - there wouldn't be a way to say the interface describes Objects and not Records. Narrowing BehaviourThis second part is more complex and is one that may require more research to help find the most appropriate solution. That said I do have some initial thoughts. To me this problem is very similar to the mutability problem I described earlier: when is it ok to let people write unsafe code and have the compiler ignore it? The 'unsound' option: continue to model
|
{} is βobject coercibleβ, which includes numbers, so i wouldnβt expect anything to hit the number branch. |
That isn't what happens today. |
ah, that's confusing, i assumed |
@rricard Thanks for the thorough and thoughtful analysis. I wanted to get a better understanding of the current behavior. I found that apparently it matters whether we're talking about On the other hand, if you have anything inside the braces, it's a different story. I went with In both cases, pay attention to the narrowed type in the Overall, I prefer the sound option (as well as finding some way to stricten the readonly/mutability handling). I'd be interested to see how this plays out in Google's codebase. Is there a patch we could run through our global presubmit to see how much breaks? |
Having a way to test both the stricter readonly and the stricter object assignability would indeed be super interesting. I have been looking at collecting data on typeof sites and it is difficult to just do that without the compiler insights. Testing out the potential breakages would be way more accurate. |
We (Bloomberg) now have some preliminary results for attempting to measure the narrowing Approach:
Patches: Both patches additionally add patch-simpler: https://github.com/bloomberg/TypeScript/tree/ac/tsc-4-7-record-type-spike-simple The simpler version of the patch does not filter out object types in a function getLengthOrSize(v: string | Set<unknown>): number {
if (typeof v === "object") {
return v.size; // OK v is Set
} else {
return v.length; // Now an error even though a record wouldn't be assignable to Set
}
} patch-complex: https://github.com/bloomberg/TypeScript/tree/ac/tsc-4-7-record-type-spike This more complex patch attempts to filter out false positives by also checking that the type is compatible with a new internal type Primitive = null | undefined | boolean | number | bigint | symbol | string | esRecord;
type esRecord = { readonly [p: string]: Primitive }; function getLengthOrSize1(v: string | Set<unknown>): number {
if (typeof v === "object") {
return v.size; // OK: v is Set
} else {
return v.length; // OK: v is string. Set is not assignable to esRecord
}
}
function getLengthOrSize2(v: string | { size: number }): number {
if (typeof v === "object") {
return v.size; // OK v is Set
} else {
return v.length; // Error: `{ size: number }` is assignable to esRecord
}
} This patch does add non-trivial complexity compared to the simpler patch. It also introduces some false negatives for when the compatibility is contravariant: function getLengthOrSize3(v: string | { size: (number | (() => number)) }): number {
if (typeof v === "object") {
return typeof v.size === "function" ? v.size() : v.size;
} else {
// @ts-expect-error: should error but does not
return v.length; // OK. v is string. { size: (number | (() => number)) } is not assignable to esRecord
}
}
getLengthOrSize3(#{ size: 1 }); // is assignable to `{ size: (number | (() => number)) }` This patch did also have a stack overflow on two of our projects which I haven't had a chance to investigate yet. So while its results may be interesting, they are possibly of less note that the simpler patch. Results: We ran this across ~700 internal TypeScript projects, with a total Simple patch: 12 type errors across 7 projects |
I tried both patches on Google's codebase. This ran ~400k projects. Here are the results:
Seems like a very small number of error overall. Also seems in line with what you found, @acutmore. |
thanks for trying out the patches @frigus02, that is much appreciated and really useful additional data.
That's OK. I think we can either take the results of the simple-patch, or assume the worst-case for the complex-patch and consider the projects that crashed as ones that would have type-errored anyway. Thanks again! |
Strawperson TypeScript integration for Record & Tuple
π Search Terms
β Viability Checklist
My suggestion meets these guidelines:
π Motivating Example
We can use and type records to be used as a coordinate system:
π» Use Cases
This change will not only make TS compliant with ECMA262 once Record & Tuple lands in the spec but it will add type representations that will make Record & Tuple even more ergonomic to use.
High quality TS support for Record & Tuple is essential for it to be widely adopted. The feature is more useful if it is backed by a good type system.
Strongly-typed immutable data structures aid writing correct and reliable programs, so Record & Tuple in TypeScript is an excellent fit!
β Suggestion
The champion group for Record & Tuple is going to ask for Stage 3 soon and we want to have a solid TS integration plan before going forward with Stage 3.
The goal of this issue is to propose a strawperson mechanism in TS to handle the proposed Record & Tuple.
This is based on what @rbuckton suggested in the proposal's repo.
Step 0: Disambiguate existing TS Records & TS Tuples
Before any work is made into integrating Record & Tuple into TS, it would be useful to disambiguate existing TS Records & TS Tuples from ES Records & ES Tuples. This is unfortunately going to be a source of confusion and it might be useful to start going ahead of this problem.
The
Record<K,V>
utility typeAs TS, like JS, is staying backwards compatible,
Record<K,V>
is here to stay and is not going anywhere.We would like to however make it an alias of another more precisely named type and then softly deprecate
Record
:Dictionary
was sugggested to us by @rauschma in a R&T issue: tc39/proposal-record-tuple#82 (comment)Naming is fully up for debate.
The TS Tuples
TS Tuples being a syntactic construct and not a named type, it will be easier to work around in the language but also harder to change across the board as both the TS documentation and hundreds of blog articles have adopted that terminology.
The disambiguating terminology could be "Fixed Array" as sugggested to us by @rauschma in a R&T issue: tc39/proposal-record-tuple#82 (comment).
Naming is fully up for debate.
Step 1:
record
&tuple
primitive typesNow comes the question of integrating ES Records & ES Tuples into TypeScript without introducing completely new semantics to TS.
Set of all Records or Tuples
We would introduce new
record
andtuple
primitive types in TS. Those match the set of all records and all tuples respectively:This would match the JS runtime behavior here since those are the
typeof
returns for Record & Tuple:Marker
record
primitive typeThose primitive types would act as markers on types like any other primitive type:
Using that principle we can get a type for a specific given record:
This will also let Records match normal interfaces:
Marker
tuple
primitive typeWe've seen records but tuples are similar:
Finally we can also represent arbitrarily sized tuples:
Record & Tuple Object constructor & wrapper types
We also need to define the following, we disambiguate with the
Wrapper
suffix:Step 2: Type syntax for Record & Tuple
To keep the feature modular to implement, we can land separately the following syntax to represent Record & Tuple:
This is not required to properly support R&T but it should be widely used if implemented.
Complementary work:
strictReadonly
The problem
Because we designed Record & Tuple to integrate well with existing JS (and therefore TS) code, we cannot make R&Ts incompatible with existing interfaces even if they are lacking
readonly
markers on their properties.This has the negative consequence of TS not erroring on the following code:
Note that the same problem already happens today with frozen objects.
We also think that not letting people plug in Records or Tuples into types that should match but are mutable would be a greater problem for Record & Tuple adoption and basically make them an "island feature" in TS that couldn't work with existing code.
Immutability by default, even for objects:
strictReadonly
The good news is that mutating object arguments are already quite a bad practice and a source of silent logic errors so they are not recommended anyway. Most reasonably modern code avoids the practice already.
Others thought of that, notably in #13347 (comment), making all fields
readonly
unless opted out by a modifier via astrictReadonly
option:And at this point,
mutable
orwritable
modifiers would make interfaces reject R&Ts:Many questions need to be answered as part of this (should this
mutable
property be "viral" to other interfaces? ...) so that is why this should be considered a parallel proposal and be untied to R&T.This also might be quite a sizable feature so keeping it separate to adding a new ECMA262 feature is beneficial.
This issue was kindly reviewed by @acutmore, @dragomirtitian and @robpalme. It also contains suggestions from @acutmore, @dragomirtitian and @rauschma.
The text was updated successfully, but these errors were encountered: