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

Check for === instead of == #3363

Closed
rockwotj opened this issue Jan 9, 2021 · 11 comments · Fixed by #3924
Closed

Check for === instead of == #3363

rockwotj opened this issue Jan 9, 2021 · 11 comments · Fixed by #3924

Comments

@rockwotj
Copy link

rockwotj commented Jan 9, 2021

Expected Behavior of the rule

Prevent usage of instance equality checks

Context

=== and == are both valid in kotlin. We also write a lot of typescript/javascript on our team (sadly no multiplatform kotlin) and a few of us accidently use === sometimes instead of == for equals. There should be a rule to prevent usage of === as you very rarely want instance equality checks anyways.

@schalkms
Copy link
Member

I suggest to create a custom rule here. Detekt tries to include rules that are relevant for the majority of the user base. IntelliJ also doesn’t flag the mentioned use case. If you have a lot of TypeScript developers in your team, a custom rule is the way to go.

https://detekt.github.io/detekt/extensions.html#custom-rulesets

@rockwotj
Copy link
Author

I think not using === is common to everyone, it just comes up frequently for us. Still, a custom rule it is

@schalkms
Copy link
Member

I think not using === is common to everyone

If you want to check the referential equality of two objects in Kotlin, you should use the === operator.
Please remember that this rule would also flag valid use cases for referential equality checks. Then the programmer could only add a suppress annotation to get rid of the warning.

it just comes up frequently for us

That's completely okay. Different teams have different needs and use cases. For that reason, detekt includes the possibility to write a custom rule. In this case, it should be fairly easy.
For questions related to custom rules please feel free to post a message.

@arturbosch arturbosch added this to the 1.16.0 milestone Jan 18, 2021
This was referenced Mar 11, 2021
@Marcono1234
Copy link
Contributor

Has such a rule actually been added? The changelog for 1.16.0 references this issue (see also all the linked pull requests above), however based on the comments it sounds like this rule has not been added.

Would it make sense to have such a rule at least for some standard types? Consider this broken example code:

fun main() {
    val other = "100" + "0".dropLast(0)
    println(other)
    println("1000" === other);
}

(That is a common bug pattern detected by many Java static code analysers, e.g. Sonar Source)

@cortinico cortinico removed this from the 1.16.0 milestone May 15, 2021
@cortinico
Copy link
Member

Has such a rule actually been added? The changelog for 1.16.0 references this issue (see also all the linked pull requests above), however based on the comments it sounds like this rule has not been added.

Nope, it has not. It was a mistake on our end to include this into the 1.16.0 milestone (that got then picked up during the release note generation).

Would it make sense to have such a rule at least for some standard types? Consider this broken example code:

I think the reasons mentioned by @schalkms still holds, also for your code sample.

(That is a common bug pattern detected by many Java static code analysers, e.g. Sonar Source)

Java has a lot more of confusions related to usages of .equals() and == (that's exactly what the rule you linked addresses). I believe Kotlin is already solving this issue at the language level (i.e. with the equals operator).

@Marcono1234
Copy link
Contributor

I think the reasons mentioned by @schalkms still holds, also for your code sample.

I understood their comment as response to the request for a general rule flagging all === usage, and I agree that this would likely result in too many false positives.
However, for some standard types, such as String (and maybe some other types?), even though there might valid use cases for === I think such cases are pretty rare (especially since String compile time constants are interned, so checking reference equality may behave unexpected in some cases).

Java has a lot more of confusions related to usages of .equals() and == (that's exactly what the rule you linked addresses). I believe Kotlin is already solving this issue at the language level (i.e. with the equals operator).

I am not that familiar with Kotlin, but to me it would appear that == and === for String are easier to confuse, and to overlook during code review.

@BraisGabin
Copy link
Member

I agree with @Marcono1234 that for Strings the usage of === is kind of unexpected. And using it with Int should return the same that ==. But it returns different things for Int?. So maybe it could have sense for some types. I don't like it as a general rule because there are some good reasons to use ===.

@rockwotj
Copy link
Author

I think there should probably just be a plugin for this check so folks can turn it on. I think it should be simple to add, but I just haven't found the time

@cortinico
Copy link
Member

So maybe it could have sense for some types. I don't like it as a general rule because there are some good reasons to use ===.

Agree, perhaps we can add a rule restricted to String only.

@BraisGabin BraisGabin reopened this May 20, 2021
@rockwotj
Copy link
Author

Agree, perhaps we can add a rule restricted to String only.

If there is work to support String then can it be a regex or config param that defaults to String? Then a team like mine would just change that to *

@BraisGabin
Copy link
Member

I don't see why not. But instead of regex I would use a list of glob. They are easier to write and we don't need all the power of regex here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants