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

fix(eslint-plugin): [prefer-readonly] report if a member's property is reassigned #6043

Merged
merged 6 commits into from Nov 26, 2022
Merged

fix(eslint-plugin): [prefer-readonly] report if a member's property is reassigned #6043

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3d88d10
fix(eslint-plugin): [prefer-readonly] report if a member's property i…
islandryu Nov 19, 2022
94c6b98
format
islandryu Nov 19, 2022
8a147ec
Add test case
islandryu Nov 21, 2022
913d934
fix test case
islandryu Nov 26, 2022
9c1de04
fix test case
islandryu Nov 26, 2022
be983e9
Merge branch 'main' into fix/prefer-readonly-member-property
JoshuaKGoldberg Nov 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion packages/eslint-plugin/src/rules/prefer-readonly.ts
View file
Open in desktop
Expand Up @@ -119,7 +119,10 @@ export default util.createRule<Options, MessageIds>({
ts.isArrayLiteralExpression(parent.parent))
) {
current = parent;
} else if (ts.isBinaryExpression(parent)) {
} else if (
ts.isBinaryExpression(parent) &&
!ts.isPropertyAccessExpression(current)
) {
return (
parent.left === current &&
parent.operatorToken.kind === ts.SyntaxKind.EqualsToken
Expand Down
138 changes: 138 additions & 0 deletions packages/eslint-plugin/tests/rules/prefer-readonly.test.ts
View file
Open in desktop
Expand Up @@ -339,6 +339,34 @@ class Foo {
}
`,
},
{
code: `
class Test {
private testObj = {
prop: '',
};

public test(): void {
this.testObj = '';
}
}
`,
},
{
code: `
class TestObject {
public prop: number;
}

class Test {
private testObj = new TestObject();

public test(): void {
this.testObj = new TestObject();
}
}
`,
},
],
invalid: [
{
Expand Down Expand Up @@ -742,5 +770,115 @@ function ClassWithName<TBase extends new (...args: any[]) => {}>(Base: TBase) {
},
],
},
{
code: `
class Test {
private testObj = {
prop: '',
};

public test(): void {
this.testObj.prop = '';
this.testObj.prop;
this.testObj?.prop;
this.testObj!.prop;
this.testObj!.prop = '';
this.testObj.prop.prop = '';
this.testObj.prop.doesSomething();
this.testObj?.prop.prop;
this.testObj?.prop?.prop;
this.testObj.prop?.prop;
this.testObj!.prop?.prop;
}
}
`,
output: `
class Test {
private readonly testObj = {
prop: '',
};

public test(): void {
this.testObj.prop = '';
this.testObj.prop;
this.testObj?.prop;
this.testObj!.prop;
this.testObj!.prop = '';
this.testObj.prop.prop = '';
this.testObj.prop.doesSomething();
this.testObj?.prop.prop;
this.testObj?.prop?.prop;
this.testObj.prop?.prop;
this.testObj!.prop?.prop;
}
}
`,
errors: [
{
data: {
name: 'testObj',
},
line: 3,
messageId: 'preferReadonly',
},
],
},
{
code: `
class TestObject {
public prop: number;
}

class Test {
private testObj = new TestObject();

public test(): void {
this.testObj.prop = 10;
this.testObj.prop;
this.testObj?.prop;
this.testObj!.prop;
this.testObj!.prop = '';
this.testObj.prop.prop = '';
this.testObj.prop.doesSomething();
this.testObj?.prop.prop;
this.testObj?.prop?.prop;
this.testObj.prop?.prop;
this.testObj!.prop?.prop;
}
}
`,
output: `
class TestObject {
public prop: number;
}

class Test {
private readonly testObj = new TestObject();

public test(): void {
this.testObj.prop = 10;
this.testObj.prop;
this.testObj?.prop;
this.testObj!.prop;
this.testObj!.prop = '';
this.testObj.prop.prop = '';
this.testObj.prop.doesSomething();
this.testObj?.prop.prop;
this.testObj?.prop?.prop;
this.testObj.prop?.prop;
this.testObj!.prop?.prop;
}
}
`,
errors: [
{
data: {
name: 'testObj',
},
line: 7,
messageId: 'preferReadonly',
},
],
},
Copy link
Member

Choose a reason for hiding this comment

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

A few more test cases to consider...

this.testObj.prop;
this.testObj?.prop;
this.testObj!.prop;
this.testObj!.prop = '';
this.testObj.prop.prop = '';
this.testObj.prop.doesSomething();
this.testObj?.prop.prop;
this.testObj?.prop?.prop;
this.testObj.prop?.prop;
this.testObj!.prop?.prop;

For any change to rule logic, it's generally good to try to add both valid and invalid test cases.

Copy link
Member

Choose a reason for hiding this comment

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

@islandryu by "test cases" I do mean cases plural - we generally try to keep these separate, so that each test case tests one case. It's easy to miss incorrect or mismatched errors when they're all lumped together. Could you split them up please?

],
});