Type Node missing properties

[hydroflame]

In acorn.d.ts the class Node can have (among other fields) the field source. This isn't listed anywhere in the d.ts. Making this library very hard to use with typescript.

[hydroflame]

It's missing so much more than that.

[Tanimodori]

There're already issues on it (#946 #741). I'd like to see Nodes with subtypes.

[Tanimodori]

The quick fix by RReverser is here. Although as of acorn 8.8.0 and TS 4.7.4, the correct fix is like

// acorn.d.ts
import { Options } from 'acorn';
import * as ESTree from 'estree';

declare module 'acorn' {
  type ExtendNode<T> = {
    [K in keyof T]: T[K] extends object ? ExtendNode<T[K]> : T[K];
  } & (T extends ESTree.Node
    ? {
        start: number;
        end: number;
      }
    : unknown);

  export function parse(s: string, o: Options): ExtendNode<ESTree.Program>;
}

Changelog:

• unknown is used as null element instead of {}
• Add acorn's Options in parse signature.

Edit: There are some problems with fix memtioned above:

declare module 'acorn' {
  // ... (fix code omitted) ...

  // Problem with Union over non-Node types
  type ID = ExtendNode<ESTree.ClassExpression>['id'];
  // -> ESTree.Identifier | null | undefined
  // Expect: ExtendNode<ESTree.Identifier> | null | undefined
  type Elements = ExtendNode<ESTree.ArrayPattern>['elements'];
  // -> (ESTree.Pattern | null)[]
  // Expect: Array<ExtendNode<ESTree.Pattern> | null>

  // Problem with Comments
  type CommentType = Comment['type'];
  // -> string
  type NonNull<T> = T extends null | undefined ? never : T;
  type ESComments = NonNull<ExtendNode<ESTree.Program>['comments']>;
  type TestStartEnd = ESComments[number] extends { start: number; end: number } ? true : false;
  // -> false
  // The two types of Comment are incompatible with eath other
}

So the correct fix is like:

// acorn.d.ts
import * as ESTree from 'estree';

declare module 'acorn' {
  type ExtendObject<T> = {
    [K in keyof T]: ExtendNode<T[K]>;
  };
  type WithStartEnd<T> = T extends ESTree.Node | ESTree.Comment
    ? { start: number; end: number }
    : unknown;
  export type ExtendNode<T> = T extends object ? ExtendObject<T> & WithStartEnd<T> : T;
  export function parse(s: string, o: Options): ExtendNode<ESTree.Program>;

  // fix type of Comment property 'type'
  export type AcornComment = Omit<Comment, 'type'> & {
    type: 'Line' | 'Block';
  };
}

Now the ExtendNode<T> will distribute over T[K]:

declare module 'acorn' {
  // ... (fix code omitted) ...

  // Problem with Union over non-Node types
  type ID = ExtendNode<ESTree.ClassExpression>['id'];
  // -> ExtendNode<ESTree.Identifier> | null | undefined
  type Elements = ExtendNode<ESTree.ArrayPattern>['elements'];
  // -> Array<ExtendNode<ESTree.Pattern> | null>

  // Problem with Comments
  type NonNull<T> = T extends null | undefined ? never : T;
  type ESComments = NonNull<ExtendNode<ESTree.Program>['comments']>;
  type TestStartEnd = ESComments[number] extends { start: number; end: number } ? true : false;
  // -> true
  type TestCompability = AcornComment extends ESComments[number] ? true : false;
  // -> true
  // AcornComment is now subtype of ExtendNode<ESTree.Comment>
}

Example use case:

import { parse, type AcornComment, type ExtendNode } from 'acorn';
export const parseCode = (code: string) => {
  const comments: AcornComment[] = [];
  const program = parse(code, {
    ecmaVersion: 'latest',
    sourceType: 'module',
    onComment: comments,
  });
  program.comments = comments;
  return program;
};

export const dumpImportDeclarations = (code: string) => {
  const program = parseCode(code);
  const result: Array<ExtendNode<ImportDeclaration>> = [];
  for (const node of program.body) {
    if (node.type === 'ImportDeclaration') {
      result.push(node);
    }
  }
  return result;
};

P.S. If you do not care about messing with estree's interfaces, you can do like:

import * as ESTree from 'estree';

declare module 'acorn' {
  export function parse(s: string, o: Options): ESTree.Program;

  // fix type of Comment property 'type'
  export type AcornComment = Omit<Comment, 'type'> & {
    type: 'Line' | 'Block';
  };
}

declare module 'estree' {
  interface BaseNodeWithoutComments {
    start: number;
    end: number;
  }
}

Which will also fix the problem, as long as your code does not depend on properties of estree's nodes.

[Tanimodori]

The type of 'type' property of Comment issue is fixed in #1140

[rwalle]

I am considering putting some work in the near future in creating a real .d.ts file with all strict typing and doing it properly, i.e. with tests. I may need to add typescript as a dev dependency. Any interest or thoughts?

[marijnh]

I haven't thought deeply about what such types would look like, but I'd gladly review a more complete set of types. If at all possible, try to write them in a way that reuses the ESTree types, so that we don't have to duplicate all those.

[rwalle]

I haven't thought deeply about what such types would look like, but I'd gladly review a more complete set of types. If at all possible, try to write them in a way that reuses the ESTree types, so that we don't have to duplicate all those.

I spent some time looking into this, and I feel it is easier to copy & paste the estree types instead of try to programmatically reuse them in some way, unless I missed something. For example, I have been doing this for a while:

import * as estree from 'estree/index'

interface AcornBaseNode {
  start: number
  end: number
}

type ProgramBase = Omit<estree.Program, 'leadingComments' | 'trailingComments' | 'body' | 'comments'> & AcornBaseNode
interface Program extends ProgramBase {
  body: Array<Directive | Statement | ModuleDeclaration>
}

Basically I am overriding the body field while removing all the comment fields that don't exist in acorn results. However, considering that Statement and ModuleDeclaration also are recursive or have fields that are recursively in some way, in order to ensure that all the types are self-consistent, I basically need to remove comments and re-add any fields of recursive types for each of Literal, all sub types of Statement, all sub types of Expression etc. The result is not so different from just copying & pasting estree type and modify BaseNode to match acorn's base node data. In other words, after I make "Program" node have start and end fields, all its children also need to have them, recursively.

I noticed that @types/estree is different from the es2022 data under formal folder, but that is the same if I want to have start and end fields in all nodes.

[marijnh]

I spent some time looking into this, and I feel it is easier to copy & paste the estree types instead of try to programmatically reuse them in some way

That makes sense, yeah. It's a shame, since it is quite a lot of types we'd have to maintain, and it means people can't just take an Acorn tree and pass it to some tool that is typed to take ESTree nodes, but if they didn't explicitly set it up in a way that allows extension (and even added non-standard stuff like comment properties) then reusing it may just not be practical.

[rwalle]

Thanks! I have two more questions about handling options:

• should separate typing files be created for each ecma version? I noticed that the interface of nodes depends on the ecma version which may affect downstream user. It is probably possible to return the exact ast for a given ecma version, and estree goes back to es5 -- although I have absolutely no idea what to do with es3. This would mean that several .d.ts files would need to be maintained, although unless there is a bug fix it is unlikely those files ever need to be updated. Otherwise, if we only maintain one version, it means that downstream user may have to do something like callExpression.optional! (or other workarounds) even though they can be certain optional exists for their ecma version.
• for the same reason, should the output base on locations and ranges option etc? This may be doable using intersection types and condition types, but will make maintenance more complicated in any case

Basically the questions are around how loose the types should be wrt parse options.

Thanks!

[marijnh]

should separate typing files be created for each ecma version? 

That sounds like way too much work an complexity. Just make the types reflect the tree you get the most recent version, I'd say.

I'd make location and ranges optional properties.

[rwalle]

I created an early version of the work in a repository:

https://github.com/rwalle/acorn-types

Want to know if this looks like a good start, and how to proceed from here.

I tried using these files in one of my projects that heavily uses acorn and so far it seems working fine, even helped to find some bugs.

I added some comments in the parse options and for acorn-walk methods. They should be handy.

Some thoughts:

• declaring the acorn namespace does not seem correct to me, so I changed it to declare module 'acorn'. This is also consistent with the current walk.d.ts
• Where should the code live? in acornjs/acorn, a separate repository or DefinitelyTyped? If tests are used and run (probably a good idea), the repository would need to depend on typescript, and it is a bit weird to make acornjs/acorn depend on typescript.
• I found a number of issues in the existing code in acorn:
  • The Position interface in the current type definition has a property offset but I don't think it ever exists
  • There seems to be a typo in options.js -- It should be "the position of the inserted semicolon as an offset". Also the comment next to onComment in this file does not mention that it can also take an array, seems to be out of sync
  • README.md code highlighting for Comment interface of onComment should be typescript instead of javascript
  • acorn-walk README could use a bit more documentation. e.g. I discovered that visitors could also have keys like Expression and Statement which handles all sub-types but was not (explicitly) mentioned.
  • It seems strange to me that the type of state in the current walk.d.ts is the generic type or Node[] -- not sure why this is necessary when state is already arbitrary, and I cannot find code that indicates this
  • Current type in the acorn-walk "visitor" like [type: string]: SimpleWalkerFn<TState> is too loose and not very usable, so I expanded them to include all possible properties. I did not figure out a way not to repeat them though
  • -- I can create GitHub issues for these
• thoughts on using eslint? currently it is not well formatted. This is also related to the second bullet point, as this needs typescript-specific rules

There may be a few more things but I don't remember them now. I can come back with additional items.

Thanks

[marijnh]

Those look promising! Instead of abusing this issue to become a messy review of it, I'm going to open issues on your repository with concerns I have.

As for weirdness in the current types, yes, those are somewhat shoddy, and it would be great to replace them wholesale. If the types look wrong, they probably are.

I've updated the comments in options.js. Writing better docs for acorn-walk would definitely be good, but I don't really have time for it at the moment.

[marijnh]

Where should the code live? in acornjs/acorn, a separate repository or DefinitelyTyped? If tests are used and run (probably a good idea), the repository would need to depend on typescript, and it is a bit weird to make acornjs/acorn depend on typescript.

I think we'll want to bundle these with the package itself, for ease of use (and to not have to go through DefinitelyTyped every time we find an issue). But maybe it does make sense to develop (and test) them in their own, separate repository, so that we don't add too much machinery for it to this one.

[marijnh]

Merged the new, much better types in attached patch.

[marijnh]

@RReverser In case you want to take a look at these before a release, I'm going to wait a few days before I tag a new version.

[RReverser]

Thanks, I will. It's unfortunate it's not based on ESTree after all, but good to see progress unblocked.