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
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.
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
},
"devDependencies": {
"@typescript-eslint/parser": "5.55.0",
"ajv": "^8.12.0",
"typescript": "*"
},
"peerDependencies": {
Expand Down
Loading

0 comments on commit c9427b7

Please sign in to comment.