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

chore: Derive keys from AST definitions #35

Closed
wants to merge 16 commits into from

Conversation

brettz9
Copy link
Contributor

@brettz9 brettz9 commented Feb 10, 2022

Per eslint/espree#529 (comment) , this PR aims to build the visitor keys out of AST definitions.

Note that I added two initial commits to alphabetize the keys for easier comparisons. You can therefore probably most easily see the current impact of what the changes would be here by looking at the diffs from after that sorting:

brettz9/eslint-visitor-keys@0071631...brettz9:ast-defs-0

Also note that I added json-diff to the devDeps. as the Mocha JSON diffs left something to be desired, with sometimes not showing what the differences were.

I also still have some minor clean-up (choosing whether to use the added chai plugin based on whether alphabetizing of keys is ok, removing logging, and adding full coverage for the build tool which currently only tests lines needed by the current AST).

I'm not fully certain of what the results should be here, however, so I believe some tweaking will be in order.

  1. I'm currently ignoring the following nodes from getting their own keys, even though they are defined as interfaces, the normal criterion I used to determine whether an item were to get its own keys: Comment, Position, and SourceLocation. (I could ignore non-exported interfaces as that would get rid of SourceLocation (and only that), but not the other two.) Let me know which if any of these should in fact have keys, and/or if others need to be ignored.
  2. I'm currently avoiding treating the following as keys: comments, innerComments, type, and operator. Let me know if any of these should instead be allowed. In getKeys, only parent, leadingComments, and trailingComments are currently ignored. If any of the new keys should also be ignored by getKeys, please let me know which ones.
  3. Most significantly, I wasn't fully sure on what basis the need for keys could be determined. It seemed clear that if an interface property only referenced the following, it could be treated as non-traverseable:
    "TSUndefinedKeyword",
    "TSNullKeyword",
    "TSBooleanKeyword",
    "TSNumberKeyword",
    "TSStringKeyword",
    "TSBigIntKeyword",
    "TSLiteralType"

Just to be safe some of the remainder should not be traverseable, I gathered a listing of all the remaining non-excluded nodes which I did therefore treat as traversable:

     "Array",
    "CatchClause",
    "ChainElement",
    "ClassBody",
    "Declaration",
    "Expression",
    "FunctionExpression",
    "Identifier",
    "JSXClosingFragment",
    "JSXIdentifier",
    "JSXMemberExpression",
    "JSXOpeningElement",
    "JSXOpeningFragment",
    "JSXClosingElement",
    "Literal",
    "Pattern",
    "TemplateLiteral",
    "VariableDeclaration"

Let me know if any of these should not be treated as traverseable for purposes of determining that properties possessing such a type should get their own key (assuming the results demonstrate that this algorithm is reasonable and sufficient in the first place).

@brettz9 brettz9 added the chore label Feb 10, 2022
@eslint-github-bot
Copy link

Hi @brettz9!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
  • There should be a space following the initial tag and colon, for example 'feat: Message'.
  • The first letter of the tag should be in lowercase

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

@brettz9 brettz9 changed the title Derive AST definition chore: Derive AST definition Feb 10, 2022
@brettz9 brettz9 changed the title chore: Derive AST definition chore: Derive keys from AST definitions Feb 10, 2022
CatchClause: [
"param",
"body"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order is important here as it indicates which property should be traversed first. As such, we can change the order to alphabetical. (Same comment throughout this file.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, sure, understood. But if the AST definers can't be convinced to define the properties in the order of iteration (taking into account nested interfaces, etc.), we'd have to add our own logic on how to sort these (and it might not be correct once new properties are added).

@@ -0,0 +1,43 @@
import { diffString } from "json-diff";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a file header comment describing what this file does?

And separators between major sections?

See https://github.com/eslint/eslintrc/blob/main/lib/cascading-config-array-factory.js as an example.

(Same for other files.)

Comment on lines +248 to +251
SimpleCallExpression: [
"arguments",
"callee"
],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nodes with type: "SimpleCallExpression" don't exist, but nodes with type: "CallExpression" do, and we need those here. When generating this object, we should probably use values of type properties instead of names of interfaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this approach:

  1. This should cause SimpleLiteral, BigIntLiteral, and RegExpLiteral to be dropped from the new output, as apparently was the case earlier and I imagine still expected now (and merging isn't a problem for this script as they don't have any non-primitives to traverse).
  2. Directive and ExpressionStatement present a similar situation, causing Directive to now be dropped as I think was expected, and though both have their own traversable property, it is the same name, expression (as makes sense).
  3. Comment has two types Line and Block though I can make two types out of that (or ignore both).

I imagine there may be not too much work to adapt this to use type, and can then see if can get the expected results (minus the ordering of the keys which seems may not be as easy to do programmatically based solely on the AST).

@brettz9 brettz9 closed this Feb 11, 2022
@brettz9 brettz9 deleted the ast-defs-0 branch February 11, 2022 10:54
@brettz9
Copy link
Contributor Author

brettz9 commented Feb 11, 2022

Apologies, plan to resubmit another PR and link back to this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants