Skip to content

fix(eslint-plugin): [prefer-readonly] support modifiers of unions and intersections #8169

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

Merged
merged 3 commits into from
Jan 8, 2024
Merged

fix(eslint-plugin): [prefer-readonly] support modifiers of unions and intersections #8169

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
66b4d37
fix(eslint-plugin): [prefer-readonly] support modifiers of unions and…
auvred Jan 2, 2024
35dcfc3
Merge branch 'main' into fix/7221
auvred Jan 6, 2024
653b438
Merge branch 'main' into fix/7221
auvred Jan 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 66 additions & 13 deletions packages/eslint-plugin/src/rules/prefer-readonly.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,13 @@ type ParameterOrPropertyDeclaration =
const OUTSIDE_CONSTRUCTOR = -1;
const DIRECTLY_INSIDE_CONSTRUCTOR = 0;

enum TypeToClassRelation {
Copy link
Member

Choose a reason for hiding this comment

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

[Praise] I found this enum and the logic around it to be quite helpful in understanding the new logic. Very readable. Nice! 👏

ClassAndInstance,
Class,
Instance,
None,
}

class ClassScope {
private readonly privateModifiableMembers = new Map<
string,
Expand Down Expand Up @@ -312,29 +319,75 @@ class ClassScope {
).set(node.name.getText(), node);
}

public getTypeToClassRelation(type: ts.Type): TypeToClassRelation {
if (type.isIntersection()) {
let result: TypeToClassRelation = TypeToClassRelation.None;
for (const subType of type.types) {
const subTypeResult = this.getTypeToClassRelation(subType);
switch (subTypeResult) {
case TypeToClassRelation.Class:
if (result === TypeToClassRelation.Instance) {
return TypeToClassRelation.ClassAndInstance;
}
result = TypeToClassRelation.Class;
break;
case TypeToClassRelation.Instance:
if (result === TypeToClassRelation.Class) {
return TypeToClassRelation.ClassAndInstance;
}
result = TypeToClassRelation.Instance;
break;
}
}
return result;
}
if (type.isUnion()) {
// any union of class/instance and something else will prevent access to
// private members, so we assume that union consists only of classes
// or class instances, because otherwise tsc will report an error
return this.getTypeToClassRelation(type.types[0]);
}
Comment on lines +344 to +349
Copy link
Member Author

Choose a reason for hiding this comment

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

Playground

class Test {
  private foo: number = 1
  private static foo: number

  test() {
    const that1 = {} as typeof Test | this
    that1.foo = 2
    //     ^ 2339: Property 'foo' does not exist on type 'this | typeof Test'

    const that2 = {} as this | { _foo: 'bar' }
    that2.foo = 2
    //     ^ 2339: Property 'foo' does not exist on type 'this | { _foo: "bar"; }'

    const that3 = {} as typeof Test | { _foo: 'bar' }
    that3.foo = 2
    //     ^ 2339: Property 'foo' does not exist on type 'typeof Test | { _foo: "bar"; }'
  }
}


if (!type.getSymbol() || !typeIsOrHasBaseType(type, this.classType)) {
return TypeToClassRelation.None;
}

const typeIsClass =
tsutils.isObjectType(type) &&
tsutils.isObjectFlagSet(type, ts.ObjectFlags.Anonymous);

if (typeIsClass) {
return TypeToClassRelation.Class;
}

return TypeToClassRelation.Instance;
}

public addVariableModification(node: ts.PropertyAccessExpression): void {
const modifierType = this.checker.getTypeAtLocation(node.expression);

const relationOfModifierTypeToClass =
this.getTypeToClassRelation(modifierType);

if (
!modifierType.getSymbol() ||
!typeIsOrHasBaseType(modifierType, this.classType)
relationOfModifierTypeToClass === TypeToClassRelation.Instance &&
this.constructorScopeDepth === DIRECTLY_INSIDE_CONSTRUCTOR
) {
return;
}

const modifyingStatic =
tsutils.isObjectType(modifierType) &&
tsutils.isObjectFlagSet(modifierType, ts.ObjectFlags.Anonymous);
if (
!modifyingStatic &&
this.constructorScopeDepth === DIRECTLY_INSIDE_CONSTRUCTOR
relationOfModifierTypeToClass === TypeToClassRelation.Instance ||
relationOfModifierTypeToClass === TypeToClassRelation.ClassAndInstance
) {
return;
this.memberVariableModifications.add(node.name.text);
}
if (
relationOfModifierTypeToClass === TypeToClassRelation.Class ||
relationOfModifierTypeToClass === TypeToClassRelation.ClassAndInstance
) {
this.staticVariableModifications.add(node.name.text);
}

(modifyingStatic
? this.staticVariableModifications
: this.memberVariableModifications
).add(node.name.text);
}

public enterConstructor(
Expand Down
159 changes: 159 additions & 0 deletions packages/eslint-plugin/tests/rules/prefer-readonly.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,6 +651,72 @@ class Foo {
}
`,
},
`
class TestIntersection {
private prop: number = 3;

test() {
const that = {} as this & { _foo: 'bar' };
that.prop = 1;
}
}
`,
`
class TestUnion {
private prop: number = 3;

test() {
const that = {} as this | (this & { _foo: 'bar' });
that.prop = 1;
}
}
`,
`
class TestStaticIntersection {
private static prop: number;

test() {
const that = {} as typeof TestStaticIntersection & { _foo: 'bar' };
that.prop = 1;
}
}
`,
`
class TestStaticUnion {
private static prop: number = 1;

test() {
const that = {} as
| typeof TestStaticUnion
| (typeof TestStaticUnion & { _foo: 'bar' });
that.prop = 1;
}
}
`,
`
class TestBothIntersection {
private prop1: number = 1;
private static prop2: number;

test() {
const that = {} as typeof TestBothIntersection & this;
that.prop1 = 1;
that.prop2 = 1;
}
}
`,
`
class TestBothIntersection {
private prop1: number = 1;
private static prop2: number;

test() {
const that = {} as this & typeof TestBothIntersection;
that.prop1 = 1;
that.prop2 = 1;
}
}
`,
],
invalid: [
{
Expand Down Expand Up @@ -1978,5 +2044,98 @@ function ClassWithName<TBase extends new (...args: any[]) => {}>(Base: TBase) {
},
],
},
{
code: `
class Test {
private prop: number = 3;

test() {
const that = {} as this & { _foo: 'bar' };
that._foo = 1;
}
}
`,
output: `
class Test {
private readonly prop: number = 3

test() {
const that = {} as this & {_foo: 'bar'}
that._foo = 1
}
}
`,
errors: [
{
data: {
name: 'prop',
},
line: 3,
messageId: 'preferReadonly',
},
],
},
{
code: `
class Test {
private prop: number = 3;

test() {
const that = {} as this | (this & { _foo: 'bar' });
that.prop;
}
}
`,
output: `
class Test {
private readonly prop: number = 3

test() {
const that = {} as this | (this & {_foo: 'bar'})
that.prop
}
}
`,
errors: [
{
data: {
name: 'prop',
},
line: 3,
messageId: 'preferReadonly',
},
],
},
{
code: `
class Test {
private prop: number;

constructor() {
const that = {} as this & { _foo: 'bar' };
that.prop = 1;
}
}
`,
output: `
class Test {
private readonly prop: number

constructor() {
const that = {} as this & {_foo: 'bar'}
that.prop = 1
}
}
`,
errors: [
{
data: {
name: 'prop',
},
line: 3,
messageId: 'preferReadonly',
},
],
},
],
});