kaleidawave / ezno

A JavaScript compiler and TypeScript checker written in Rust with a focus on static analysis and runtime performance
https://kaleidawave.github.io/posts/introducing-ezno/
MIT License
2.44k stars 45 forks source link

Ast typegen #114

Closed H-Plus-Time closed 7 months ago

H-Plus-Time commented 8 months ago

NB: macro-free for the moment (self-rust-tokenize was not happy).

Problematic types so far:

In general, everything that uses an associated type needs a bit of adjustment.

kaleidawave commented 8 months ago

Awesome, looking good!

Don't have to worry about Marker its equivalent to a u16 or whatever, the type bounds are just for some tricks, it doesn't have any properties.

With WithComment I think I manually made serde just serialize the inner. So it's type should be just equivalent to T.

Got a bit panicked at the diff count but realised most of it is src/js-cli-and-library/package-lock.json 😄

Did you manage to get to macro_rules_attribute working?

I wonder why tsify doesn't work well with associate generics? I would think it could do where T:A: Tsify and T:A.tsify() etc?

H-Plus-Time commented 8 months ago

Yeah, the Marker, WithComment and ClassDeclaration ones were the oddest thing - no matter what I did, debug builds would not pick up on typescript custom section produced in either of those three files, but release builds did :thinking: .

:stuck_out_tongue: well, the file count is still a little alarming.

re macro_rules_attribute, I think I'll get the wasm_bindings types fixed up then backtrack to swapping out the simpler ones (especially now I have a type-error free benchmark to compare against). I'm circumspect re how simple the SelfRustTokenize usage will have to be for the macro to work, but we'll see.

I wonder why tsify doesn't work well with associate generics? I would think it could do where T:A: Tsify and T:A.tsify() etc?

Mm, it would have to do some kind of de-genericizing to achieve it, so for something like:

struct Foo<T: KindTrait> {
   pub baz: T,
   pub bar: T::VariableType
}

struct Foobar {
  pub inner: u32
}
impl KindTrait for Foobar {
  type VariableType = u32;
}
fn main() {
  let outer: Foo<Foobar> = Foo { baz: Foobar { inner: 0 }, bar: 0 };
}

because you can't fully express the trait (especially the associated type) in TS, the closest you could get is:

type Foo<T, T_VariableType> = {
  baz: T;
  bar: T_VariableType;
}
type Foobar = {
  inner: number;
}
// impossible to encapsulate within the parent type
type Foobar_VariableType = number;
let instance: Foo<Foobar, Foobar_VariableType> = {
  baz: { inner: 0 },
  bar: 0
};

The current approaches (tsify, ts-rs, typeshare, probably bridge.rs too) usually do either of: a. Notice it's inexpressible without cascading associate expansion, and panic. b. Accidentally strip the parent (so instances of T::VariableType convert to VariableType), failing to hoist the associated type :unamused:.

We're quite fortunate that it's only FunctionBase and PropertyKey (with ~6 variants for FunctionBase, 2 trivially overridden variants for PropertyKey).

H-Plus-Time commented 8 months ago

One thing - is there a combination of flags/params/methods that works for .d.ts files, or is that automatic?

I've played around a bit with running ezno on the generated typedefs - ignoring the export function complaints, there's a few that have me puzzled (e.g. type Foo = | number |string is, while ugly, fairly common).

For the meantime, I've added a quick tsc call to the wasm part of the CI.

H-Plus-Time commented 7 months ago

:| it looks like the fuzz CI jobs have run afoul of the stdsimd issue that's crept into a number of dependencies (this one's in ahash) as of nightly 1.78.0 (released yesterday :sob: ). I could get them to sort of run locally (well, with SIGSEGVs on the main branch :-/), post updating to that nightly, you get the unknown feature 'stdsimd' error.

kaleidawave commented 7 months ago

Awesome! Don't worry about fuzzing (those tests were broken already and will hopefully be fixed in #111).

In the Rust->extras workflow is it possible to use the debug profile to build? The --release flag is a bit overkill and slows down the CI time (it only needs to test, not using this build for benchmarks).

I am assuming the "can't find type Span" issue in extras is because https://github.com/kaleidawave/source-map/pull/3 needs merging and releasing first?

I've played around a bit with running ezno on the generated typedefs - ignoring the export function complaints, there's a few that have me puzzled (e.g. type Foo = | number |string is, while ugly, fairly common).

Interesting! Did it crash at all (could you put the output in a gist)? I think at the moment because promises and most of the standard library isn't there (e.g. WebAssembly.Module) it won't be correct at all. There are also some issues around multiple .d.ts files when involving the standard library. So perfectly fine to use TSC for now.


Everything else looks great. I am a bit busy at the moment but at the end of the week will merge and release the source map PR and then I will update this. I think this repo will get an release next week so these type definitions will be included 🎉

H-Plus-Time commented 7 months ago

Interesting! Did it crash at all (could you put the output in a gist)?

Yep, I chipped away at the output until it parsed completely and came up with ~3-4 areas that throw parse errors. Here's the gist: https://gist.github.com/H-Plus-Time/fef7b91f2742fc0aed3dc6722756bb22

Re the release build - yeah, this one's necessary to avoid the loss of Marker, WithComment and ClassDeclaration in the debug build (there's something about the files those three are in, or how they're used in the parser crate that makes them vanish in debug, but reappear in release). Agreed it's not great that it slows down CI, though I'm still at a loss as to how the debug build drops those macro outputs (and only for those structs/enums).

kaleidawave commented 7 months ago

Ah interesting. I didn't know | string | number that was valid TS syntax. Will have to fix that, thanks for the comment.

Have just updated the source-map crate. Will have a look at the PR later, update versions and merge.

kaleidawave commented 7 months ago

I will fix and merge when my patience has regenerated 😅 I am excited to get this in the next release!

kaleidawave commented 7 months ago

Had an idea of how Tsify could support ADTs / the TS it could generate

interface PropertyKeyAssociatedTypes extends Record<any, any> {}

interface PropertyKey_<T extends keyof PropertyKeyAssociatedTypes> {
    name: PropertyKeyAssociatedTypes[T]["name"]
}

// Example 1 of what could be generated for a impl with ADTs
declare const x: unique symbol;
type PropertyKeyWithX = PropertyKey_<typeof x>;
// Override
interface PropertyKeyAssociatedTypes {
    [x]: { name:string }
}

// Another example
declare const y: unique symbol;
type PropertyKeyWithY = PropertyKey_<typeof y>;
// Override
interface PropertyKeyAssociatedTypes {
    [y]: { name: undefined }
}

type Example1 = PropertyKeyWithX["name"]
type Example2 = PropertyKeyWithY["name"]
H-Plus-Time commented 7 months ago

Ah, thanks for the mass-rename, and the fix for the macro incompat (it was seriously just invocation order? I really should have checked that).

possibly caused because all are generic

You know, that might actually be it - I hadn't thought to check for similar files with only generics (just ones that had at least one generic, explicit serialize impl).

kaleidawave commented 7 months ago

Yeah I was trying it and looks like order has an effect. It should technically work, so must be a bug/edge-case in Rust macro expansion. That apply macro is really useful, thanks for showing me that! Wish I knew about it when I was added serde support and things!

kaleidawave commented 7 months ago

This is strange, somehow the Marker and ClassDeclaration issue is fixed on the remote/CI (I don't think I touched anything to fix it) but I still get it locally (on my Windows machine, latest Rust (same as CI, should be the same dependencies). Maybe a cache issue? Still on both remote and my machine WithComment is missing on debug? I have added this temp fix in for debug builds:

https://github.com/h-plus-time/ezno/blob/cb6a4d27aa0100b5d76294e674c4efcbf31ca609/parser/src/lib.rs#L269-L294

I assume it is still the case from your previous comment and the declaration file is okay for the release build that is published to NPM.

It seems like an external issue, do you think it is okay to merge now or do you want to do some more experimentation in this PR? I am out of ideas and I don't know a lot about how wasm-bindgen works 😅

H-Plus-Time commented 7 months ago

Yeah, I think if it works on my most recent commit in CI, we're good (I've run cargo clean between the npm run build and npm run build-release steps just to make absolutely certain). It's likely a weird cache thing in CI that's causing the magical self-fixes.

As for the underlying issue, yeah, agreed, it's likely some external heisenbug buried within tsify or wasm-bindgen :shrug:.

I reckon it's good to merge - imho, it's the basis of a useful test feedback loop (simultaneously a. predictable (as opposed to testing against one of the big, well-known public TS projects) b. sufficiently advanced c. digestible), so even a somewhat brittle version is worth it.

kaleidawave commented 7 months ago

Looks like it is a cache issue in CI. Clearing the CI cache has fixed the missing type declarations? Now as they are being correctly generated, the "fix" I added to debug builds now produces duplicates 😅. All this missing vs duplicate, debug vs release, local vs CI has got my head in a spin 😆

So theoretically if I remove this: https://github.com/kaleidawave/ezno/blob/15bfa43122585e99e0b7a64b7139343da58a6592/parser/src/lib.rs#L269-L293 It should work on debug (and I will add a temp test for the release build as well)

As long as it works on release on the CI (which is how it is published) it should be fine for merging. While the local problems are annoying it shouldn't be important for local development for now.

kaleidawave commented 7 months ago

Awesome, debug passes (and although release branch didn't fire, I did test it on my GitHub actions testing repository and it worked, so it should be fine for publishing). I know about the difference so will be looking out for it appearing in CI. Will merge now! Thanks for the addition, will be included in the next blog post! 🎉