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

Add 'no instanceof' config switch #16693

Closed
AsherBarak opened this issue Jun 22, 2017 · 5 comments
Closed

Add 'no instanceof' config switch #16693

AsherBarak opened this issue Jun 22, 2017 · 5 comments
Labels
Out of Scope This idea sits outside of the TypeScript language design constraints Suggestion An idea for TypeScript

Comments

@AsherBarak
Copy link

The way instanceof works presents a potential pitfall for ts developers. An instance passing ts type check may (or may not) fail instanceof check depending on the way it was constructed:

class A {
    a: string;
}

let a1: A = new A();
let a2: A = { a: 'test' };

console.log(a1 instanceof A); // true
console.log(a2 instanceof A); // false

While this may not be avoidable given the relationship between js and ts, it might be useful to have the compiler enforce a policy where using instanceof is simply disallowed.

@kitsonk
Copy link
Contributor

kitsonk commented Jun 22, 2017

Since this is valid JavaScript and would be a coding convention, like using let or const over var, it would be best served by a linter rule than being baked into TypeScript, in my opinion. While TypeScript does dabble in erroring on certain patterns, like code after return, it generally constrains itself to code that is undesirable at runtime. While there are challenges with the structural type system of TypeScript and the instanceof operator, it is still the only valid way of narrowing certain types and constructs and has a role to play.

@DanielRosenwasser DanielRosenwasser added the Out of Scope This idea sits outside of the TypeScript language design constraints label Jun 22, 2017
@DanielRosenwasser
Copy link
Member

Agreed with @kitsonk - disallowing these sorts of constructs is not something we generally do.

@jcalz
Copy link
Contributor

jcalz commented Jun 22, 2017

I assume instanceof is safe in this direction:

a1.a = 'hello';
function safe(x: any) {
    if (x instanceof A) {
        return x.a;  // safe
    }
    return 'who knows';
}
safe(a1); // 'hello'
safe(a2); // 'who knows'

but not safe in this direction:

function unsafe(x: A | string) {
    if (x instanceof A) {
        return x.a.concat('!');
    }
    return x.concat('!'); // not necessarily
}
unsafe(a1); // 'hello!'
unsafe(a2); // TypeError: x.concat is not a function

It would be nice if any linter warning didn't flag the first usage.

@kitsonk
Copy link
Contributor

kitsonk commented Jun 22, 2017

That would be up to the linter. tslint has access to the language services and could try to determine what sort of narrowing is going on, but it sounds rather complex, but the example does show perfectly safe use of instanceof in TypeScript. 😁

@RyanCavanaugh RyanCavanaugh added the Suggestion An idea for TypeScript label Jun 22, 2017
@AsherBarak
Copy link
Author

AsherBarak commented Jun 22, 2017

@jcalz In your second example the difference between the way ts perceives A and the way js handles instaceof confuses the ts compiler to thinking the A case of the input parameter has already been handled. I did not think of that, and (@kitsonk, @DanielRosenwasser, @RyanCavanaugh) I think that is something that might be better handled in TypeScript compiler.
I was aiming to your first example and I think that, while being safe, its is misleading. A programmer might be surprised to learn that a perfectly good instance of A running around in his code returns who knows while others return the value of a.a. This, however, would probably be better as ts-lint rule palantir/tslint#2943

@mhegazy mhegazy closed this as completed Nov 20, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Out of Scope This idea sits outside of the TypeScript language design constraints Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

6 participants