-
-
Notifications
You must be signed in to change notification settings - Fork 724
feat(linter): implement eslint-rule dot-notation #15957
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
base: main
Are you sure you want to change the base?
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
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. |
| "(let?.true)", | ||
| r#"(let?.["true"])"#, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I needed to add parenthesis here. Otherwise, the parser doesn't accept it as I checked in the AST Explorer.
d84c5c2 to
9684761
Compare
| fn is_valid_identifier(s: &str) -> bool { | ||
| let mut chars = s.bytes(); | ||
| chars.next().is_some_and(|c| | ||
| /* a-zA-Z_$ */ c.is_ascii_alphabetic() || c == b'_' || c == b'$') | ||
| && chars.all(|c| /* a-zA-Z0-9_$ */ c.is_ascii_alphanumeric() || c == b'_' || c == b'$') | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Incorrect validation of identifiers with Unicode characters
The is_valid_identifier function only validates ASCII characters, but JavaScript identifiers can include Unicode letters. This will incorrectly reject valid Unicode identifiers.
For example, these are valid in JavaScript but would be rejected:
foo["你好"] // Chinese characters are valid identifiers
foo["café"] // é is a valid identifier characterThe function should use proper Unicode identifier validation according to the JavaScript specification, or leverage existing identifier validation utilities from the AST parser.
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
| return; | ||
| } | ||
| if let Some(allow_pattern) = &self.allow_pattern { | ||
| if Regex::new(allow_pattern).unwrap().is_match(&value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical: Potential panic on invalid user-provided regex
The code calls Regex::new(allow_pattern).unwrap() where allow_pattern comes from user configuration. If a user provides an invalid regex pattern, this will panic in production.
if let Some(allow_pattern) = &self.allow_pattern {
if let Ok(regex) = Regex::new(allow_pattern) {
if regex.is_match(&value) {
return;
}
}
// Or handle the error appropriately
}Alternatively, validate and compile the regex once during configuration parsing in from_configuration() and store the compiled regex in the config struct.
| if Regex::new(allow_pattern).unwrap().is_match(&value) { | |
| if let Ok(regex) = Regex::new(allow_pattern) { | |
| if regex.is_match(&value) { |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
CodSpeed Performance ReportMerging #15957 will not alter performanceComparing Summary
Footnotes
|
ff9779a to
a6dcab3
Compare
|
@camc314 @connorshea Thank you for the support so far. |
| ("08['prop']", "08 .prop", None), | ||
| ("090['prop']", "090 .prop", None), | ||
| ("018['prop']", "018 .prop", None), | ||
| ("5_000['prop']", "5_000 .prop", None), | ||
| ("5_000_00['prop']", "5_000_00 .prop", None), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these results actually intended to work/do they work?
| #[schemars(rename_all = "camelCase")] | ||
| pub struct DotNotationConfig { | ||
| allow_keywords: bool, | ||
| allow_pattern: Option<Regex>, | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want to tweak this a bit to make sure the docs that get output are correct and have descriptions for these two config options:
| #[schemars(rename_all = "camelCase")] | |
| pub struct DotNotationConfig { | |
| allow_keywords: bool, | |
| allow_pattern: Option<Regex>, | |
| } | |
| #[serde(rename_all = "camelCase", default)] | |
| pub struct DotNotationConfig { | |
| /// TODO: Describe this option | |
| allow_keywords: bool, | |
| /// TODO: Describe this option | |
| #[serde(with = "Regex")] | |
| allow_pattern: Option<Regex>, | |
| } |
The default thing just ensures that the JSON Schema emits a default value. #[serde(with = "Regex")] will ensure it gets a type value, otherwise I believe it just sets the type value to null and is not displayed. It may need to be schemars(with = "Regex") if serde doesn't work on that.
You can check what the generated docs look like by running cargo run -p website_linter rules --rule-docs target/rule-docs --git-ref $(git rev-parse HEAD) (or look at just website in the justfile and run that), and the generated docs for this rule will end up in target/rule-docs/eslint/dot-notation.md.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah also you will need to rebase the branch to ensure that website command works, it was changed in the last few days
Add ESLint rule
dot-notation.I took some inspiration and some code snippets from #9344.
I also implemented the fixer taking inspiration from https://github.com/oxc-project/oxc/blob/main/crates/oxc_minifier/src/peephole/convert_to_dotted_properties.rs as as suggested by @Boshen in #9344 and from the original ESLint implementation.