Add typescript types to bin

[hildjj]

No description provided.

[hildjj]

The real news here is outputting .d.ts files next to the emitted .js file.

[LukeAbby]

Hey! Already finding this helpful but here's a quick review from a passerby (i.e. not a contributor) after trying this.

[LukeAbby]

Is there any way I could avoid any here?

[hildjj]

It's either that, unknown, or we put in another generic type, which will make the whole thing kind of a mess.

It really can be almost anything, as long as it can be converted to a string with String(source), and it matches the grammarSource that got passed in to parse.

[LukeAbby]

Since you asked for a more full review, yeah I'd suggest unknown or Source here. You said it should match grammarSource hence Source (which I talked about typing in a different comment) if that even makes sense.

[LukeAbby]

You asked for my full review. I happen to write libraries with extremely pedantic accuracy requirements so a lot of my review may come off as excessive. The main thing I think is helpful is trying to hone down a Source type and in general trying to remove anys. Unfortunately those are the two areas where I had to most guess.

Given the quantity of changes I've attached a patch file with every change I would make to the .d.ts file

review.patch

You can download this file, put it alongside generated_template.d.ts and apply it with git apply review.patch

If you want to put me as author you can with:

git commit --author="LukeAbby <luke@lukeabby.dev>"

But I don't mind that much either way.

[LukeAbby]

The comments say this is an object. If this cannot be trusted then you should update the comment and pursue a different change. I can work with you on this.

---

To start off I see this code block:

    if (offset && peg$source && (typeof peg$source.offset === "function")) {
      res.start = peg$source.offset(res.start);
      res.end = peg$source.offset(res.end);
    }

offset seems to only be true when options.true (within the compiler itself) is true. However this still indicates a shape.

Maybe type it like so?

// A mutable object can be assigned to a readonly object just fine so this really does allow any object.
type AnyObject = {
    // This MUST be `unknown` instead of `any` because `any` allows arrays and functions as well.
    // Functions and arrays are allowed with `any` because both of these types can be indexed by strings successfully.
    // There is no reasonable explanation why `unknown` acts differently. They're literally just special cased differently in indexed types.
    // See https://github.com/microsoft/TypeScript/wiki/Breaking-Changes/83af27fca396d172b4d895d480b10c3bacf89112#-k-string-unknown--is-no-longer-a-wildcard-assignment-target
    readonly [K: string]: unknown;
}

// Based upon your remark "any object" I'm assuming arrays, functions, numbers, etc. are invalid so I typed it as `AnyObject`.
export interface Source extends AnyObject {
    /**
     * This function is used when trace is set to true to do ???
     * TODO: Better documentation. I don't know what's going on here entirely to be honest.
     */
    offset?: (location: Location) => Location
}

export interface LocationRange {
    source: Source;

    ...
}

The inline comments there are for the benefit of explanation I would not leave them in.

---

On the other hand I see some internal code that suggests that it could be a string or a File or anything else. In which case I would suggest

type Source = File | string | {} | null | undefined;

I wrote it this way for intellisense purposes. The type {} actually means "any type that isn't null or undefined" and then I added null and undefined to allow both of those. This is better than File | string | unknown because in intellisense it'd just collapse and show unknown.

Though I bet you probably just want some subset like objects/arrays/so on? For instance you probably don't want a function but I guess I don't know.

[hildjj]

Don't worry about being too pedantic. I indeed did ask for it. :)

Yeah, the comment is wrong. grammarSource is often a string containing the file name, but could also be an object that produces a useful string when String(grammarSource) is applied, e.g. something with a toString() method. They also need to be testable for equality with ===. As such, numbers would work also, as long as they aren't NaN.

[hildjj]

The GrammarLocation class in lib/grammar-location.js is a good example of why you might want an object as the grammarSource.

[hildjj]

And you're right that GrammarLocation has a few methods that the generated parser can use if they are there. They should be documented.

[LukeAbby]

Yeah so my final type; type Source = File | string | {} | null | undefined; in the last comment may be close to what you want. It'll allow literally anything with some intellisense hints about common uses.

But here's another try at it based upon your new constraints:

// I'm sure you can come up with a better name for this
// This will contain all of the methods you mentioned that the parser can use if they're there.
interface ObjectSource {
    ... // Methods go here, you can make them required if you want as `AnyObject` will catch it in.
}

// `AnyObject` is a bit broader than you talked about; there's no guarantee it has a useful `toString` method. You might think that you could fix this with the type `{ toString(): string }` but unfortunately `Object` itself defines a `toString` method that returns `"[object Object]"`
// `
//
// You can probably decide for yourself if `null` and `undefined` makes sense. They most likely make sense to mean "not present" rather than a valid value though.
// It sounds like arrays probably don't make sense here. If they do you could add `readonly unknown[]` to the union to allow any array (readonly or not)
// `unique symbol` - the return of `Symbol(...)`  may not be usable because of the `===`/`toString` constraints?
// `symbol` - the return of `Symbol.for()` may make more sense but still a bit strange to put here.
// It sounds like functions almost certainly don't make sense here.
type GrammarSource = ObjectSource | AnyObject | string | number | bigint;

Notably this new version does NOT just allow anything but I left a comment about everything it omits.

[hildjj]

I've got this:

export interface GrammarSourceObject {
  readonly source?: undefined | string | unknown;
  readonly offset?: undefined | ((loc: Location) => Location);
  readonly toString: () => string;
}

export type GrammarSource = string | GrammarSourceObject | unknown;

That unknown is too broad, and I'd be open to just removing it in favor of the things we know work and are useful.

[LukeAbby]

Alright, what things are known to work and are useful? I can help you figure that out.

[hildjj]

I'm driving across Kansas today, currently charging the car in Maple Hill. Sorry in advance for spotty connectivity.

I think I'm probably the only one who ever uses anything other than a string, and the only other thing I ever use is GrammarLocation, which conforms to GrammarSourceObject. Let's just take the unknown out and let people file bugs when they want more. :)

[LukeAbby]

Since you asked for a more full review, yeah I'd suggest unknown or Source here. You said it should match grammarSource hence Source (which I talked about typing in a different comment) if that even makes sense.

[hildjj]

Given the quantity of changes I've attached a patch file with every change I would make to the .d.ts file

I'm going to edit in all of your suggested changes myself so I can reinforce my learning, if that's ok with you.

[LukeAbby]

I've got no problem with editing by hand! I just didn't want to be overwhelming.

[hildjj]

@LukeAbby can you look at the current version and mark each of our sub-discussions as "resolved" if I've gotten them taken care of? I continue to be open to any other nits you find -- I appreciate your help!

[LukeAbby]

Hey! @hildjj sorry, I guess I wasn't seeing the ping because of how my GitHub notifications are set up. I actually only checked the issue again to see how things are moving. I think my only comment left is about the typing of GrammarSource;

You currently have

export type GrammarSource = string | GrammarSourceObject | unknown;

But like you said, I think we can probably be more specific! If you can give me a list of everything you think it can be then I could write out the type and I think that'd be the last of it.

[hildjj]

I think the Stringable change here captures what's important. Will this work for you?

[LukeAbby]

It probably still catches more than you expect; for example it allows Symbol, number, object, arrays, effectively everything except something like Object.create(null);.

Besides that it looks like it should! 🎉🎉🎉

Great job with the persistence with getting this right, I hope I was of help! If you ever want help again you can ping me and I'd be happy to take a look.

[hildjj]

OK, I'm going to write docs for this, then land it.

[hildjj]

@LukeAbby this should be almost done.

One last thing, can you please change dts: false to dts: true in src/opts.mjs, then run npm build? tsc is getting stuck in an infinite loop trying to deal with the generated .d.ts file. I don't think it's related to the returnTypes; switch those to any and the error still happens. It looks like it's actually trying to infer types from parser.js even though there is a .d.ts file, which is surprising to me.

[LukeAbby]

Hi. Sorry, I got busy so I didn't swing back around to this as fast as I should've. I spent a bit of time reducing down the problem space. You can reproduce with just this program:

var a$b = {};
var c, d;
d = a$b;
while (d !== a$b);
while ((c = a$b != a$b)) c.e;

(Obviously when reduced down to this simple it's absolutely absurd)

Looks like this was actually fixed in this commit: microsoft/TypeScript@71fb864 so presumably the next TypeScript version won't have this problem. You can also downgrade to 5.4 which doesn't have this problem either. I ended up reporting this in this issue: microsoft/TypeScript#59594 but I only asked them to add a regression test since it was already fixed.

[hildjj]

Thanks, @LukeAbby. I think the least-bad option is to keep this PR the way it is, then flip false to true for dts when the next TS rev comes out.