-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Ensure that Comparers only return -1, 0, 1 so results can be equated #52897
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
Conversation
@typescript-bot perf test this |
Heya @jakebailey, I've started to run the perf test suite on this PR at 2106d54. You can monitor the build here. Update: The results are in! |
Does this catch any actual bugs? Should we just change the comparer type to return |
No, it doesn't, and that surprised me. If we want to do that, the binary search API needs to change to not check equality on |
Well, it did catch one bug; When I was working on #52891, I accidentally passed I think that if we had any existing bugs related to this, they'd already have been caught, because the result is pretty catastrophic. |
@jakebailey Here they are:
CompilerComparison Report - main..52897
System
Hosts
Scenarios
TSServerComparison Report - main..52897
System
Hosts
Scenarios
StartupComparison Report - main..52897
System
Hosts
Scenarios
Developer Information: |
Interestingly, 5.0 nightly catches the case where the comparer returns a literal |
Yeah, then then leftover one is sort of the "known hole" as it has uses. Unfortunate. |
I've always wondered why we use |
Are there places in the code where we're doing a binary search on imports that we haven't previously checked are sorted? IIRC, that's the purpose of We have public "branded" types like |
I doubt it. But I think it's a little easier to understand.
Even so, I do like that even if you don't use the types always, you are forced to cast to a sorted array of you need to use them (depending), which does make one think twice about whether or not the array is actually sorted or not. |
I'd almost rather have a data structure that encapsulates both the sorted array and the comparer, though I'm not sure that's going to be as efficient. |
I noticed this while working on cleaning up our (inconsistent) use of the binary search and ordered insert APIs.
The binary search API requires that you have comparisons of exactly
-1 | 0 | 1
, however, we had some comparers which didn't do this thanks to a hole in our type system (Playground Link).Make it so that we consistently return only one of the three valid values, so comparers can always be used in all APIs.
I also noticed (but did not change) the fact that we use binary searches for things like import insertion with the assumption that the input is always sorted, but, I don't think this actually turns out to be the case all the time anymore?