Skip to content
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

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

Merged
merged 1 commit into from
Jun 12, 2023

Conversation

Jack-Works
Copy link
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
@typescript-bot
Copy link
Collaborator

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

@DanielRosenwasser DanielRosenwasser changed the title chore: add TS2839 to JS files Add check for reference-compared literals to JS files May 18, 2022
@sandersn
Copy link
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.

@Jack-Works
Copy link
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.

@Jack-Works
Copy link
Contributor Author

Add test files.

@sandersn sandersn self-assigned this May 26, 2022
@sandersn
Copy link
Member

A couple more examples from @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.

@Jack-Works
Copy link
Contributor Author

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

@sandersn
Copy link
Member

Yeah, I think so.

@Jack-Works
Copy link
Contributor Author

done

@Jack-Works
Copy link
Contributor Author

rebased

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Almost ready, just one more change.

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

@sandersn 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.

None yet

3 participants