feat(linter): add unicorn/no-accessor-recursion rule #9971
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. |
github-actions
bot
added
A-linter
huangtiandi1999
changed the title
CodSpeed Instrumentation Performance ReportMerging #9971 will not alter performanceComparing Summary
|
|
hi @Sysix, this job is finish, please help to review |
| fn get_closest_function<'a>(node: &AstNode, ctx: &'a LintContext) -> Option<&'a AstNode<'a>> { | ||
| let mut parent = ctx.nodes().parent_node(node.id())?; | ||
|
|
||
| loop { |
There was a problem hiding this comment.
can you rewrite it, to not use a loop and instead use something like while Some(parent) = ctx.nodes()...
There was a problem hiding this comment.
if it's possible, I'd even recommend using ctx.nodes().ancestor_kinds(). here is an example elsewhere, where we do something similar: https://github.com/oxc-project/oxc/blob/main/crates/oxc_linter/src/rules/eslint/no_new_func.rs#L66-L72
| if !is_parent_property_or_method_def(func, ctx) { | ||
| return; | ||
| } | ||
| let Some(key_name) = get_property_or_method_def_name(func, ctx) else { | ||
| return; | ||
| }; |
There was a problem hiding this comment.
is_parent_property_or_method_def and get_property_or_method_def_name lookup up the parent from ctx.
I would prefer to get the parent in this method and avoiding a ctx passtrough :)
| impl Rule for NoAccessorRecursion { | ||
| fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { | ||
| match node.kind() { | ||
| AstKind::VariableDeclarator(decl) => { |
There was a problem hiding this comment.
VariableDeclarator and MemberExpression are the core structure of any JS program.
Maybe we can get more performance when we check for ThisExpression and its possible valid parent.
What do you think @camchenry
There was a problem hiding this comment.
I think it's a good idea. @huangtiandi1999 to put it more concretely: try restructuring the rule so that it only searches for AstKind::ThisExpression. Then, look up/down the tree as needed to match the cases. It might require a bit of restructuring, but it should be a lot faster, as we'll be able to skip evaluating lots of nodes that are common like @Sysix pointed out.
| OxcDiagnostic::warn(format!("Disallow recursive access to `this` within {kind}.")) | ||
| .with_label(span) |
There was a problem hiding this comment.
Every rule should have at least a short help message, to help users understand how to fix it the issue. How about something like this:
| OxcDiagnostic::warn(format!("Disallow recursive access to `this` within {kind}.")) | |
| .with_label(span) | |
| let method_kind = match kind { | |
| "setters" => "set", | |
| _ => "get", | |
| }; | |
| OxcDiagnostic::warn(format!("Disallow recursive access to `this` within {kind}.")) | |
| .with_help("Remove this property access, or remove `{method_kind}` from the method") | |
| .with_label(span) |
| impl Rule for NoAccessorRecursion { | ||
| fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { | ||
| match node.kind() { | ||
| AstKind::VariableDeclarator(decl) => { |
There was a problem hiding this comment.
I think it's a good idea. @huangtiandi1999 to put it more concretely: try restructuring the rule so that it only searches for AstKind::ThisExpression. Then, look up/down the tree as needed to match the cases. It might require a bit of restructuring, but it should be a lot faster, as we'll be able to skip evaluating lots of nodes that are common like @Sysix pointed out.
| fn get_closest_function<'a>(node: &AstNode, ctx: &'a LintContext) -> Option<&'a AstNode<'a>> { | ||
| let mut parent = ctx.nodes().parent_node(node.id())?; | ||
|
|
||
| loop { |
There was a problem hiding this comment.
if it's possible, I'd even recommend using ctx.nodes().ancestor_kinds(). here is an example elsewhere, where we do something similar: https://github.com/oxc-project/oxc/blob/main/crates/oxc_linter/src/rules/eslint/no_new_func.rs#L66-L72
| let exist = obj_pattern | ||
| .properties | ||
| .iter() | ||
| .find(|ident| ident.key.name().is_some_and(|name| name == key_name)); | ||
| if exist.is_some() { |
There was a problem hiding this comment.
| let exist = obj_pattern | |
| .properties | |
| .iter() | |
| .find(|ident| ident.key.name().is_some_and(|name| name == key_name)); | |
| if exist.is_some() { | |
| let exists = obj_pattern | |
| .properties | |
| .iter() | |
| .any(|ident| ident.key.name().is_some_and(|name| name == key_name)); | |
| if exists { |
| AstKind::VariableDeclarator(decl) => { | ||
| // Here we deal with deconstructive access to this inside getter | ||
| // e.g. "const { baz } = this" | ||
| if let Some(init) = &decl.init { |
There was a problem hiding this comment.
I'd suggest doing an early return here, to remove a level of indentation:
| if let Some(init) = &decl.init { | |
| let Some(init) = &decl.init else { | |
| return; | |
| } |
Relates to #684
Rule detail：https://github.com/sindresorhus/eslint-plugin-unicorn/blob/v57.0.0/docs/rules/no-accessor-recursion.md