Skip to content
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.

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

[prefer-readonly-parameter-types] unique symbols as interface keys causes eslint to crash #2143

Closed
JacobLey opened this issue Jun 1, 2020 · 1 comment · Fixed by #2567
Closed
Labels
bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@JacobLey
Copy link

JacobLey commented Jun 1, 2020

Repro
tsconfig.json

{}

package.json

{
  "name": "prefer-readonly-parameter-types-test",
  "dependencies": {
    "@typescript-eslint/eslint-plugin": "3.0.2",
    "@typescript-eslint/parser": "3.0.2",
    "eslint": "7.1.0",
    "typescript": "3.9.3"
  }
}

.eslintrc.json

{
    "root": true,
    "parser": "@typescript-eslint/parser",
    "plugins": ["@typescript-eslint"],
    "parserOptions": {
        "project": ["tsconfig.json"]
    },
    "rules": {
        "@typescript-eslint/prefer-readonly-parameter-types": "error"
    }
}

test.ts

const sym = Symbol('sym');

interface MyInterface {
    [sym]: number;
}

const willCrash = (foo: Readonly<MyInterface>) => {};

Test command: eslint . --ext .ts

Expected Result
eslint . --ext .ts should not crash (successfully lint code) and for the given minimal rule set above, should not return any errors.

Actual Result

Oops! Something went wrong! :(

ESLint: 7.1.0

Error: Non-null Assertion Failed: Expected to find a property "__@sym@10" for the type.
Occurred while linting <ROOT>/test.ts:7
    at Object.nullThrows (<ROOT>/node_modules/@typescript-eslint/eslint-plugin/dist/util/nullThrows.js:23:15)
    at isTypeReadonlyObject (<ROOT>/node_modules/@typescript-eslint/eslint-plugin/dist/util/isTypeReadonly.js:83:37)
    at isTypeReadonlyRecurser (<ROOT>/node_modules/@typescript-eslint/eslint-plugin/dist/util/isTypeReadonly.js:128:30)
    at Object.isTypeReadonly (<ROOT>/node_modules/@typescript-eslint/eslint-plugin/dist/util/isTypeReadonly.js:138:13)
    at ArrowFunctionExpression, FunctionDeclaration, FunctionExpression, TSCallSignatureDeclaration, TSConstructSignatureDeclaration, TSDeclareFunction, TSEmptyBodyFunctionExpression, TSFunctionType, TSMethodSignature (<ROOT>/node_modules/@typescript-eslint/eslint-plugin/dist/rules/prefer-readonly-parameter-types.js:79:45)
    at <ROOT>/node_modules/eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (<ROOT>/node_modules/eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (<ROOT>/node_modules/eslint/lib/linter/node-event-generator.js:254:26)
    at NodeEventGenerator.applySelectors (<ROOT>/node_modules/eslint/lib/linter/node-event-generator.js:283:22)

Additional Info
(I'll save the noise of dumping the debug output, but can provide if requested)

The error is being emitted here: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/src/util/isTypeReadonly.ts#L104-L105

(So I don't think it is explicitly related to prefer-readonly-parameter-types, but it is one that successfully recreates the error).

My understanding is the purpose of this check is to ensure that properties being referenced are defined in the root type.

In my case, the only property is a "unique symbol" that typescript has aliased as __@sym@10.
So property.getName() => __@sym@10.

I can't find any documentation for Typescript's checker, besides the raw source code but that getTypeOfPropertyOfType on checker is defined as a higher order function getTypeOfPropertyOfType: (type, name) => getTypeOfPropertyOfType(type, escapeLeadingUnderscores(name)),

Note the escapeLeadingUnderscores which basically transforms __@sym@10 => ___@sym@10 (three underscores at start). That key is not present in the type.members map, so it returns undefined and results in a thrown error.

Due to typescript's handling of double underscore, this could be a typescript error, but the code compiles and is valid in typescript, so I imagine there is a correct way around this. The decision to prepend double underscore and not pre-escape the value feels like a conscious decision on their part.

All I've been able to do so far is understand why the error is happening... I can't claim I really understand the logic behind the code design, or an alternative solution.

e.g. is there an alternative to getTypeOfPropertyOfType that handles this edge case? Or is that call even necessary to perform readonly logic?

I'd be happy to help with a PR, but I as I said I can't find any decent documentation for Typescript's checker, so I don't even know where to start.

Versions

package version
@typescript-eslint/eslint-plugin 3.0.2
@typescript-eslint/parser 3.0.2
TypeScript 3.9.3
ESLint 7.1.0
node 14.3.0
npm 6.14.5
@JacobLey JacobLey added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Jun 1, 2020
@bradzacher bradzacher added bug Something isn't working and removed triage Waiting for maintainers to take a look labels Jun 1, 2020
@bradzacher
Copy link
Member

I think this is just weirdness in the TS API.
Looking at tsutils, they handle this themselves:

https://github.com/ajafff/tsutils/blob/8fceea350ce528bd911ea6eca4cce62ba83ee548/util/type.ts#L176-L180

We will probably have to do the same thing.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants