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.

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

feat(eslint-plugin): [prefer-readonly-parameter-types] add option treatMethodsAsReadonly #3733

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Expand Up @@ -130,6 +130,7 @@ interface Options {
const defaultOptions: Options = {
checkParameterProperties: true,
ignoreInferredTypes: false,
treatMethodsAsReadonly: false,
};
```

Expand Down Expand Up @@ -214,3 +215,43 @@ export const acceptsCallback: AcceptsCallback;
```

</details>

### `treatMethodsAsReadonly`

This option allows you to treat all mutable methods as though they were readonly. This may be desirable in when you are never reassigning methods.

Examples of **incorrect** code for this rule with `{treatMethodsAsReadonly: false}`:

```ts
type MyType = {
readonly prop: string;
method(): string; // note: this method is mutable
};
function foo(arg: MyType) {}
```

Examples of **correct** code for this rule with `{treatMethodsAsReadonly: false}`:

```ts
type MyType = Readonly<{
prop: string;
method(): string;
}>;
function foo(arg: MyType) {}

type MyOtherType = {
readonly prop: string;
readonly method: () => string;
};
function bar(arg: MyOtherType) {}
```

Examples of **correct** code for this rule with `{treatMethodsAsReadonly: true}`:

```ts
type MyType = {
readonly prop: string;
method(): string; // note: this method is mutable
};
function foo(arg: MyType) {}
```
Expand Up @@ -8,7 +8,7 @@ type Options = [
{
checkParameterProperties?: boolean;
ignoreInferredTypes?: boolean;
},
} & util.ReadonlynessOptions,
];
type MessageIds = 'shouldBeReadonly';

Expand All @@ -34,6 +34,7 @@ export default util.createRule<Options, MessageIds>({
ignoreInferredTypes: {
type: 'boolean',
},
...util.readonlynessOptionsSchema.properties,
},
},
],
Expand All @@ -45,10 +46,13 @@ export default util.createRule<Options, MessageIds>({
{
checkParameterProperties: true,
ignoreInferredTypes: false,
...util.readonlynessOptionsDefaults,
},
],
create(context, options) {
const [{ checkParameterProperties, ignoreInferredTypes }] = options;
const [
{ checkParameterProperties, ignoreInferredTypes, treatMethodsAsReadonly },
] = options;
const { esTreeNodeToTSNodeMap, program } = util.getParserServices(context);
const checker = program.getTypeChecker();

Expand Down Expand Up @@ -94,7 +98,9 @@ export default util.createRule<Options, MessageIds>({

const tsNode = esTreeNodeToTSNodeMap.get(actualParam);
const type = checker.getTypeAtLocation(tsNode);
const isReadOnly = util.isTypeReadonly(checker, type);
const isReadOnly = util.isTypeReadonly(checker, type, {
treatMethodsAsReadonly: treatMethodsAsReadonly!,
});

if (!isReadOnly) {
context.report({
Expand Down
68 changes: 60 additions & 8 deletions packages/eslint-plugin/src/util/isTypeReadonly.ts
Expand Up @@ -4,6 +4,7 @@ import {
isUnionOrIntersectionType,
unionTypeParts,
isPropertyReadonlyInType,
isSymbolFlagSet,
} from 'tsutils';
import * as ts from 'typescript';
import { getTypeOfPropertyOfType, nullThrows, NullThrowsReasons } from '.';
Expand All @@ -17,9 +18,32 @@ const enum Readonlyness {
Readonly = 3,
}

export interface ReadonlynessOptions {
readonly treatMethodsAsReadonly?: boolean;
}

export const readonlynessOptionsSchema = {
type: 'object',
additionalProperties: false,
properties: {
treatMethodsAsReadonly: {
type: 'boolean',
},
},
};

export const readonlynessOptionsDefaults: ReadonlynessOptions = {
treatMethodsAsReadonly: false,
};

function hasSymbol(node: ts.Node): node is ts.Node & { symbol: ts.Symbol } {
return Object.prototype.hasOwnProperty.call(node, 'symbol');
}

function isTypeReadonlyArrayOrTuple(
checker: ts.TypeChecker,
type: ts.Type,
options: ReadonlynessOptions,
seenTypes: Set<ts.Type>,
): Readonlyness {
function checkTypeArguments(arrayType: ts.TypeReference): Readonlyness {
Expand All @@ -40,7 +64,7 @@ function isTypeReadonlyArrayOrTuple(
if (
typeArguments.some(
typeArg =>
isTypeReadonlyRecurser(checker, typeArg, seenTypes) ===
isTypeReadonlyRecurser(checker, typeArg, options, seenTypes) ===
Readonlyness.Mutable,
)
) {
Expand Down Expand Up @@ -76,6 +100,7 @@ function isTypeReadonlyArrayOrTuple(
function isTypeReadonlyObject(
checker: ts.TypeChecker,
type: ts.Type,
options: ReadonlynessOptions,
seenTypes: Set<ts.Type>,
): Readonlyness {
function checkIndexSignature(kind: ts.IndexKind): Readonlyness {
Expand All @@ -93,7 +118,18 @@ function isTypeReadonlyObject(
if (properties.length) {
// ensure the properties are marked as readonly
for (const property of properties) {
if (!isPropertyReadonlyInType(type, property.getEscapedName(), checker)) {
if (
!(
isPropertyReadonlyInType(type, property.getEscapedName(), checker) ||
(options.treatMethodsAsReadonly &&
property.valueDeclaration !== undefined &&
hasSymbol(property.valueDeclaration) &&
isSymbolFlagSet(
property.valueDeclaration.symbol,
ts.SymbolFlags.Method,
))
)
) {
return Readonlyness.Mutable;
}
}
Expand All @@ -117,7 +153,7 @@ function isTypeReadonlyObject(
}

if (
isTypeReadonlyRecurser(checker, propertyType, seenTypes) ===
isTypeReadonlyRecurser(checker, propertyType, options, seenTypes) ===
Readonlyness.Mutable
) {
return Readonlyness.Mutable;
Expand All @@ -142,14 +178,15 @@ function isTypeReadonlyObject(
function isTypeReadonlyRecurser(
checker: ts.TypeChecker,
type: ts.Type,
options: ReadonlynessOptions,
seenTypes: Set<ts.Type>,
): Readonlyness.Readonly | Readonlyness.Mutable {
seenTypes.add(type);

if (isUnionType(type)) {
// all types in the union must be readonly
const result = unionTypeParts(type).every(t =>
isTypeReadonlyRecurser(checker, t, seenTypes),
isTypeReadonlyRecurser(checker, t, options, seenTypes),
);
const readonlyness = result ? Readonlyness.Readonly : Readonlyness.Mutable;
return readonlyness;
Expand All @@ -169,12 +206,22 @@ function isTypeReadonlyRecurser(
return Readonlyness.Readonly;
}

const isReadonlyArray = isTypeReadonlyArrayOrTuple(checker, type, seenTypes);
const isReadonlyArray = isTypeReadonlyArrayOrTuple(
checker,
type,
options,
seenTypes,
);
if (isReadonlyArray !== Readonlyness.UnknownType) {
return isReadonlyArray;
}

const isReadonlyObject = isTypeReadonlyObject(checker, type, seenTypes);
const isReadonlyObject = isTypeReadonlyObject(
checker,
type,
options,
seenTypes,
);
/* istanbul ignore else */ if (
isReadonlyObject !== Readonlyness.UnknownType
) {
Expand All @@ -187,9 +234,14 @@ function isTypeReadonlyRecurser(
/**
* Checks if the given type is readonly
*/
function isTypeReadonly(checker: ts.TypeChecker, type: ts.Type): boolean {
function isTypeReadonly(
checker: ts.TypeChecker,
type: ts.Type,
options: ReadonlynessOptions,
): boolean {
return (
isTypeReadonlyRecurser(checker, type, new Set()) === Readonlyness.Readonly
isTypeReadonlyRecurser(checker, type, options, new Set()) ===
Readonlyness.Readonly
);
}

Expand Down
Expand Up @@ -154,6 +154,74 @@ ruleTester.run('prefer-readonly-parameter-types', rule, {
}
function foo(arg: Readonly<Foo>) {}
`,
// immutable methods
`
type MyType = Readonly<{
prop: string;
method(): string;
}>;
function foo(arg: MyType) {}
`,
`
type MyType = {
readonly prop: string;
readonly method: () => string;
};
function bar(arg: MyType) {}
`,
// methods treated as readonly
{
code: `
type MyType = {
readonly prop: string;
method(): string;
};
function foo(arg: MyType) {}
`,
options: [
{
treatMethodsAsReadonly: true,
},
],
},
{
code: `
class Foo {
method() {}
}
function foo(arg: Foo) {}
`,
options: [
{
treatMethodsAsReadonly: true,
},
],
},
{
code: `
interface Foo {
method(): void;
}
function foo(arg: Foo) {}
`,
options: [
{
treatMethodsAsReadonly: true,
},
],
},
// ReadonlySet and ReadonlyMap are seen as readonly when methods are treated as readonly
{
code: `
function foo(arg: ReadonlySet<string>) {}
function bar(arg: ReadonlyMap<string, string>) {}
`,
options: [
{
treatMethodsAsReadonly: true,
},
],
},

// parameter properties should work fine
{
Expand Down Expand Up @@ -715,5 +783,23 @@ ruleTester.run('prefer-readonly-parameter-types', rule, {
},
],
},
// Mutable methods.
{
code: `
type MyType = {
readonly prop: string;
method(): string;
};
function foo(arg: MyType) {}
`,
errors: [
{
messageId: 'shouldBeReadonly',
line: 6,
column: 22,
endColumn: 33,
},
],
},
],
});