Skip to content

Commit

Permalink
fix(eslint-plugin): [prefer-readonly-parameter-types] handle recursiv…
Browse files Browse the repository at this point in the history 
…e types (#1672)

Fixes #1665
  • Loading branch information
@bradzacher
bradzacher committed Mar 4, 2020
1 parent fbf1640 commit e5db36f
Show file tree
Hide file tree
Showing 2 changed files with 203 additions and 46 deletions.
128 changes: 82 additions & 46 deletions packages/eslint-plugin/src/util/isTypeReadonly.ts
View file
Expand Up @@ -8,30 +8,40 @@ import {
import * as ts from 'typescript';
import { nullThrows, NullThrowsReasons } from '.';

/**
* Returns:
* - null if the type is not an array or tuple,
* - true if the type is a readonly array or readonly tuple,
* - false if the type is a mutable array or mutable tuple.
*/
const enum Readonlyness {
/** the type cannot be handled by the function */
UnknownType = 1,
/** the type is mutable */
Mutable = 2,
/** the type is readonly */
Readonly = 3,
}

function isTypeReadonlyArrayOrTuple(
checker: ts.TypeChecker,
type: ts.Type,
): boolean | null {
function checkTypeArguments(arrayType: ts.TypeReference): boolean {
seenTypes: Set<ts.Type>,
): Readonlyness {
function checkTypeArguments(arrayType: ts.TypeReference): Readonlyness {
const typeArguments = checker.getTypeArguments(arrayType);
// this shouldn't happen in reality as:
// - tuples require at least 1 type argument
// - ReadonlyArray requires at least 1 type argument
/* istanbul ignore if */ if (typeArguments.length === 0) {
// this shouldn't happen in reality as:
// - tuples require at least 1 type argument
// - ReadonlyArray requires at least 1 type argument
return true;
return Readonlyness.Readonly;
}

// validate the element types are also readonly
if (typeArguments.some(typeArg => !isTypeReadonly(checker, typeArg))) {
return false;
if (
typeArguments.some(
typeArg =>
isTypeReadonlyRecurser(checker, typeArg, seenTypes) ===
Readonlyness.Mutable,
)
) {
return Readonlyness.Mutable;
}
return true;
return Readonlyness.Readonly;
}

if (checker.isArrayType(type)) {
Expand All @@ -40,49 +50,46 @@ function isTypeReadonlyArrayOrTuple(
NullThrowsReasons.MissingToken('symbol', 'array type'),
);
const escapedName = symbol.getEscapedName();
if (escapedName === 'Array' && escapedName !== 'ReadonlyArray') {
return false;
if (escapedName === 'Array') {
return Readonlyness.Mutable;
}

return checkTypeArguments(type);
}

if (checker.isTupleType(type)) {
if (!type.target.readonly) {
return false;
return Readonlyness.Mutable;
}

return checkTypeArguments(type);
}

return null;
return Readonlyness.UnknownType;
}

/**
* Returns:
* - null if the type is not an object,
* - true if the type is an object with only readonly props,
* - false if the type is an object with at least one mutable prop.
*/
function isTypeReadonlyObject(
checker: ts.TypeChecker,
type: ts.Type,
): boolean | null {
function checkIndexSignature(kind: ts.IndexKind): boolean | null {
seenTypes: Set<ts.Type>,
): Readonlyness {
function checkIndexSignature(kind: ts.IndexKind): Readonlyness {
const indexInfo = checker.getIndexInfoOfType(type, kind);
if (indexInfo) {
return indexInfo.isReadonly ? true : false;
return indexInfo.isReadonly
? Readonlyness.Readonly
: Readonlyness.Mutable;
}

return null;
return Readonlyness.UnknownType;
}

const properties = type.getProperties();
if (properties.length) {
// ensure the properties are marked as readonly
for (const property of properties) {
if (!isPropertyReadonlyInType(type, property.getEscapedName(), checker)) {
return false;
return Readonlyness.Mutable;
}
}

Expand All @@ -97,59 +104,88 @@ function isTypeReadonlyObject(
checker.getTypeOfPropertyOfType(type, property.getName()),
NullThrowsReasons.MissingToken(`property "${property.name}"`, 'type'),
);
if (!isTypeReadonly(checker, propertyType)) {
return false;

// handle recursive types.
// we only need this simple check, because a mutable recursive type will break via the above prop readonly check
if (seenTypes.has(propertyType)) {
continue;
}

if (
isTypeReadonlyRecurser(checker, propertyType, seenTypes) ===
Readonlyness.Mutable
) {
return Readonlyness.Mutable;
}
}
}

const isStringIndexSigReadonly = checkIndexSignature(ts.IndexKind.String);
if (isStringIndexSigReadonly === false) {
if (isStringIndexSigReadonly === Readonlyness.Mutable) {
return isStringIndexSigReadonly;
}

const isNumberIndexSigReadonly = checkIndexSignature(ts.IndexKind.Number);
if (isNumberIndexSigReadonly === false) {
if (isNumberIndexSigReadonly === Readonlyness.Mutable) {
return isNumberIndexSigReadonly;
}

return true;
return Readonlyness.Readonly;
}

/**
* Checks if the given type is readonly
*/
function isTypeReadonly(checker: ts.TypeChecker, type: ts.Type): boolean {
// a helper function to ensure the seenTypes map is always passed down, except by the external caller
function isTypeReadonlyRecurser(
checker: ts.TypeChecker,
type: ts.Type,
seenTypes: Set<ts.Type>,
): Readonlyness.Readonly | Readonlyness.Mutable {
seenTypes.add(type);

if (isUnionType(type)) {
// all types in the union must be readonly
return unionTypeParts(type).every(t => isTypeReadonly(checker, t));
const result = unionTypeParts(type).every(t =>
isTypeReadonlyRecurser(checker, t, seenTypes),
);
const readonlyness = result ? Readonlyness.Readonly : Readonlyness.Mutable;
return readonlyness;
}

// all non-object, non-intersection types are readonly.
// this should only be primitive types
if (!isObjectType(type) && !isUnionOrIntersectionType(type)) {
return true;
return Readonlyness.Readonly;
}

// pure function types are readonly
if (
type.getCallSignatures().length > 0 &&
type.getProperties().length === 0
) {
return true;
return Readonlyness.Readonly;
}

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

const isReadonlyObject = isTypeReadonlyObject(checker, type);
/* istanbul ignore else */ if (isReadonlyObject !== null) {
const isReadonlyObject = isTypeReadonlyObject(checker, type, seenTypes);
/* istanbul ignore else */ if (
isReadonlyObject !== Readonlyness.UnknownType
) {
return isReadonlyObject;
}

throw new Error('Unhandled type');
}

/**
* Checks if the given type is readonly
*/
function isTypeReadonly(checker: ts.TypeChecker, type: ts.Type): boolean {
return (
isTypeReadonlyRecurser(checker, type, new Set()) === Readonlyness.Readonly
);
}

export { isTypeReadonly };
121 changes: 121 additions & 0 deletions packages/eslint-plugin/tests/rules/prefer-readonly-parameter-types.test.ts
View file
Expand Up @@ -199,6 +199,34 @@ ruleTester.run('prefer-readonly-parameter-types', rule, {
'function foo(arg: readonly string[]);', // TSDeclareFunction
'type Foo = (arg: readonly string[]) => void;', // TSFunctionType
'interface Foo { foo(arg: readonly string[]): void }', // TSMethodSignature

// https://github.com/typescript-eslint/typescript-eslint/issues/1665
// directly recursive
`
interface Foo {
readonly prop: Foo;
}
function foo(arg: Foo) {}
`,
// indirectly recursive
`
interface Foo {
readonly prop: Bar;
}
interface Bar {
readonly prop: Foo;
}
function foo(arg: Foo) {}
`,
`
interface Foo {
prop: Readonly<Bar>;
}
interface Bar {
prop: Readonly<Foo>;
}
function foo(arg: Readonly<Foo>) {}
`,
],
invalid: [
// arrays
Expand Down Expand Up @@ -503,5 +531,98 @@ ruleTester.run('prefer-readonly-parameter-types', rule, {
},
],
},

// https://github.com/typescript-eslint/typescript-eslint/issues/1665
// directly recursive
{
code: `
interface Foo {
prop: Foo;
}
function foo(arg: Foo) {}
`,
errors: [
{
messageId: 'shouldBeReadonly',
line: 5,
column: 22,
endColumn: 30,
},
],
},
{
code: `
interface Foo {
prop: Foo;
}
function foo(arg: Readonly<Foo>) {}
`,
errors: [
{
messageId: 'shouldBeReadonly',
line: 5,
column: 22,
endColumn: 40,
},
],
},
// indirectly recursive
{
code: `
interface Foo {
prop: Bar;
}
interface Bar {
readonly prop: Foo;
}
function foo(arg: Foo) {}
`,
errors: [
{
messageId: 'shouldBeReadonly',
line: 8,
column: 22,
endColumn: 30,
},
],
},
{
code: `
interface Foo {
prop: Bar;
}
interface Bar {
readonly prop: Foo;
}
function foo(arg: Readonly<Foo>) {}
`,
errors: [
{
messageId: 'shouldBeReadonly',
line: 8,
column: 22,
endColumn: 40,
},
],
},
{
code: `
interface Foo {
prop: Readonly<Bar>;
}
interface Bar {
prop: Readonly<Foo>;
}
function foo(arg: Foo) {}
`,
errors: [
{
messageId: 'shouldBeReadonly',
line: 8,
column: 22,
endColumn: 30,
},
],
},
],
});

0 comments on commit e5db36f

Please sign in to comment.