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.

Sign up for GitHub

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

Jump to bottom

feat(linter): noMisleadingCharacterClass #909

Merged
merged 21 commits into from
Dec 4, 2023
Merged

feat(linter): noMisleadingCharacterClass #909

merged 21 commits into from
Dec 4, 2023

Conversation

togami2864
Copy link
Contributor

@togami2864 togami2864 commented Nov 27, 2023 •
edited
Loading

Summary

close: #52
implement no-misleading-character-class

This rule isn't 100% compatible with ESLint because it requires an ECMAScript regex parser for certain cases.

Test Plan

add snaps for valid/invalid cases

@github-actions github-actions bot added A-Project Area: project A-Linter Area: linter A-Website Area: website L-JavaScript Language: JavaScript and super languages A-Diagnostic Area: diagnostocis labels Nov 27, 2023
@togami2864 togami2864 force-pushed the feat/no-misleading-character-class branch from 38493ce to 11e307b Compare November 27, 2023 14:58
@TaKO8Ki
Copy link
Contributor

TaKO8Ki commented Nov 27, 2023

@togami2864 Could you execute just codegen and commit/push changes?

@unvalley
Copy link
Member

unvalley commented Nov 27, 2023 •
edited
Loading

@togami2864 cc: @ematipico
About regex crate, when I implemented some lint rules about regex, I haven't used that because of the crate size. But my implementation became a bit complex referencing below:

ref:

We may not have to be so particular now. I'd like anyone to share any opinions.
I found rust-analyzer also doesn't use regex crate, this post is published in 2021 mentioned that.

@TaKO8Ki
Copy link
Contributor

TaKO8Ki commented Nov 27, 2023

Oh, this is a WIP pull request. I see.

@nhedger nhedger changed the title Feat(lint): noMisleadingCharacterClass feat(linter): noMisleadingCharacterClass Nov 27, 2023
@togami2864 togami2864 force-pushed the feat/no-misleading-character-class branch from 11e307b to 0e69ea1 Compare December 2, 2023 06:18
@netlify Netlify
Copy link

netlify bot commented Dec 2, 2023 •
edited
Loading

✅ Deploy Preview for rad-torte-839a59 canceled.

Name Link
🔨 Latest commit 72099e0
🔍 Latest deploy log https://app.netlify.com/sites/rad-torte-839a59/deploys/656df19517718300083072eb

@togami2864 togami2864 marked this pull request as ready for review December 2, 2023 06:30
Conaclos
Conaclos reviewed Dec 2, 2023
View reviewed changes
Copy link
Member

@Conaclos Conaclos left a comment

Choose a reason for hiding this comment

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

As a first look, this looks good! I need more time to review your PR, I was nnot expecting that it also handle nons-static regex. I just left a first comment for the docs.

crates/biome_js_analyze/src/semantic_analyzers/nursery/no_misleading_character_class.rs Outdated Show resolved Hide resolved
ematipico
ematipico approved these changes Dec 4, 2023
View reviewed changes
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

I left some suggestions, and the usage of global_identifier for the globalThis.

I think we should add more test cases:

  • window.RegExp
  • globalThis.globalThis

I leave the decision to you @togami2864 , this is a nursery rule, so you can decide make a second PR after we merge this one. Let us know what you want to do and we're happy to help.

crates/biome_js_analyze/src/semantic_analyzers/nursery/no_misleading_character_class.rs Outdated Show resolved Hide resolved
crates/biome_js_analyze/src/semantic_analyzers/nursery/no_misleading_character_class.rs Outdated Show resolved Hide resolved
crates/biome_js_analyze/src/semantic_analyzers/nursery/no_misleading_character_class.rs Outdated Show resolved Hide resolved
crates/biome_js_analyze/src/semantic_analyzers/nursery/no_misleading_character_class.rs
Comment on lines +135 to +140
let is_global_this = match callee.object().ok()? {
AnyJsExpression::JsIdentifierExpression(e) => {
e.name().ok()?.has_name("globalThis")
}
_ => false,
};
Copy link
Member

Choose a reason for hiding this comment

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

We have an utility called global_identifier for these cases, and you use this one. That's needed because it handles recursive cases of global identifiers, like this:

globalThis.globalThis.globalThis.RegExp

@togami2864
Copy link
Contributor Author

@ematipico
Thank you for your suggestion!! I'll make a follow-up PR to cover those additional cases.

@ematipico
Copy link
Member

ematipico commented Dec 4, 2023 •
edited
Loading

Thank you @togami2864 ! When you merge this PR, please make sure to open a new task (issue) to track your work :)

@ematipico ematipico merged commit 3b0689e into biomejs:main Dec 4, 2023
16 of 17 checks passed
@togami2864 togami2864 deleted the feat/no-misleading-character-class branch December 4, 2023 16:27
@togami2864 togami2864 mentioned this pull request Dec 9, 2023
Closed
@Conaclos Conaclos added the A-Changelog Area: changelog label Jan 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@unvalley unvalley unvalley left review comments

@Conaclos Conaclos Conaclos left review comments

@ematipico ematipico ematipico approved these changes

Assignees
No one assigned
Labels
A-Changelog Area: changelog A-Diagnostic Area: diagnostocis A-Linter Area: linter A-Project Area: project A-Website Area: website L-JavaScript Language: JavaScript and super languages
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

📎 Implement lint/noMisleadingClassInRegex - eslint/no-misleading-character-class
5 participants
@togami2864 @TaKO8Ki @unvalley @ematipico @Conaclos