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

Adding to getKeys blocklist #38

Open
brettz9 opened this issue Mar 28, 2022 · 4 comments
Open

Adding to getKeys blocklist #38

brettz9 opened this issue Mar 28, 2022 · 4 comments
Labels
Projects

Comments

@brettz9
Copy link
Contributor

brettz9 commented Mar 28, 2022

As per the points raised in #36 by @mdjermanovic , getKeys might be ignoring the following additional properties, with the items in parentheses being relative to ESLint but may call for a major version bump if considering other users:

  1. type (perf)
  2. range (from BaseNodeWithoutComments) (perf)
  3. loc (from SourceLocation) (perf)
  4. comments and innerComments (from Comment) (fix since may be mistakenly traversed)

User-facing impact

It is possible some custom AST could be using those properties as legitimate keys, might somehow be (ab)using getKeys to not expect the above properties to be excluded, or might have a traversal function which doesn't ignore those properties when iterating through them.

What is the suggested fix?

However, from the perspective of ESLint, and I think reasonable expectations at least in a major bump, these shouldn't be allowed for traversal given that some are uniformly known not to need traversal given their fixed, special meaning (the perf-labeled items).

For comments and innerComments the case is even stronger to exclude because these will be traversed if present, but as per our prior exclusion of leadingComments and trailingComments, the expectation seems to be not to traverse Comment nodes even if present.

Our new tool to get keys from the TypeScript AST at least does not allow these when building our own key list (by excluding any which lead to Line or Block types), but other tools do not have access to this algorithm when using getKeys().

@brettz9 brettz9 added the bug label Mar 28, 2022
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Mar 28, 2022
@nzakas nzakas changed the title Adding to getKeys blacklist Adding to getKeys blocklist Mar 29, 2022
@nzakas
Copy link
Member

nzakas commented Mar 29, 2022

Can you expand on this? What is the user-facing impact? What is the suggested fix?

@nzakas nzakas moved this from Needs Triage to Triaging in Triage Mar 29, 2022
@brettz9
Copy link
Contributor Author

brettz9 commented Mar 29, 2022

Can you expand on this? What is the user-facing impact? What is the suggested fix?

I've amended the original message.

@nzakas
Copy link
Member

nzakas commented Mar 30, 2022

Thanks! 👍

@nzakas nzakas moved this from Triaging to Feedback Needed in Triage Mar 30, 2022
@fasttime
Copy link
Member

What exactly is innerComments? I can't find any properties with that name in ESTree or in the eslint org.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Feedback Needed
Triage
Feedback Needed
Development

No branches or pull requests

3 participants