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

feat(linter): add eslint/dot-notation #9344

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ammubhave
Copy link
Contributor

Copy link

graphite-app bot commented Feb 24, 2025

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

@github-actions github-actions bot added A-linter Area - Linter C-enhancement Category - New feature or request labels Feb 24, 2025
@ammubhave ammubhave changed the title feat(linter): add eslint/dot-notation feat(linter): add eslint/dot-notation Feb 24, 2025
@Sysix
Copy link
Collaborator

Sysix commented Feb 25, 2025

I think there are some possible improvements. One is: instead of VALID_IDENTIFER: Regex there can be a simple char parser.
@camchenry @shulaoda you guys have more knowledge about performance in rust

@Sysix Sysix requested review from camchenry and shulaoda February 25, 2025 22:47
@shulaoda
Copy link
Member

I think there are some possible improvements. One is: instead of VALID_IDENTIFER: Regex there can be a simple char parser.

Perhaps we can take inspiration from rolldown/rolldown#3677. I’ll review it later.

Copy link

codspeed-hq bot commented Feb 25, 2025

CodSpeed Performance Report

Merging #9344 will not alter performance

Comparing ammubhave:dot-notation (d8858e2) with main (44fd2c4)

Summary

✅ 33 untouched benchmarks

Copy link
Contributor

@camchenry camchenry left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I've made a few suggestions to help this rule better fit the style of our codebase as well as perform better.

);

/// List of ES3 keywords.
const KEYWORDS: &[&str] = &[
Copy link
Contributor

Choose a reason for hiding this comment

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

given the size of this, I'd recommend using hash set instead. here's an example: https://github.com/oxc-project/oxc/blob/main/crates/oxc_linter/src/rules/eslint/valid_typeof.rs#L141

Copy link
Contributor Author

@ammubhave ammubhave Feb 26, 2025

Choose a reason for hiding this comment

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

good point, I was curious so I ran benchmarks for linear search vs using match (which i suspect might be doing a binary search) and using phf_set.

test tests::bench_linear            ... bench:          18.53 ns/iter (+/- 2.30)
test tests::bench_match             ... bench:           2.01 ns/iter (+/- 0.13)
test tests::bench_set               ... bench:          10.67 ns/iter (+/- 0.89)

I changed the implementation to a is_keyword function that uses match.

btw, for the valid_typeof link that you mentioned above, I ran the same benchmark with that list of valid types. looks like the set lookup takes significantly longer in that case presumably because the number of elements is much smaller. recommend we change that implementation to a matches! as well.

test tests::bench_valid_type_linear ... bench:           0.29 ns/iter (+/- 0.03)
test tests::bench_valid_type_match  ... bench:           0.28 ns/iter (+/- 0.03)
test tests::bench_valid_type_set    ... bench:          10.81 ns/iter (+/- 1.01)

ref: https://gist.github.com/ammubhave/ca9f531f0e3760616f664f4230da7983

Copy link
Member

@Boshen Boshen left a comment

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linter Area - Linter C-enhancement Category - New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants