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

Number.isInteger and others should accept any and act as type guards #21199

Closed
cyco130 opened this issue Jan 16, 2018 · 6 comments
Closed

Number.isInteger and others should accept any and act as type guards #21199

cyco130 opened this issue Jan 16, 2018 · 6 comments
Labels
Duplicate An existing issue was already created

Comments

@cyco130
Copy link

cyco130 commented Jan 16, 2018

TypeScript Version: 2.7.0-dev.20180116

Code

function ensureInt(x: number | string): number {
    if (Number.isInteger(x)) { // Same with isSafeInteger, isNan and isFinite
        return x;
    } else {
        throw Error("Type error");
    }
}

Expected behavior:
It should compile without errors.

Actual behavior:
The compiler complains as follow:

file.ts(2,26): error TS2345: Argument of type 'string | number' is not assignable to parameter of type 'number'.
  Type 'string' is not assignable to type 'number'.
file.ts(3,9): error TS2322: Type 'string | number' is not assignable to type 'number'.
  Type 'string' is not assignable to type 'number'.

Proposed solution:
Rewrite es6 type definition of NumberConstructor.isInteger (et al.) like this:

isInteger(number: any): number is number;
@jcalz
Copy link
Contributor

jcalz commented Jan 16, 2018

Seems like one of many duplicates of #4002, and maybe a duplicate of #19557, and another opportunity to think about how to prevent such duplicates. Maybe the text under <!-- BUGS: Please use this template. --> in the issue reporting template should be very explicit about how to check for existing issues? (Instead of just relying on the link at the top saying "read the contributing guidelines"?)

You don't want anything to act as arg is T unless true means it is and false means it isn't. Do you want Number.isInteger(0.5) === false to imply that 0.5 is not a number?

@ghost ghost added the Duplicate An existing issue was already created label Jan 16, 2018
@cyco130
Copy link
Author

cyco130 commented Jan 16, 2018

I see, it makes sense. I did search for isInteger before submitting but it didn't occur to me to search for isNaN etc.

@jcalz
Copy link
Contributor

jcalz commented Jan 16, 2018

Sure, it happens.

The relevant advice in the contributing guidelines says "Check for synonyms. For example, if your bug involves an interface, it likely also occurs with type aliases or classes." (I guess the word "synonyms" should probably be replaced with "related terms", since the example isn't showing literal synonyms anyway.)

The question is: is the stuff in that document the best we can do to guide potential issue-filers to find potential duplicates without scaring them away entirely? A prominent warning like "You're probably not the first person to encounter this issue; you'd better spend at least an hour searching through the existing issues for every possible related term before filing; if you file a duplicate, we will send monsters to devour you." would probably cut down on the false positives, but false negatives are bad too. Oh well, who knows?

@cyco130
Copy link
Author

cyco130 commented Jan 16, 2018

Heh, thanks for the advice. I'll make sure to search for at least an hour before requesting something like isInteger(n: any): implies n is number :)

@cyco130 cyco130 closed this as completed Jan 17, 2018
@RyanCavanaugh
Copy link
Member

@jcalz didn't quite go as far as what you have up there, but #21238 puts some tough love in the template

@MicahZoltu
Copy link
Contributor

MicahZoltu commented May 27, 2018

I opened #24436 without finding this (apparently I also searched for the wrong keywords). However, this (and #24436) are not duplicates of #4002. Despite the similarity in naming, Number.isFinite and isFinite are not the same function and all of the arguments made in #4002 do not apply to the functions on the Number constructor. If this (and #24436) are to remain closed, I would like to see new arguments formulated/expressed as to why to ensure they aren't being closed just because they look like duplicates of #4002.

@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

4 participants