FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add check for reference-compared literals to JS files by Jack-Works · Pull Request #49164 · microsoft/TypeScript · GitHub

Add check for reference-compared literals to JS files - #49164

Merged
Nathan Shively-Sanders (sandersn) merged 1 commit into
microsoft:mainfrom
Jack-Works:allow-2839-in-js
Jun 12, 2023
Merged

Add check for reference-compared literals to JS files#49164
Nathan Shively-Sanders (sandersn) merged 1 commit into
microsoft:mainfrom
Jack-Works:allow-2839-in-js

Conversation

Copy link
Copy Markdown
Contributor

I don't know where I can add a test for JS files.

According to discussion in #45978, I open this PR to also check JavaScript files.

TypeScript Bot (typescript-bot) added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label May 18, 2022

Copy link
Copy Markdown
Contributor

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

Daniel Rosenwasser (DanielRosenwasser) changed the title chore: add TS2839 to JS files Add check for reference-compared literals to JS files May 18, 2022

Copy link
Copy Markdown
Member

Plain JS tests are in $ts/tests/cases/conformance/salsa/plainJS*. You can get the settings you need to test them from the existing tests.

A couple of gotchas I thought of:

  • "[object Object]" == {}
  • { toString() { return 'hi' }, valueOf() { return 'no' } } == { toString() { return 'no' }, valueOf() { return 'hi' } }

At least, I think there are shenanigans possible with valueOf; I couldn't get a working example.

Copy link
Copy Markdown
Contributor Author

I think there's no need to support the first case. The second case is a bit more interesting, maybe I can add detection to check if the literal contains toString, valueOf, or any computed property name only in JS.

Copy link
Copy Markdown
Contributor Author

Add test files.

Copy link
Copy Markdown
Member

A couple more examples from Brad Zacher (@bradzacher)'s comment:

[1,2,3] == '1,2,3'
// -> true

({toString() { return 1 }}) == 1
// -> true

The second one is pretty close to the second example in my comment but arguably more likely to be written as a weird workaround for something. Even a weird workaround shouldn't have an error in JS given the reaction of some JS users when an unavoidable red squiggly shows up.

Copy link
Copy Markdown
Contributor Author

So we only emit it for === not for ==?

Copy link
Copy Markdown
Member

Yeah, I think so.

Copy link
Copy Markdown
Contributor Author

done

Copy link
Copy Markdown
Contributor Author

rebased

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Almost ready, just one more change.

It's been a long time, so if you want me to finish this, let me know.

Comment thread src/compiler/checker.ts Outdated
Nathan Shively-Sanders (sandersn) merged commit e60cf12 into microsoft:main Jun 12, 2023
Jack Works (Jack-Works) deleted the allow-2839-in-js branch June 13, 2023 01:30
Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 22, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

For Uncommitted Bug PR for untriaged, rejected, closed or missing bug

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants


Back | FazBrowse Home | New Git URL