Fix(15506) - Disallow non-any/number/string operands on comparisons

[c0sta]

Fixes #15506 :

This PR aims to fix the #15506 ticket, here I based myself on previous PRs (and suggestions) that tried to resolve this same issue in the past (so I would say that it was co-authored by @feloy and @FabianLauer).

From what I understand, the main idea is that we should flag an error on every comparison (<, >, <= and >=) that is not between a string, number, or any types. Doing this, triggered a lot of new errors on the baselines (that seem correctly triggered assuming that I understand the issue correctly), but I would like to confirm that I got this right because it seems like a huge change to me.

I'm submitting the PR as a spike for now, just to gather suggestions, understand if this still a desired change and confirm that I'm following the right path here since this issue is kinda old and this is my first time contributing here.

[typescript-bot]

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

[c0sta]

@microsoft-github-policy-service agree

[typescript-bot]

The TypeScript team hasn't accepted the linked issue #15506. If you can get it accepted, this PR will have a better chance of being reviewed.

[c0sta]

This last commit fixes the Self Check action error by allowing compare Objects, Union, TypeParameter, and Intersection. That's because TS has some internal comparisons that use these types (already discussed here in the past). But I'm unsure if that's a good idea since it appears to go against the main issue purpose to disallow non-any, number, and string operands.

[jakebailey]

This last commit fixes the Self Check action error by allowing compare Objects, Union, TypeParameter, and Intersection. That's because TS has some internal comparisons that use these types (already discussed here in the past). But I'm unsure if that's a good idea since it appears to go against the main issue purpose to disallow non-any, number, and string operands.

I would expect that we would be recursing into those types to verify, and not ignore them. I'm pretty sure we have this sort of checking for other operators, so it feels weird that we are adding a whole new function to check this...

[typescript-bot]

Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @mjbvz, @zkat, and @joj for you. Feel free to loop in other consumers/maintainers if necessary.

[typescript-bot]

Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page.

Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up.

[c0sta]

Just some updates, I've searched for checkers that could make this comparison but I couldn't find it, so basically what I did was start using this isTypeAssignableTo. I'm just struggling a little bit to trigger the error on never and number comparisons but I'm working on this.

The Self Check error will come back now and after fixing the never and number comparison I'll look at them.

This last push brings a lot from the main branch with it, not sure if I'm doing something wrong here.

[jakebailey]

It seems like you incorrectly merged main, I'm afraid.

[c0sta]

It seems like you incorrectly merged main, I'm afraid.

Sorry about that, I did a reset on my 875169e commit, but if you think it's a better idea to open another PR I can do that too.
I changed my configs to use merge instead of rebase, it seems to be merging properly now.

[c0sta]

So I've worked to fix some of the self-check errors, most of them were related to Date comparisons so I started using .getTime() to convert the Date objects to numbers.

But I'm not sure how to go on this last error: Operator '<' cannot be applied to types 'string | number' and 'string | number'.. Since string and number are valid types for this operand, should this union keep triggering an error? and should it be possible to compare string with number?

[jakebailey]

How is this PR different from #52807?

compareComparableValues is also flagged by that PR for the same reason; it's unsafe but cannot actually be safe.

[c0sta]

How is this PR different from #52807?

compareComparableValues is also flagged by that PR for the same reason; it's unsafe but cannot actually be safe.

I don't think they are, they look the same to me. In that case, it's a better idea to just close this current PR?

[jakebailey]

Maybe; #52807 did not mention #15506 but was instead in the context of better handling valueOf which I don't think this PR does (but likely should, as it is part of a way to make it less breaky). I don't recall where we were with #52807 but @RyanCavanaugh may remember. I'm personally still hoping that'll be merged.