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

Design Meeting Notes, 12/10/2021 #47151

Closed
DanielRosenwasser opened this issue Dec 14, 2021 · 10 comments
Closed

Design Meeting Notes, 12/10/2021 #47151

DanielRosenwasser opened this issue Dec 14, 2021 · 10 comments
Labels
Design Notes Notes from our design meetings

Comments

@DanielRosenwasser
Copy link
Member

"Correlated Union Types"

#30581

type NumberRecord = { kind: "n", v: number, f: (v: number) => void };
type StringRecord = { kind: "s", v: string, f: (v: string) => void };
type BooleanRecord = { kind: "b", v: boolean, f: (v: boolean) => void };
type UnionRecord = NumberRecord | StringRecord | BooleanRecord;

function processRecord(record: UnionRecord) {
  record.f(record.v); // error!
 // error msg in TS3.2 and below: can't call union of functions
 // error msg in TS3.3 and up: record.v not assignable to never
}
  • We want to be able to feed the callback with the original value; but now v is a union type and f's parameter type is an intersection; that's not precise for v, and that intersection now only accepts never.

  • People have speculated that maybe we need existential types so that we can say "this function takes whatever type v has" and can feed it through.

  • Can try to re-model this in the form of a re-indexed mapped type.

    interface TypeMap {
        "n": number;
        "s": string;
        "b": boolean;
    }
    
    type UnionRecord<P extends keyof TypeMap> = { [K in P}]: {
            kind: K;
            v: TypeMap[K];
            f: (p: TypeMap[K]) => void;
        }
    }[K];
    
    function processRecord<K extends keyof TypeMap>(record: UnionRecord<K>) {
        record.f(record.v);
    }
  • With some improvements of inference and apparent types, we can enable most of these cases - 2 improvements on the scale of what feel like bug fixes.

    • First is inference to the IIFE-like mapped object pattern from above.
    • Second is how we present the apparent members of certain types when you index into an already-instantiated mapped object type (a "table type") with some generic key.
      • { [P in K]: Foo<P> }[X] => Foo<X>
      • Does that mean that if you aren't using a type that was declared as a mapped type, then we won't get a more precise type?
        • Yes, if you leave off types on these objects.
          • I guess we don't have an easy way to correlate.
          • We could try to do something there...but that's a bit of a stretch.
  • This is a weird pattern. Would like to not have to teach people to write this.

Modules Migration Status Update

#46567

  • A lot of time running tests.
  • A lot of time is going into getters for every single binding.
  • Had TypeScript generate to CJS, had esbuild run on top of that.
  • Swapped TS to generate ESM, and esbuild generates the same sorts of bindings - so no optimization there.
  • 25 minutes to run the full tests! Thanks to the multiple levels of indirections of calling functions.
  • Could switch to ES3 emit to avoid accessors.
  • Also could switch to native ESM emit.
    • No telling how slow ESM live bindings would be.
  • Could avoid indirect barrel imports.
  • Looked at bundler front-ends that are largely esbuild wrappers.
    • But these tend to push for the optimized build to use Rollup.
  • [[Whole discussion on what the right shipping format and layout to send.]]
  • Dogfooding with modules showed us just how slow this is.
  • Conclusion
    • Investigate the __createBindings issue.
    • Investigate alternative bundlers.

Ensuring Modules Conform to a Type

#38511

  • Lots of people would like to be able to ensure "this file has this sort of shape."
    • Then "I would like this folder to describe this shape".
  • export implements SomeType that
  • PR suggests everything should be contextually typed.
  • You can emulate this today with a self-import.
    • Without contextual typing, this is possibly just sugar that saves you 2 lines. Maybe not as compelling.
  • This links to typing default exports - is that the real feature?
  • Out of time - revisit in the new year.
@DanielRosenwasser DanielRosenwasser added the Design Notes Notes from our design meetings label Dec 14, 2021
@DanielRosenwasser
Copy link
Member Author

PR up for correlated union types is up #47109

@ShuiRuTian
Copy link
Contributor

What is the difference between #46567 and #35561?

@orta
Copy link
Contributor

orta commented Dec 15, 2021

#46567 builds on #35561

@evanw
Copy link
Contributor

evanw commented Dec 21, 2021

Hello! I'm the developer behind esbuild. I noticed that you're running the TypeScript compiler through esbuild, so I couldn't help trying it out myself. I've been meaning to do cross-module enum inlining so I did a quick implementation and tried it out on the branch in #46567. Here's a simple benchmark of the result:

lib/tsc.js esbuild 0.14.6 esbuild 0.14.7 fixes + esbuild 0.14.6 fixes + esbuild 0.14.7
Type check time 2.96s 5.31s (1.79x) 4.94s (1.67x) 3.45s (1.17x) 2.95s (1.00x)

These tests measure the time to type check the Rollup code base (with fixes for type errors due to the TypeScript upgrade). As you can see, bundling with esbuild after inlining enums results in a TypeScript compiler build that's the same speed as the one in lib/tsc.js.

But I had to fix a bug in that branch that was slowing things down a lot (the "fixes" columns in the table):

diff --git a/src/compiler/corePublic.ts b/src/compiler/corePublic.ts
index 11c5416b54..4334edad7f 100644
--- a/src/compiler/corePublic.ts
+++ b/src/compiler/corePublic.ts
@@ -138,6 +138,7 @@ namespace NativeCollections {
     export function tryGetNativeMap(): MapConstructor | undefined {
         // Internet Explorer's Map doesn't support iteration, so don't use it.
         // eslint-disable-next-line no-in-operator
+        const Map = globalThis.Map as any;
         return typeof Map !== "undefined" && "entries" in Map.prototype && new Map([[0, 0]]).size === 1 ? Map : undefined;
     }
 
@@ -147,6 +148,7 @@ namespace NativeCollections {
     export function tryGetNativeSet(): SetConstructor | undefined {
         // Internet Explorer's Set doesn't support iteration, so don't use it.
         // eslint-disable-next-line no-in-operator
+        const Set = globalThis.Set as any;
         return typeof Set !== "undefined" && "entries" in Set.prototype && new Set([0]).size === 1 ? Set : undefined;
     }
 }

This code was picking up on the local versions of Map and Set instead of the global versions, so it was always installing the shim version and never using the native version. Using the native Map and Set is a 50% speedup. I assume you'll figure this out too if you haven't already, but I thought I'd save you the trouble in case it's helpful.

Anyway just wanted to share this result and bug fix. It's totally up to you whether or not you use esbuild, so no pressure. I imagine there are concerns about wanting to dogfood the TypeScript compiler. I was actually surprised to see you trying out esbuild at all.

Edit: Oh yeah, also I shipped cross-module enum inlining in esbuild version 0.14.7 if you want to try it out.

@DanielRosenwasser
Copy link
Member Author

@evanw you're a life-saver.

@elibarzilay can you experiment with the above? The only catch is that we don't always have globalThis defined in older JS versions, so I think you'll have to try something like

const localGlobal =
    typeof globalThis !== "undefined" ? globalThis :
    typeof global !== "undefined" ? global :
    typeof window !== "undefined" ? window;
const Map = localGlobal.Map as any;

@DanielRosenwasser
Copy link
Member Author

I think one other thing to consider is that this is something the conversion script kept working, but silently wrong. Seems like nested namespaces and places where we use local declares might be something to watch out for.

@elibarzilay
Copy link
Contributor

@evanw Thanks for that and sorry that I got to it so late. I'm guessing that you were running esbuild directly on the TS sources--? I'm using it with the CJS output of the compiler, and that has some differences in runtime -- specifically, my concern was the time it takes to run the full set of TS tests... First, I didn't see any change when bumping the version (which is probably expected, given that it's the cjs output).

Second, the Map/Set bug didn't actually affect the cjs output since it doesn't create a global but uses exports.Map directly -- I did try at some point to run esbuild directly on the ts sources which was very slow, so I'm wondering if that could have caused some of the cost (especially if comes on top of the getter cost). In any case, it is definitely a bug, and I just opened #47409 for that.

@evanw
Copy link
Contributor

evanw commented Jan 13, 2022

I'm guessing that you were running esbuild directly on the TS sources--?

Yes, that's correct. I did that because you need to run esbuild on the TS sources to take advantage of TS features such as enum inlining, so that's the fastest way to run the code.

@mohsen1
Copy link
Contributor

mohsen1 commented Jan 18, 2022

RE: #38511

You can emulate this today with a self-import

Can you explain this with an example please? I don't fully understand how importing self can achieve the same thing as proposed.

This links to typing default exports - is that the real feature?

Apparently export default: Type <thing> syntax was introducing so much syntax ambiguity that this request has been stalled (#3792). With export implements we can type the default export without limitations mentioned there:

export implements { default: number };
export default 1

@DanielRosenwasser
Copy link
Member Author

Can you explain this with an example please? I don't fully understand how importing self can achieve the same thing as proposed.

In theory you can write

import * as self from "./self.js"
const _: ExpectedType = self;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Notes Notes from our design meetings
Projects
None yet
Development

No branches or pull requests

6 participants