Skip to content

Enhancement: Tests that visitor keys visit fields in source code order #11282

Open
@overlookmotel

Description

@overlookmotel

Before You File a Proposal Please Confirm You Have Done The Following...

Relevant Package

visitor-keys

My proposal is suitable for this project

  • I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Description

A comment in visitor-keys package states that keys "should be sorted in the order that they appear in the source code".

/*
********************************** IMPORTANT NOTE ********************************
* *
* The key arrays should be sorted in the order in which you would want to visit *
* the child keys. *
* *
* DO NOT SORT THEM ALPHABETICALLY! *
* *
* They should be sorted in the order that they appear in the source code. *
* For example: *
* *
* class Foo extends Bar { prop: 1 } *
* ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ClassDeclaration *
* ^^^ id ^^^ superClass *
* ^^^^^^^^^^^ body *
* *
* It would be incorrect to provide the visitor keys ['body', 'id', 'superClass'] *
* because the body comes AFTER everything else in the source code. *
* Instead the correct ordering would be ['id', 'superClass', 'body']. *
* *
**********************************************************************************
*/

#11276 related to 5 types where this was not the case (fixed in #11279).

The field orders for standard ESTree types are inherited from eslint-visitor-keys, which are outside of TS-ESLint's control.

After #11279 and eslint/js#656, visitor keys for all types are in source code order, as intended, but there are no tests for this. It would be ideal to have tests, to ensure this rule doesn't get broken again unintentionally in future.

Additional Info

I'm not sure how such tests would be implemented. Ideally you'd probably want to codegen them so they automatically cover any new AST nodes added in future, but then you'd need to run those tests on a large corpus of test fixtures to make sure they cover all syntax. But a vast corpus might make those tests extremely slow to run, so maybe it'd be something of a sledgehammer to crack a nut situation.

Metadata

Metadata

Assignees

No one assigned

    Labels

    accepting prsGo ahead, send a pull request that resolves this issueenhancementNew feature or requestrepo maintenancethings to do with maintenance of the repo, and not with code/docs

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions