Skip to content

Commit

Permalink
feat(eslint-plugin): [prefer-readonly-parameter-types] added an optio…
Browse files Browse the repository at this point in the history 
…nal type allowlist (#4436)

* feat(eslint-plugin): [prefer-readonly-parameter-types] Added an optional type whitelist

* chore(eslint-plugin): [prefer-readonly-parameter-types] whitelist -> allowlist

* feat(eslint-plugin): [prefer-readonly-parameter-types] Split the allowlist between internal and configurable

* fix(eslint-plugin): [prefer-readonly-parameter-types] Fixed lint issue with non-null assertion

* fix(eslint-plugin): [prefer-readonly-parameter-types] Using allowlist everywhere in deep readonlyness checks

* fix(eslint-plugin): [prefer-readonly-parameter-types] Passing internal allowlist from rule

* fix(eslint-plugin): [prefer-readonly-parameter-types] Decoupled options and schema of rule and util

* feat(eslint-plugin): [prefer-readonly-parameter-types] Added tests

* fix(eslint-plugin): [prefer-readonly-parameter-types] Added missing docs for option treatMethodsAsReadonly

* docs(eslint-plugin): [prefer-readonly-parameter-types] Added docs for allowlist

* fix(eslint-plugin): [prefer-readonly-parameter-types] Fixed regressions from merging main

* feat(eslint-plugin): [prefer-readonly-parameter-types] Merged exceptions and internalExceptions together to create a universal allowlist API

* feat(eslint-plugin): [prefer-readonly-parameter-types] Added a schema for type allowlist

* chore(eslint-plugin): [prefer-readonly-parameter-types] Split TypeAllowlistItem out into own file

* docs(eslint-plugin): [prefer-readonly-parameter-types] Updated docs for the more sophisticated allowlist

* docs(eslint-plugin): [prefer-readonly-parameter-types] Fixed allowlist option type

* test(eslint-plugin): [prefer-readonly-parameter-types] Added tests for type allowlist with wrong kinds of types

* chore(eslint-plugin): [prefer-readonly-parameter-types] Deduplicated default configuration

* fix(eslint-plugin): [prefer-readonly-parameter-types] Added back readonlynessOptionsSchema

* chore(eslint-plugin): [prefer-readonly-parameter-types] Removed default allowlist

* docs(eslint-plugin): [prefer-readonly-parameter-types] Fixed default allowlist in docs

* test(eslint-plugin): [prefer-readonly-parameter-types] Not using DOM in tests

* chore(eslint-plugin): [prefer-readonly-parameter-types] Using property shorthand

* feat(eslint-plugin): [prefer-readonly-parameter-types] TypeAllowlistItem is now a discriminated union

* docs(eslint-plugin): [prefer-readonly-parameter-types] TypeAllowlistItem is now a discriminated union - docs update

* test(type-utils): [prefer-readonly-parameter-types] Added rudimentary test for allowlist

* test(type-utils): [prefer-readonly-parameter-types] Added test for allowlist containing local definition

* Update packages/type-utils/src/TypeAllowListItem.ts to use enum in JSON schema

Co-authored-by: Brad Zacher <brad.zacher@gmail.com>

* fix(eslint-plugin): [prefer-readonly-parameter-types] Added trainling slash to package path check

* fix(eslint-plugin) Fixed type imports not being separated

* feat(type-utils): Added TypeOrValueSpecifier, its schema and test for the schema

* feat(type-utils): Added typeMatchesSpecifier() and switched isTypeReadonly over to TypeOrValueSpecifier

* fix(eslint-plugin): [prefer-readonly-parameter-types] Fixed tests having old allowlist format

* fix(type-utils): Removed unneeded function isTypeExcepted

* feat(type-utils): Added source file checking to typeMatchesFileSpecifier()

* docs(eslint-plugin): [prefer-readonly-parameter-types] Updated docs to use TypeOrValueSpecifier allowlist style

* docs(eslint-plugin): [prefer-readonly-parameter-types] Typo fix

* docs(eslint-plugin): [prefer-readonly-parameter-types] Typo fix 2

* feat(type-utils): Added tests for typeMatchesSpecifier()

* fix(type-utils): Using node path joining typeMatchesSpecifier()

* feat(type-utils): Removed MultiSourceSpecifier

* chore(type-utils): Simplified typeMatchesSpecifier()

* feat(type-utils): Added more tests for typeMatchesSpecifier()

* docs(prefer-readonly-parameter-types) more legible docs

Co-authored-by: Josh Goldberg <git@joshuakgoldberg.com>

* fix(eslint-plugin): [prefer-readonly-parameter-types] Fixed missing end of code listing in docs

* chore(type-utils): Simplified typeDeclaredInFile()

* chore(type-utils): Using unknown instead of any in tests

* test(type-utils): grammar fix in test specifications

* chore: Reset yarn.lock

* chore: renamed readonlyness allowlist to just allow

* fix(type-utils): fixed services.program now being optional and not checked in tests

* test(type-utils): negative tests for isTypeReadonly

* fix(eslint-plugin): bracket style array notation

Co-authored-by: Josh Goldberg <git@joshuakgoldberg.com>

* fix(type-utils): Fixed array style

* fix(type-utils): Not fetching symbol repeatedly

* fix(type-utils): Remove ManySpecifiers format from TypeOrValueSpecifier schema

* docs(eslint-plugin): [prefer-readonly-parameter-types] described file specifier path as being relative

* path and package

* Update docs too

* Update docs too (again)

* Added test name helpers, and fixed test data

* test(type-utils): fixed package schema tests

* test(eslint-plugin): fixed type whitelist schema in prefer-readonly-parameter-types tests

* Applied lowercasing to typeDeclaredInFile

---------

Co-authored-by: Brad Zacher <brad.zacher@gmail.com>
Co-authored-by: Josh Goldberg <git@joshuakgoldberg.com>
  • Loading branch information
@marekdedic @bradzacher @JoshuaKGoldberg
3 people committed Mar 20, 2023
1 parent 6652ebe commit c9427b7
Show file tree
Hide file tree
Showing 10 changed files with 884 additions and 28 deletions.
95 changes: 95 additions & 0 deletions packages/eslint-plugin/docs/rules/prefer-readonly-parameter-types.md
View file
Expand Up @@ -129,6 +129,101 @@ interface Foo {

## Options

### `allow`

Some complex types cannot easily be made readonly, for example the `HTMLElement` type or the `JQueryStatic` type from `@types/jquery`. This option allows you to globally disable reporting of such types.

Each item must be one of:

- A type defined in a file (`{from: "file", name: "Foo", path: "src/foo-file.ts"}` with `path` being an optional path relative to the project root directory)
- A type from the default library (`{from: "lib", name: "Foo"}`)
- A type from a package (`{from: "package", name: "Foo", package: "foo-lib"}`, this also works for types defined in a typings package).

Additionally, a type may be defined just as a simple string, which then matches the type independently of its origin.

Examples of code for this rule with:

```json
{
"allow": [
"$",
{ "source": "file", "name": "Foo" },
{ "source": "lib", "name": "HTMLElement" },
{ "from": "package", "name": "Bar", "package": "bar-lib" }
]
}
```

<!--tabs-->

#### ❌ Incorrect

```ts
interface ThisIsMutable {
prop: string;
}

interface Wrapper {
sub: ThisIsMutable;
}

interface WrapperWithOther {
readonly sub: Foo;
otherProp: string;
}

function fn1(arg: ThisIsMutable) {} // Incorrect because ThisIsMutable is not readonly
function fn2(arg: Wrapper) {} // Incorrect because Wrapper.sub is not readonly
function fn3(arg: WrapperWithOther) {} // Incorrect because WrapperWithOther.otherProp is not readonly and not in the allowlist
```

```ts
import { Foo } from 'some-lib';
import { Bar } from 'incorrect-lib';

interface HTMLElement {
prop: string;
}

function fn1(arg: Foo) {} // Incorrect because Foo is not a local type
function fn2(arg: HTMLElement) {} // Incorrect because HTMLElement is not from the default library
function fn3(arg: Bar) {} // Incorrect because Bar is not from "bar-lib"
```

#### ✅ Correct

```ts
interface Foo {
prop: string;
}

interface Wrapper {
readonly sub: Foo;
readonly otherProp: string;
}

function fn1(arg: Foo) {} // Works because Foo is allowed
function fn2(arg: Wrapper) {} // Works even when Foo is nested somewhere in the type, with other properties still being checked
```

```ts
import { Bar } from 'bar-lib';

interface Foo {
prop: string;
}

function fn1(arg: Foo) {} // Works because Foo is a local type
function fn2(arg: HTMLElement) {} // Works because HTMLElement is from the default library
function fn3(arg: Bar) {} // Works because Bar is from "bar-lib"
```

```ts
import { Foo } from './foo';

function fn(arg: Foo) {} // Works because Foo is still a local type - it has to be in the same package
```

### `checkParameterProperties`

This option allows you to enable or disable the checking of parameter properties.
Expand Down
25 changes: 19 additions & 6 deletions packages/eslint-plugin/src/rules/prefer-readonly-parameter-types.ts
View file
Expand Up @@ -5,9 +5,11 @@ import * as util from '../util';

type Options = [
{
allow?: util.TypeOrValueSpecifier[];
checkParameterProperties?: boolean;
ignoreInferredTypes?: boolean;
} & util.ReadonlynessOptions,
treatMethodsAsReadonly?: boolean;
},
];
type MessageIds = 'shouldBeReadonly';

Expand All @@ -25,13 +27,15 @@ export default util.createRule<Options, MessageIds>({
type: 'object',
additionalProperties: false,
properties: {
allow: util.readonlynessOptionsSchema.properties.allow,
checkParameterProperties: {
type: 'boolean',
},
ignoreInferredTypes: {
type: 'boolean',
},
...util.readonlynessOptionsSchema.properties,
treatMethodsAsReadonly:
util.readonlynessOptionsSchema.properties.treatMethodsAsReadonly,
},
},
],
Expand All @@ -41,17 +45,25 @@ export default util.createRule<Options, MessageIds>({
},
defaultOptions: [
{
allow: util.readonlynessOptionsDefaults.allow,
checkParameterProperties: true,
ignoreInferredTypes: false,
...util.readonlynessOptionsDefaults,
treatMethodsAsReadonly:
util.readonlynessOptionsDefaults.treatMethodsAsReadonly,
},
],
create(
context,
[{ checkParameterProperties, ignoreInferredTypes, treatMethodsAsReadonly }],
[
{
allow,
checkParameterProperties,
ignoreInferredTypes,
treatMethodsAsReadonly,
},
],
) {
const services = util.getParserServices(context);
const checker = services.program.getTypeChecker();

return {
[[
Expand Down Expand Up @@ -94,8 +106,9 @@ export default util.createRule<Options, MessageIds>({
}

const type = services.getTypeAtLocation(actualParam);
const isReadOnly = util.isTypeReadonly(checker, type, {
const isReadOnly = util.isTypeReadonly(services.program, type, {
treatMethodsAsReadonly: treatMethodsAsReadonly!,
allow,
});

if (!isReadOnly) {
Expand Down
198 changes: 198 additions & 0 deletions packages/eslint-plugin/tests/rules/prefer-readonly-parameter-types.test.ts
View file
Expand Up @@ -401,6 +401,83 @@ ruleTester.run('prefer-readonly-parameter-types', rule, {
},
],
},
// Allowlist
{
code: `
interface Foo {
readonly prop: RegExp;
}
function foo(arg: Foo) {}
`,
options: [
{
allow: [{ from: 'lib', name: 'RegExp' }],
},
],
},
{
code: `
interface Foo {
prop: RegExp;
}
function foo(arg: Readonly<Foo>) {}
`,
options: [
{
allow: [{ from: 'lib', name: 'RegExp' }],
},
],
},
{
code: `
interface Foo {
prop: string;
}
function foo(arg: Foo) {}
`,
options: [
{
allow: [{ from: 'file', name: 'Foo' }],
},
],
},
{
code: `
interface Bar {
prop: string;
}
interface Foo {
readonly prop: Bar;
}
function foo(arg: Foo) {}
`,
options: [
{
allow: [{ from: 'file', name: 'Foo' }],
},
],
},
{
code: `
interface Bar {
prop: string;
}
interface Foo {
readonly prop: Bar;
}
function foo(arg: Foo) {}
`,
options: [
{
allow: [{ from: 'file', name: 'Bar' }],
},
],
},
],
invalid: [
// arrays
Expand Down Expand Up @@ -885,5 +962,126 @@ ruleTester.run('prefer-readonly-parameter-types', rule, {
`,
errors: [{ line: 6, messageId: 'shouldBeReadonly' }],
},
// Allowlist
{
code: `
function foo(arg: RegExp) {}
`,
options: [
{
allow: [{ from: 'file', name: 'Foo' }],
},
],
errors: [
{
messageId: 'shouldBeReadonly',
line: 2,
column: 22,
endColumn: 33,
},
],
},
{
code: `
interface Foo {
readonly prop: RegExp;
}
function foo(arg: Foo) {}
`,
options: [
{
allow: [{ from: 'file', name: 'Bar' }],
},
],
errors: [
{
messageId: 'shouldBeReadonly',
line: 6,
column: 22,
endColumn: 30,
},
],
},
{
code: `
interface Foo {
readonly prop: RegExp;
}
function foo(arg: Foo) {}
`,
options: [
{
allow: [{ from: 'lib', name: 'Foo' }],
},
],
errors: [
{
messageId: 'shouldBeReadonly',
line: 6,
column: 22,
endColumn: 30,
},
],
},
{
code: `
interface Foo {
readonly prop: RegExp;
}
function foo(arg: Foo) {}
`,
options: [
{
allow: [{ from: 'package', name: 'Foo', package: 'foo-lib' }],
},
],
errors: [
{
messageId: 'shouldBeReadonly',
line: 6,
column: 22,
endColumn: 30,
},
],
},
{
code: `
function foo(arg: RegExp) {}
`,
options: [
{
allow: [{ from: 'file', name: 'RegExp' }],
},
],
errors: [
{
messageId: 'shouldBeReadonly',
line: 2,
column: 22,
endColumn: 33,
},
],
},
{
code: `
function foo(arg: RegExp) {}
`,
options: [
{
allow: [{ from: 'package', name: 'RegExp', package: 'regexp-lib' }],
},
],
errors: [
{
messageId: 'shouldBeReadonly',
line: 2,
column: 22,
endColumn: 33,
},
],
},
],
});
1 change: 1 addition & 0 deletions packages/type-utils/package.json
View file
Expand Up @@ -52,6 +52,7 @@
},
"devDependencies": {
"@typescript-eslint/parser": "5.55.0",
"ajv": "^8.12.0",
"typescript": "*"
},
"peerDependencies": {
Expand Down

0 comments on commit c9427b7

Please sign in to comment.