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

Dont check WrongEqualsTypeParameter if the function is topLevel #2027

Merged
merged 1 commit into from
Oct 19, 2019
Merged

Dont check WrongEqualsTypeParameter if the function is topLevel #2027

merged 1 commit into from
Oct 19, 2019

Conversation

BraisGabin
Copy link
Member

You can have a function named equals as a top level function. It's strange but safe.

@codecov-io
Copy link

codecov-io commented Oct 16, 2019

Codecov Report

Merging #2027 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2027   +/-   ##
=========================================
  Coverage     80.68%   80.68%           
- Complexity     1985     1986    +1     
=========================================
  Files           329      329           
  Lines          5591     5591           
  Branches       1021     1021           
=========================================
  Hits           4511     4511           
  Misses          539      539           
  Partials        541      541
Impacted Files Coverage Δ Complexity Δ
...osch/detekt/rules/bugs/WrongEqualsTypeParameter.kt 100% <100%> (ø) 8 <4> (+1) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d99550d...ed111e8. Read the comment docs.

@3flex
Copy link
Member

3flex commented Oct 17, 2019

I think this rule should instead be changed to instead only equals() functions that are overrides (similar to EqualsAlwaysReturnsTrueOrFalse rule) instead of checking if it's a top level function.

I don't think a top-level equals() function would ever have to fulfil this contract.

@BraisGabin
Copy link
Member Author

I think that maybe can be reformuled to: "any non-top-level function called equals that has only one parameter should have the modifier override". The reason is that otherwise we are overloading the equals function and that's exactly what this rule is about. If you agree with this definition I can rewrite this rule.

@3flex
Copy link
Member

3flex commented Oct 18, 2019

What I meant was, the rule would check for a function named "equals" that is an override. If those conditions are true, then the rule would check that the return type is as expected.

If the equals function is not an override then this rule shouldn't flag an issue.

@BraisGabin
Copy link
Member Author

If the equals function is not an override then this rule shouldn't flag an issue.

Yes, it should. Think on this code:

fun main(args: Array<String>) {
    A().equals("")  // the output is "wrong"
}

private class A {

    fun equals(other: String): Boolean {
        println("wrong")
        return !super.equals(other)
    }

    override fun equals(other: Any?): Boolean {
        println("correct")
        return super.equals(other)
    }
}

The first equals function doesn't have the override modifier but it's overloading the equals(Any?). So this can lead to very ugly bugs. This is what this rule is cheking.

@3flex 3flex merged commit 4790a9b into detekt:master Oct 19, 2019
@3flex
Copy link
Member

3flex commented Oct 19, 2019

Thanks for your patience and contribution!

@arturbosch arturbosch added this to the 1.2.0 milestone Oct 28, 2019
smyachenkov pushed a commit to smyachenkov/detekt that referenced this pull request Dec 9, 2019
smyachenkov pushed a commit to smyachenkov/detekt that referenced this pull request Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants