Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Add strict-type-predicates rule #2046

Merged
merged 5 commits into from Jan 19, 2017

Conversation

andy-hanson
Copy link
Contributor

@andy-hanson andy-hanson commented Jan 15, 2017

PR checklist

  • Addresses an existing issue: #0000
  • New feature, bugfix, or enhancement
    • Includes tests
  • Documentation update
  • Enable CircleCI for your fork (https://circleci.com/add-projects)

What changes did you make?

Added the strict-type-predicates rule, which warns for typeof x === "string" where x is always (or never) a string, and for x === undefined where x is always (or never) undefined.

Is there anything you'd like reviewers to focus on?

Can't do instanceof checks yet as the type checker does not expose assignability checks. If it did, the getTypePredicateForKind function could be removed.

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [F]

get<number | undefined>() === undefined;
get<string | undefined>() == null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this rule also check for non strict comparisons to null or undefined if only one of them is possible?
Example:

get<string|undefined>() == null;
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [use === undefined instead]

get<string|undefined|null>() == null; // still allowed

get<string | undefined>() == null;
get<string | null>() == undefined;
get<string | null>() == null;
get<string | undefined>() == undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here the rule could suggest to use strict equality comparison

get<string | undefined>() == null;
get<string | null>() == undefined;
get<string | null>() == null;
get<string | undefined>() == undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test for the following:

get<null|undefined> == null;
~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [T]
get<null|undefined> != undefined;
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [F]

@@ -0,0 +1,210 @@
/**
* @license
* Copyright 2016 Palantir Technologies, Inc.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2017

@@ -178,6 +178,11 @@ export function isTypeFlagSet(type: ts.Type, flagToCheck: ts.TypeFlags): boolean
/* tslint:enable:no-bitwise */
}

/** Type predicate to test for a union type. */
export function isUnionType(type: ts.Type): type is ts.UnionType {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utils shouldn't be cluttered with wrappers for isTypeFlagSet

@andy-hanson
Copy link
Contributor Author

Would this rule make typeof-compare unnecessary?

@mohsen1
Copy link
Contributor

mohsen1 commented Jan 18, 2017

@andy-hanson if it catches typos like typeof foo === 'Object' it should...

Copy link
Contributor

@nchen63 nchen63 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nchen63
Copy link
Contributor

nchen63 commented Jan 18, 2017

sorry, can you merge with master?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants