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

Ast typegen #114

Merged
merged 34 commits into from
Feb 14, 2024
Merged

Ast typegen #114

merged 34 commits into from
Feb 14, 2024

Conversation

H-Plus-Time
Copy link
Contributor

@H-Plus-Time H-Plus-Time commented Feb 5, 2024

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

Problematic types so far:

  • WithComment. Presumably because of the manual serde derives, tsify isn't picking up on it.
  • PropertyKey - uses associated types, will need overriding, or an Omit typedef.
  • ArrayDestructuringField - associated types, fortunately just the one variant.
  • ObjectDestructuringField - associated types, ditto.
  • Marker - custom typescript section not taking effect
  • ClassDeclaration - custom typescript section not taking effect.
  • PublicOrPrivate
  • Private
  • AlwaysPublic

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

@kaleidawave kaleidawave added the js-wasm-unplugin-and-playground The JS library, Unplugin NPM packages and playground label Feb 6, 2024
@kaleidawave kaleidawave linked an issue Feb 6, 2024 that may be closed by this pull request
@kaleidawave
Copy link
Owner

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
Copy link
Contributor Author

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 🤔 .

😛 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 😒.

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
Copy link
Contributor Author

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.

…ro) of tsify in checker (used rarely enough in that crate not to need the bundle macro), definition of bundle macro and demo usage in parser crate.
…erive triplet, restricted to only those enums/structs that do not use either of PartialEqExtras or GetFieldByType
@H-Plus-Time H-Plus-Time marked this pull request as ready for review February 6, 2024 14:06
@H-Plus-Time
Copy link
Contributor Author

:| 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 😭 ). 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
Copy link
Owner

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 kaleidawave/source-map#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
Copy link
Contributor Author

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
Copy link
Owner

kaleidawave commented Feb 10, 2024

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
Copy link
Owner

kaleidawave commented Feb 10, 2024

  • So I decided to include the manual definitions in parser/lib.rs (it is definitely something weird with the comments.rs file and and other files, as the custom_typescript_definition didn't work in there when I tested locally but also possibly caused because all are generic?). (I should try to figure out a minimum repro to file somewhere). And it was working locally 🎉 and checked with TSC 🎉. And now it is broken in CI saying there is duplicate definition, which means it was generated the first time? 🥴🥴🥴, I have no idea
    • I should also try figure out the clash you mentioned (I wrote those macros, so I might be able to figure out if I go back down the cargo-expand rabbit hole again)
  • I renamed the derive bundle as I saw the comment 👍! Unfortunately I can't call it ASTNode as it clashes with the trait :( so derive_ASTNode it is.
  • clippy and rustfmt are also broken as it seems there was a new Rust release last week!

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

@kaleidawave
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Owner

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 😅

…ebug and release (locally, nuking cargo cache between runs). Fingers crossed for CI 😧
@H-Plus-Time
Copy link
Contributor Author

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 🤷.

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
Copy link
Owner

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:

ezno/parser/src/lib.rs

Lines 269 to 293 in 15bfa43

// TODO temp?
// These are here because for some reason **under debug builds** these definitions are missing.
// It has something to do with the files as these definitions only work here :?. Experimentation needed
#[cfg_attr(
all(target_family = "wasm", debug_assertions),
wasm_bindgen::prelude::wasm_bindgen(typescript_custom_section)
)]
const TYPES_THAT_ARE_MISSING: &str = "
export type WithComment<T> =
| { None: T }
| { PrefixComment: [string, T, Span] }
| { PostfixComment: [T, string, Span] };
// shouldn't be useful to p need
export type Marker<T> = null;
export interface ClassDeclaration<T> {
name: T,
type_parameters?: Array<GenericTypeConstraint>,
extends?: Expression,
implements?: Array<TypeAnnotation>,
members: Array<Decorated<ClassMember>>,
position: Span,
}
";

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
Copy link
Owner

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! 🎉

@kaleidawave kaleidawave merged commit 2c0f6e7 into kaleidawave:main Feb 14, 2024
6 of 8 checks passed
@kaleidawave kaleidawave mentioned this pull request Feb 21, 2024
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
js-wasm-unplugin-and-playground The JS library, Unplugin NPM packages and playground
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typescript definition for wasm target
2 participants