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

[no-unsafe-assignment] fails to infer the type from class properties #2109

Closed
photonstorm opened this issue May 26, 2020 · 6 comments · Fixed by #3394
Closed

[no-unsafe-assignment] fails to infer the type from class properties #2109

photonstorm opened this issue May 26, 2020 · 6 comments · Fixed by #3394
Labels
bug Something isn't working good first issue Good for newcomers has pr there is a PR raised to close this package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@photonstorm
Copy link

Repro

import { IEventInstance } from './IEventInstance';

export class EventEmitter
{
    events: Map<string, Set<IEventInstance>>;

    constructor ()
    {
        this.events = new Map();
    }
}

Expected Result

The no-unsafe-assignment rule is flagging the creation of the Map in the constructor as being unsafe, claiming it can accept any types, even though the type is enforced by the events property in the class.

Actual Result

I see no logical reason why this should be considered an unsafe assignment when the type is clearly enforced by the property. TypeScript itself prevents incorrect insertion of values into this Map.

Additional Info

In order to stop the rule complaining, the following works:

import { IEventInstance } from './IEventInstance';

export class EventEmitter
{
    events: Map<string, Set<IEventInstance>>;

    constructor ()
    {
        this.events = new Map<string, Set<IEventInstance>>();
    }
}

However, this is just duplicating the type already defined.

Versions

package version
@typescript-eslint/eslint-plugin 3.0.1
@typescript-eslint/parser 3.0.1
TypeScript 3.9.3
ESLint 7.1.0
node 10.16.0
npm 6.9.0
@photonstorm photonstorm added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels May 26, 2020
@bradzacher
Copy link
Member

Yeah I ran into this when attempting to introduce the rule into this codebase.

For some reason TS doesn't automatically infer the type of the RHS if it's new Map().
This works 100% of the time for new Set(), so it's unique to map.

I think it's due to how the Map constructor is defined.
https://github.com/microsoft/TypeScript/blob/6ac6a0410988e9f400c9153ac2b5fe9419849bf3/src/lib/es2015.collection.d.ts#L11-L16

interface MapConstructor {
    new(): Map<any, any>;
    new<K, V>(entries?: readonly (readonly [K, V])[] | null): Map<K, V>;
    readonly prototype: Map<any, any>;
}
declare var Map: MapConstructor;

Compared to Set:
https://github.com/microsoft/TypeScript/blob/6ac6a0410988e9f400c9153ac2b5fe9419849bf3/src/lib/es2015.collection.d.ts#L47-L51

interface SetConstructor {
    new <T = any>(values?: readonly T[] | null): Set<T>;
    readonly prototype: Set<any>;
}
declare var Set: SetConstructor;

Note that Map has an empty constructor which defaults to anys, and a parameterised constructor that doesn't.
OTOH Set only has a parameterised constructor, but its generic type has a default.

Unsure if it's a "bug" in TS or not. Either way, it's something that will have to be special cased in the rule unfortunately.

@bradzacher bradzacher added bug Something isn't working and removed triage Waiting for maintainers to take a look labels May 26, 2020
@bradzacher
Copy link
Member

Doing some research, it looks like this is intentional.
Clicking through the history for the lib def, you find microsoft/TypeScript#25374 which did exactly what we want!

However because Map has multiple type parameters, this caused weirdness with extending Map (microsoft/TypeScript#27951), which then lead to just the change to just the Map constructor being reverted (microsoft/TypeScript#28052).

So the map constructor being like this is completely intentional, which means we'll have to add a special case for new Map().

@bradzacher bradzacher added the good first issue Good for newcomers label May 26, 2020
@photonstorm
Copy link
Author

Thanks for the quick update. Sounds like this is quite the rabbit hole! I've sadly found I have had to disable all of the unsafe rules for my codebase because they seem to conflict with TS in subtle ways. I don't envy you having to work with such a moving feast as this! Will keep an eye out at the Change Log.

@bradzacher
Copy link
Member

they seem to conflict with TS in subtle ways

How so? If there are problems with the rule, please report them so we can look into getting them fixed!

@Raynos
Copy link

Raynos commented Jun 1, 2020

I ran into this issues today as well.

Would be lovely if Map can be special cased in @typescript-eslint because it's builtin into typescript stdlib definitions.

If I implement my own custom Generic MapLike and have an any signature in my own class definition then it's a "bug" in my constructor because I shouldn't use any.

Fixing TypeScript standard library for ES6 / etc to remove any is a large task ... so special casing Map in @typescript-eslint would be very practical.

@Raynos
Copy link

Raynos commented Jun 1, 2020

Here's an example workaround for JSDoc.

class Foo {
  constructor () {
    // https://github.com/typescript-eslint/typescript-eslint/issues/1943
    /* eslint-disable @typescript-eslint/no-unsafe-assignment */
    /** @type {Map<string, FunctionConfiguration[]>} */
    this._profiles = /** @type {Map<string, FunctionConfiguration[]>} */ (new Map())
    /* eslint-enable @typescript-eslint/no-unsafe-assignment */
  }
}

JSdoc does not support new Map<string, string> but does support casts ... I opened an issue in TypeScript for the lacking JSDoc functionality in function calls ( microsoft/TypeScript#38876 ).

bradzacher added a commit that referenced this issue May 15, 2021
… map constructor with no generics

Fixes #2109

Sucks that it had to be this way, but oh well.
@bradzacher bradzacher added the has pr there is a PR raised to close this label May 15, 2021
bradzacher added a commit that referenced this issue May 15, 2021
… map constructor with no generics (#3394)

Fixes #2109

Sucks that it had to be this way, but oh well.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working good first issue Good for newcomers has pr there is a PR raised to close this package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
3 participants