feat: support error when comparing with object/array literals

[Jack-Works]

[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.

[sandersn]

@Jack-Works please create an issue to track this. I believe @DanielRosenwasser has some experience about what issues we need to consider for adding a check like this.

[DanielRosenwasser]

Honestly, if the thing on either side is an explicit literal, I don't think there's ever an issue with providing an error.

[Jack-Works]

Do I still need to create an issue since @DanielRosenwasser replied?

[sandersn]

Request for one more test

(And I learned that this applies to ==, not just ===)

[Jack-Works]

Request for one more test

(And I learned that this applies to ==, not just ===)

What test do you want to have?

Yes, it applies to == to, although JavaScript can implicit doing a type coherence make the comparison true, TypeScript doesn't consider that case already so I guess it is ok.

if(0==false){// This condition will always return 'false' since the types 'number' and 'boolean' have no overlap.(2367)// but actually true in JavaScript}

[sandersn]

I want to test something that is (1) an existing type error (2) the new object/array literal error:

constb=[1]if({a : 1}==b){}

That's the only addition.
My other comments were just about my surprise at learning that == doesn't coerce to string like I thought it did.

[Jack-Works]

I have resolved the conflict, can you give another review? thanks, @sandersn@DanielRosenwasser

[Jack-Works]

Thanks! I have rebased again to fix merge conflicts.

[sandersn]

Looks like a couple of tests still need their baselines updated.

[DanielRosenwasser]

Should we...always show this to JavaScript users?

[Jack-Works]

Should we...always show this to JavaScript users?

Of course, should I do anything to enable this in JavaScript?

[sandersn]

@Jack-Works there is a set of errors to show to JS users in src/compiler/program.ts .. plainJSErrors, I think. Add it there and it will show up for all JS users. There might be cases where ==could succeed that might not be appropriate to show, however, so the check might need to be more complex for JS files.

[Jack-Works]

@Jack-Works there is a set of errors to show to JS users in src/compiler/program.ts .. plainJSErrors, I think. Add it there and it will show up for all JS users. There might be cases where ==could succeed that might not be appropriate to show, however, so the check might need to be more complex for JS files.

There're only binder errors and grammar errors in plainJSErrors. Is it ok to add type errors?

[sandersn]

Technically, yes. Grammar errors already come from the checker.
Practically, we need to be really certain the error is correct in order to show it to Javascript users. Syntax and grammar errors are good because they're always runtime errors. I would need to re-read the spec on == to understand whether that's true there.

I vote that we do this for 4.9, after Typescript users have had a chance to report any problems with it. In the past, JS users have gotten irate when code they know to be correct gets a TS error put on it.

[Jack-Works]

I can't think any case that == will return true for either side is literal object 🤔 I will open a PR for it

[DanielRosenwasser]

It could be surfaced as a suggestion, right? I do agree that we should be a little conservative on things like this for JS users.

[Jack-Works]

It could be surfaced as a suggestion, right? I do agree that we should be a little conservative on things like this for JS users.

But that's definitely a mistake even you're using JS 🤔

[sandersn]

Also, suggestions ought to provide a codefix. And their default UI in vscode is extremely subtle.

[Jack-Works]

In this case I cannot provide a code fix because it really depends on the intension of the code 🤔

[sandersn]

I think we should ship this as an error in 4.9 for JS, perhaps with some more restrictions to cut down on false positives.

[Jack-Works]

Can you give an example of false positives on this error? 😂 I can't think any of it

[sandersn]

I commented on the PR. So far they all look like intentionally bad code, so there might not be anything to do.

[bradzacher]

I can't think any case that == will return true for either side is literal object 🤔

@Jack-Works

[1,2,3]=='1,2,3'// -> true({toString(){return1}})==1// -> true

== is super sketchy due to its coercion
IIRC the algorithm is something like:

• if both sides are objects, compare by reference
• if both sides are the same type, compare exactly
• else stringify both sides and compare the strings