Skip to content

[prefer-readonly-parameter-types] Proposal: Add type allowlist #4185

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.

Sign up for GitHub

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

Jump to bottom
Closed
marekdedic opened this issue Nov 18, 2021 · 5 comments · Fixed by #4436
Closed

[prefer-readonly-parameter-types] Proposal: Add type allowlist #4185

marekdedic opened this issue Nov 18, 2021 · 5 comments · Fixed by #4436
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@marekdedic
Copy link
Contributor

This propsal is a continuation of the discussion in #4061, cc @bradzacher

The rule prefer-readonly-parameter-types is known to not play nicely with certain types, with the canonical example being the HTMLElement from the standard lib. This type cannot be made deep-readonly, as that breaks TS compilation altogether.

Outstanding questions

  • Should only the types themselves be ignored, or should the check ignore them, even if they are part of a more complex type?
@marekdedic marekdedic added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Nov 18, 2021
@bradzacher bradzacher added enhancement: plugin rule option New rule option for an existing eslint-plugin rule and removed triage Waiting for team members to take a look labels Nov 19, 2021
@bradzacher
Copy link
Member

Copying my comment here for additional context:

We can create an allowlist of types that are problematic and expand on them as required.
This should be pretty straightforward to implement.

  1. get the Symbol for the type
  2. get the declarations for the type
  3. ensure that the declaration is within a TS "default library"
    • This check is to ensure that we don't false-negative if someone declares a HTMLElement type locally within their file

For example here is some code we could generalise from an existing rule:

typescript-eslint/packages/eslint-plugin/src/rules/no-implied-eval.ts

Lines 83 to 91 in 71c9370

if (symbol && symbol.escapedName === FUNCTION_CONSTRUCTOR) {
const declarations = symbol.getDeclarations() ?? [];
for (const declaration of declarations) {
const sourceFile = declaration.getSourceFile();
if (program.isSourceFileDefaultLibrary(sourceFile)) {
return true;
}
}
}

@marekdedic
Copy link
Contributor Author

marekdedic commented Nov 19, 2021 •
edited
Loading

As far as the subtype goes, I would allow that as well - e.g.

const x = {a: readonly number; b: {c: readonly string; d: HTMLElement;}; }

would pass (the rule would only skip x.b.d and still check the other fields).

@marekdedic
Copy link
Contributor Author

Hi,
could this issue please be marked as accepting PRs? Or are there any changes needed? I may have some time over Christmas for FOSS contributions, maybe this could be one of them :)

@bradzacher bradzacher added the accepting prs Go ahead, send a pull request that resolves this issue label Dec 22, 2021
@marekdedic marekdedic mentioned this issue Dec 27, 2021
Closed
@marekdedic marekdedic mentioned this issue Jan 12, 2022
Merged
6 tasks
@JoshuaKGoldberg JoshuaKGoldberg changed the title [prefer-readonly-parameter-types] Proposal: Add type whitelist [prefer-readonly-parameter-types] Proposal: Add type allowlist Jan 24, 2022
@RebeccaStevens
Copy link
Contributor

RebeccaStevens commented Jan 30, 2022 •
edited
Loading

Just thought I'd document this somewhere (the important parts should probably be added to the docs).

type-fest's ReadonlyDeep type does its best to convert any given type to an immutable type but currently fails for objects that contain both properties and call signatures.
I've submit a PR type-fest#359 to add support of a handling the case of single call signature with properties but it seems that due to a TypeScript limitation, multiple call signature cannot be supported. This issue is reported in TypeScript#29732.

allowlist will need to be used to for these type to prevent isTypeReadonly from returning false for them.
Note: treatMethodsAsReadonly isn't enough for this as ReadonlyDeep<T> just returns T for objects that have call signatures. Thus all properties will also stay mutable.

@RebeccaStevens RebeccaStevens mentioned this issue Jan 30, 2022
Closed
3 tasks
@bradzacher bradzacher mentioned this issue May 27, 2022
Closed
4 tasks
@RebeccaStevens RebeccaStevens mentioned this issue Sep 30, 2022
Closed
@marekdedic
Copy link
Contributor Author

Fixed in #4436, due to be released in 6.0.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(eslint-plugin): [prefer-readonly-parameter-types] added an optional type allowlist
3 participants
@marekdedic @RebeccaStevens @bradzacher