From deddf543886206a755faa324a1fe20eae29dfa44 Mon Sep 17 00:00:00 2001 From: Brad Zacher Date: Thu, 30 Jan 2020 21:17:28 -0800 Subject: [PATCH] feat: object checks --- .../index.ts => isTypeReadonly.ts} | 32 ++++-- .../util/isTypeReadonly/isReadonlySymbol.ts | 82 -------------- .../prefer-readonly-parameter-types.test.ts | 100 +++++++++++------- .../eslint-plugin/typings/typescript.d.ts | 15 ++- 4 files changed, 94 insertions(+), 135 deletions(-) rename packages/eslint-plugin/src/util/{isTypeReadonly/index.ts => isTypeReadonly.ts} (80%) delete mode 100644 packages/eslint-plugin/src/util/isTypeReadonly/isReadonlySymbol.ts diff --git a/packages/eslint-plugin/src/util/isTypeReadonly/index.ts b/packages/eslint-plugin/src/util/isTypeReadonly.ts similarity index 80% rename from packages/eslint-plugin/src/util/isTypeReadonly/index.ts rename to packages/eslint-plugin/src/util/isTypeReadonly.ts index 06eae5475d5..eea0a8785df 100644 --- a/packages/eslint-plugin/src/util/isTypeReadonly/index.ts +++ b/packages/eslint-plugin/src/util/isTypeReadonly.ts @@ -3,10 +3,10 @@ import { isUnionType, isUnionOrIntersectionType, unionTypeParts, + isPropertyReadonlyInType, } from 'tsutils'; import * as ts from 'typescript'; -import { nullThrows, NullThrowsReasons } from '..'; -import { isReadonlySymbol } from './isReadonlySymbol'; +import { nullThrows, NullThrowsReasons } from '.'; /** * Returns: @@ -79,20 +79,32 @@ function isTypeReadonlyObject( const properties = type.getProperties(); if (properties.length) { + // ensure the properties are marked as readonly for (const property of properties) { - // eslint-disable-next-line @typescript-eslint/ban-ts-ignore - // @ts-ignore - const x = checker.isReadonlySymbol(property); - const y = isReadonlySymbol(property); - if (x !== y) { - throw new Error('FUCK'); - } - if (!isReadonlySymbol(property)) { + if (!isPropertyReadonlyInType( + type, + property.getEscapedName(), + checker, + )) { return false; } } // all properties were readonly + // now ensure that all of the values are readonly also. + + // do this after checking property readonly-ness as a perf optimization, + // as we might be able to bail out early due to a mutable property before + // doing this deep, potentially expensive check. + for (const property of properties) { + const propertyType = nullThrows( + checker.getTypeOfPropertyOfType(type, property.getName()), + NullThrowsReasons.MissingToken(`property "${property.name}"`, 'type'), + ); + if (!isTypeReadonly(checker, propertyType)) { + return false; + } + } } const isStringIndexSigReadonly = checkIndexSignature(ts.IndexKind.String); diff --git a/packages/eslint-plugin/src/util/isTypeReadonly/isReadonlySymbol.ts b/packages/eslint-plugin/src/util/isTypeReadonly/isReadonlySymbol.ts deleted file mode 100644 index 69b55945938..00000000000 --- a/packages/eslint-plugin/src/util/isTypeReadonly/isReadonlySymbol.ts +++ /dev/null @@ -1,82 +0,0 @@ -// - this code is ported from typescript's type checker -// Starting at https://github.com/Microsoft/TypeScript/blob/4212484ae18163df867f53dab19a8cc0c6000793/src/compiler/checker.ts#L26285 - -import * as ts from 'typescript'; - -// #region internal types used for isReadonlySymbol - -// we can't use module augmentation because typescript uses export = ts -/* eslint-disable @typescript-eslint/ban-ts-ignore */ - -// CheckFlags is actually const enum -// https://github.com/Microsoft/TypeScript/blob/236012e47b26fee210caa9cbd2e072ef9e99f9ae/src/compiler/types.ts#L4038 -const enum CheckFlags { - Readonly = 1 << 3, -} -type GetCheckFlags = (symbol: ts.Symbol) => CheckFlags; -// @ts-ignore -const getCheckFlags: GetCheckFlags = ts.getCheckFlags; - -type GetDeclarationModifierFlagsFromSymbol = (s: ts.Symbol) => ts.ModifierFlags; -const getDeclarationModifierFlagsFromSymbol: GetDeclarationModifierFlagsFromSymbol = - // @ts-ignore - ts.getDeclarationModifierFlagsFromSymbol; - -/* eslint-enable @typescript-eslint/ban-ts-ignore */ - -// function getDeclarationNodeFlagsFromSymbol(s: ts.Symbol): ts.NodeFlags { -// return s.valueDeclaration ? ts.getCombinedNodeFlags(s.valueDeclaration) : 0; -// } - -// #endregion - -function isReadonlySymbol(symbol: ts.Symbol): boolean { - // The following symbols are considered read-only: - // Properties with a 'readonly' modifier - // Variables declared with 'const' - // Get accessors without matching set accessors - // Enum members - // Unions and intersections of the above (unions and intersections eagerly set isReadonly on creation) - - // transient readonly property - if (getCheckFlags(symbol) & CheckFlags.Readonly) { - console.log('check flags is truthy'); - return true; - } - - // Properties with a 'readonly' modifier - if ( - symbol.flags & ts.SymbolFlags.Property && - getDeclarationModifierFlagsFromSymbol(symbol) & ts.ModifierFlags.Readonly - ) { - return true; - } - - // Variables declared with 'const' - // if ( - // symbol.flags & ts.SymbolFlags.Variable && - // getDeclarationNodeFlagsFromSymbol(symbol) & ts.NodeFlags.Const - // ) { - // return true; - // } - - // Get accessors without matching set accessors - if ( - symbol.flags & ts.SymbolFlags.Accessor && - !(symbol.flags & ts.SymbolFlags.SetAccessor) - ) { - return true; - } - - // Enum members - if (symbol.flags & ts.SymbolFlags.EnumMember) { - return true; - } - - return false; - - // TODO - maybe add this check? - // || symbol.declarations.some(isReadonlyAssignmentDeclaration) -} - -export { isReadonlySymbol }; diff --git a/packages/eslint-plugin/tests/rules/prefer-readonly-parameter-types.test.ts b/packages/eslint-plugin/tests/rules/prefer-readonly-parameter-types.test.ts index 8d85b687b6a..894f6de6245 100644 --- a/packages/eslint-plugin/tests/rules/prefer-readonly-parameter-types.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-readonly-parameter-types.test.ts @@ -39,19 +39,12 @@ const arrays = [ 'readonly [string]', 'Readonly<[string]>', ]; +const objects = [ + '{ foo: "" }', + '{ foo: readonly string[] }', + '{ foo(): void }', +]; const weirdIntersections = [ - ` - interface Test extends ReadonlyArray { - readonly property: boolean - } - function foo(arg: Test) {} - `, - ` - type Test = (readonly string[]) & { - readonly property: boolean - }; - function foo(arg: Test) {} - `, ` interface Test { (): void @@ -65,26 +58,10 @@ const weirdIntersections = [ }; function foo(arg: Test) {} `, - ` - type Test = string & number; - function foo(arg: Test) {} - `, ]; ruleTester.run('prefer-readonly-parameter-types', rule, { valid: [ - ` - type Test = (readonly string[]) & { - property: boolean - }; - function foo(arg: Test) {} - `, - ` - interface Test extends ReadonlyArray { - property: boolean - } - function foo(arg: Test) {} - `, 'function foo(arg: { readonly a: string }) {}', 'function foo() {}', @@ -108,9 +85,35 @@ ruleTester.run('prefer-readonly-parameter-types', rule, { 'function foo(arg: ReadonlyArray | ReadonlyArray) {}', // objects + ...objects.map(type => `function foo(arg: Readonly<${type}>) {}`), + ` + function foo(arg: { + readonly foo: { + readonly bar: string + } + }) {} + `, // weird other cases ...weirdIntersections.map(code => code), + ` + interface Test extends ReadonlyArray { + readonly property: boolean + } + function foo(arg: Readonly) {} + `, + ` + type Test = (readonly string[]) & { + readonly property: boolean + }; + function foo(arg: Readonly) {} + `, + ` + type Test = string & number; + function foo(arg: Test) {} + `, + + // declaration merging ` class Foo { readonly bang = 1; @@ -123,6 +126,7 @@ ruleTester.run('prefer-readonly-parameter-types', rule, { } function foo(arg: Foo) {} `, + // method made readonly via Readonly ` class Foo { method() {} @@ -181,16 +185,36 @@ ruleTester.run('prefer-readonly-parameter-types', rule, { }, // objects - // { - // code: ` - // interface MutablePropFunction { - // (): void - // mutable: boolean - // } - // function foo(arg: MutablePropFunction) {} - // `, - // errors: [], - // }, + ...objects.map>(type => { + return { + code: `function foo(arg: ${type}) {}`, + errors: [ + { + messageId: 'shouldBeReadonly', + column: 14, + endColumn: 19 + type.length, + }, + ], + }; + }), + { + code: ` + function foo(arg: { + readonly foo: { + bar: string + } + }) {} + `, + errors: [ + { + messageId: 'shouldBeReadonly', + line: 2, + column: 22, + endLine: 6, + endColumn: 10, + }, + ], + }, // weird intersections ...weirdIntersections.map>( diff --git a/packages/eslint-plugin/typings/typescript.d.ts b/packages/eslint-plugin/typings/typescript.d.ts index 12b12c88953..7c9089158b4 100644 --- a/packages/eslint-plugin/typings/typescript.d.ts +++ b/packages/eslint-plugin/typings/typescript.d.ts @@ -6,16 +6,21 @@ declare module 'typescript' { /** * @returns `true` if the given type is an array type: - * - Array - * - ReadonlyArray - * - foo[] - * - readonly foo[] + * - `Array` + * - `ReadonlyArray` + * - `foo[]` + * - `readonly foo[]` */ isArrayType(type: Type): type is TypeReference; /** * @returns `true` if the given type is a tuple type: - * - [foo] + * - `[foo]` + * - `readonly [foo]` */ isTupleType(type: Type): type is TupleTypeReference; + /** + * Return the type of the given property in the given type, or undefined if no such property exists + */ + getTypeOfPropertyOfType(type: Type, propertyName: string): Type | undefined; } }