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

Boolean["toString"] inconsistency #30225

Open
sirian opened this issue Mar 5, 2019 · 11 comments
Open

Boolean["toString"] inconsistency #30225

sirian opened this issue Mar 5, 2019 · 11 comments
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@sirian
Copy link
Contributor

sirian commented Mar 5, 2019

TypeScript Version: 3.4.0-dev.20190305

Code

const key: keyof Boolean = "toString"; // error - "toString" is not assignable to "valueOf"

const x: {toString(): string} = new Boolean(); // no error
const y: Boolean["toString"] = true.toString; // no error

class Bool extends Boolean { 
     protected toString() { // also no error, but should be
          return "";
     }
}

Expected behavior:
keyof Boolean should be "toString" | "valueOf"
Playground

@RyanCavanaugh
Copy link
Member

Apparent properties (such as toString) intentionally aren't included in keyof or inheritance validity checks, so some inconsistencies between keyof and property access is expected and intentional.

I think we could safely add toString to interface Boolean, though I am extremely curious how you came to notice this in the first place.

@sirian
Copy link
Contributor Author

sirian commented Mar 5, 2019

@RyanCavanaugh you wrong. keyof of any other type includes toString. For example: keyof Number, keyof String. even keyof Symbol

@RyanCavanaugh
Copy link
Member

you wrong. keyof of any other type

No, I right.

interface Foo { m; }
// Error
const k: keyof Foo = "toString";

Unlike Boolean, Number, String, and Symbol have explicitly-declared toString methods in their corresponding interface, which is why toString is in their keyofs, which is why I proposed adding toString to Boolean's interface.

@DanielRosenwasser DanielRosenwasser added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Mar 5, 2019
@sirian
Copy link
Contributor Author

sirian commented Mar 6, 2019

@RyanCavanaugh ok. so the same problem with RegExp, Date, ... And now there is inconsistency:
image
playground

@domenic
Copy link

domenic commented Sep 12, 2019

@jacksteinberg and I have noticed this in a project that is using the TypeScript definitions. We have so far identified the following missing declarations:

In contrast, others overrides of Object.prototype methods are included in the TypeScript definitions, e.g. Number.prototype.toString, Boolean.prototype.valueOf, Array.prototype.toLocaleString.

We haven't done an exhaustive search; those were just particular instances of overridden toStrings that we were trying to pick up with our tool (but ended up getting the wrong result due to the missing TypeScript definitions). In particular I wonder if other stringifiers in Web IDL interfaces are not being picked up correctly.

/cc @saschanaz as someone who has been helpful keeping TypeScript's built-in type definitions up to date in the past.

@saschanaz
Copy link
Contributor

saschanaz commented Sep 12, 2019

Corresponding issue on TSJS-lib-generator: microsoft/TypeScript-DOM-lib-generator#322.

The emitter currently does not do anything for stringifier and only provides the inherited toString.

We could generate toString for every interfaces but that would be a big noise and duplication, maybe we should just include toString in keyof?

@domenic
Copy link

domenic commented Sep 13, 2019

I think it'd be best to include toString for the interfaces which have specific toString behavior that differs from Object.prototype. There are not many of those. (I'd guess 5 in the JS spec, 20ish on the web platform.)

This already seems to be the policy, e.g. Array.prototype.toString and Number.prototype.toString are included. There are just some that got missed for whatever reason.

@saschanaz
Copy link
Contributor

I think it'd be best to include toString for the interfaces which have specific toString behavior that differs from Object.prototype.

Definitely can be done, but what would be the use cases? Couldn't understand the purpose of jackbsteinberg/get-originals-rewriter#79.

@domenic
Copy link

domenic commented Sep 13, 2019

Abbreviated summary: we're using the TypeScript type system to transform calls of the form x.toString() into AppropriateClass.prototype.toString.call(x). Without accurate information about the methods in x's prototype chain, this is not possible.

In general, anything which is relying on the TypeScript definitions giving an accurate summary of what methods a class has will encounter this problem.

@askirmas
Copy link

askirmas commented Oct 16, 2019

Object.prototype.toString is a native function that returns '[object Object]'. So shouldn't be errors like

  • Type '{ toString: () => string; }' is not assignable to type 'string'

because it is JS core feature.
PS And we didn't start to talk about Symbol.toPrimitive

@JoshuaKGoldberg
Copy link
Contributor

typescript-eslint/typescript-eslint#4999 hit this in typescript-eslint-land.

We take an approach similar to #30225 (comment) the @typescript-eslint/no-base-to-string. Error objects are being flagged by the rule for not having a toString, but that's incorrect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

7 participants