[experiment] Don't relate primitives to their wrappers #52947
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
|
@typescript-bot test this |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 5211178. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 5211178. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at 5211178. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 5211178. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 5211178. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the abridged perf test suite on this PR at 5211178. You can monitor the build here. Update: The results are in! |
This comment was marked as outdated.
This comment was marked as outdated.
|
@jakebailey Here are the results of running the user test suite comparing Something interesting changed - please have a look. Details |
|
Heya @jakebailey, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
|
@jakebailey Here they are:Comparison Report - main..52947
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
It's the opposite - const a: number = new Number(0); // error
const b: Number = 0; // allowed - structurally compatible |
Yep, sorry. Flipped it when writing! |
| if (source !== originalSource && originalFlags & TypeFlags.HasWrapper && !(sourceFlags & TypeFlags.HasWrapper) && source === target) { | ||
| return Ternary.False; | ||
| } |
There was a problem hiding this comment.
I actually am not sure if you want to disallow primitives from being assignable and comparable to wrapper objects. I think there's at least some rationale that you actually want to allow 1 as Number even if that is a bit silly - but I can't imagine why.
Given that, I think the right place for this is an ad-hoc check in === and !==, and possibly == and != too. But this is fine for prototyping.
There was a problem hiding this comment.
That's true, I think I misinterpreted your suggestion.
There was a problem hiding this comment.
It's not the craziest idea - we highly discourage using the wrapper objects anyway.
|
@jakebailey Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
|
driveby q: what does "comparable" really mean? |
|
Comparability is a relaxed form of assignability. It aims to answer "is this plausibly of the same type" instead of "is this pretty much substitutable". The biggest difference used to just be that we check if any member of a union type source is related to a target, instead of all members of a union type source. The second biggest difference was around optional properties. It's used for
See #5517 for the original PR. The biggest motivator was #6167. See #17214 for an oldish list of differences between comparability/assignability. |
|
Fwiw I really like the result here. I almost think we should re-allow comparing number and Number before 5.0 goes out, and then do this PR instead for 5.1 along with the valueOf change, because comparison is really the only safe thing you can do with wrapper types. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
The cure might worse than the disease here. I'm nervous that this introduces subtyping commutativity violations, and does so nominally.
|
|
I should disclaim that this is definitely not the final iteration of this PR, but something I threw together late at night. Probably we could just do the conservative thing of not allowing |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
@typescript-bot pack this |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 17bda87. You can monitor the build here. |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
I always thought the reason |
|
Yep, that's right (what Ryan was saying above). In this case, we get the "apparent" type which is the wrapper and has all of the properties and methods, and that's structurally compared. |
|
I'm going to close this as this isn't the kind of approach we'd be taking. It also didn't seem to catch any bugs, just people using I might come back to doing this like |
Basically, I was trying to understand why we allow
const x: Number = 1when the latter is a wrapper object. e.g.1 === new Number(1)will yield false andtypeof new Number(1) === "object", notnumber.@DanielRosenwasser said this might be an interesting experiment. Might as well try.