Skip to content

Add no-accessor-recursion rule #2525

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

Merged
merged 59 commits into from
Jan 24, 2025

Conversation

axetroy
Copy link
Contributor

@axetroy axetroy commented Jan 15, 2025

Close #1052

@axetroy axetroy marked this pull request as draft January 15, 2025 16:12
@axetroy axetroy marked this pull request as ready for review January 16, 2025 12:44
@axetroy

This comment was marked as resolved.

@@ -0,0 +1,74 @@
# Disallow recursive access to this within getter and setter
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
# Disallow recursive access to this within getter and setter
# Disallow recursive access to `this` within getters and setters

meta: {
type: 'problem',
docs: {
description: 'Disallow recursive access to this within getter and setter',
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
description: 'Disallow recursive access to this within getter and setter',
description: 'Disallow recursive access to `this` within getters and setters.',

@sindresorhus
Copy link
Owner

Can you rename the test file from .mjs to .js?

@axetroy axetroy force-pushed the no_accessor_recursion branch from 533092e to cd67a7c Compare January 20, 2025 02:26
@sindresorhus
Copy link
Owner

You need to regenerate the snapshots npx ava -u.

@axetroy

This comment was marked as resolved.

@axetroy
Copy link
Contributor Author

axetroy commented Jan 21, 2025

There are missing cases for setters, please give a shoot, if it's hard to fix we can ignore.

class Foo{
	set bar() {
		[this.bar] = array;
	}
}
class Foo{
	set bar() {
		({propery: this.bar} = object);
	}
}
class Foo{
	set bar() {
		Object.assign(this, {bar: 1})
	}
}

But this case for getters is common, we should report

class Foo{
	get bar() {
		const {bar} = this;
	}
}

The last case is already supported and tested, the other cases I don't think anyone will write that way.

And they are difficult to detect, Maybe we should ignore it

@param {import('estree').VariableDeclarator} parent
@param {import('estree').Property | import('estree').MethodDefinition} property
*/
const isRecursiveDestructuringAccess = (parent, property) => parent.type === 'VariableDeclarator' && parent.id.type === 'ObjectPattern' && parent.id.properties.some(declaratorProperty => declaratorProperty.type === 'Property' && isSameKey(declaratorProperty.key, property.key));
Copy link
Collaborator

@fisker fisker Jan 21, 2025

Choose a reason for hiding this comment

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

Not checking shorthand(Not needed), and computed.

@fisker fisker self-assigned this Jan 22, 2025
@fisker
Copy link
Collaborator

fisker commented Jan 22, 2025

@axetroy Handled more cases a17cef9 (#2525), please take a look.

@@ -240,5 +240,20 @@ test.snapshot({
}
}
`,
//
...[
'++ this.bar;',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@fisker fisker self-requested a review January 22, 2025 08:16
Copy link
Collaborator

@fisker fisker left a comment

Choose a reason for hiding this comment

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

Do you know how this works in class static initialization blocks? Can you check if we need logic adjustments?

@fisker
Copy link
Collaborator

fisker commented Jan 22, 2025

Alright, I found

The this inside a static block refers to the constructor object of the class.

So, I think we need some updates to getClosestFunctionScope.

const foo = {
	get bar() {
		class Foo {
			static {
				this.bar // <- This should not reported.
			}
		}
	}
}

Copy link
Collaborator

@fisker fisker left a comment

Choose a reason for hiding this comment

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

Good job!

@sindresorhus sindresorhus merged commit 92b5671 into sindresorhus:main Jan 24, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule proposal: no-accessor-recursion
3 participants