Skip to content

UnusedPrivateMember doesn't report the correct warning count #1981

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

Closed
BraisGabin opened this issue Oct 4, 2019 · 9 comments · Fixed by #2500
Closed

UnusedPrivateMember doesn't report the correct warning count #1981

BraisGabin opened this issue Oct 4, 2019 · 9 comments · Fixed by #2500

Comments

@BraisGabin
Copy link
Member

BraisGabin commented Oct 4, 2019

Expected Behavior

class Test {
    private fun unusedFunction(): Int {
        return 5
    }
}

class Test2 {
    private fun unusedFunction(): Int {
        return 5
    }
}

In that code I expect to have two warnings because there are two unused functions

Observed Behavior

Only one warning is raised. The reason is becase the functions are called the same.

Steps to Reproduce

You can check this comparison: master...BraisGabin:unused-private-function

Context

I found this issue fixing a similar issue with parameters that are called the same in different functions. That issue was easier to fix that this one.

Your Environment

  • Version of detekt used: 1.0.1
  • Operating System and version: Mac
@BraisGabin BraisGabin changed the title UnusedPrivateMember doesn't report the correct number UnusedPrivateMember doesn't report the correct warning count Oct 5, 2019
@schalkms
Copy link
Member

schalkms commented Oct 6, 2019

This rule only flags unused functions/members/parameters in classes/interfaces/objects.
Therefore, it only checks members of classes/interfaces/objects.
This won't be included in this rule, since we would need to change the name and description of the rule.
Anyway, thanks for battle testing detekt while implementing the HTML report.
Your feedback is really valuable in that regard.

@schalkms schalkms closed this as completed Oct 6, 2019
@schalkms schalkms added the rules label Oct 6, 2019
@BraisGabin
Copy link
Member Author

Ok, I edited the snippet using just methods inside classes and the error is there. Can we reopen this?

@arturbosch
Copy link
Member

Yep, the rule currently counts the references to each name in a hashmap.
Storing the whole path taken from top level till the variable etc could solve this problem.
e.g.: ClassA>FunctionA>UnusedVar to KtElement instead of just UnusedVar to KtElement

@sowmyav24
Copy link
Contributor

Do we want to handle function overloading as part of this rule ?
Eg:

class Test {
    private fun unusedFunction(): Int {
        return 5
    }
   private fun unusedFunction(num: Int): Int {
        return num
    }
}

@BraisGabin
Copy link
Member Author

Good one! I'm going to create a test for this case too.

@BraisGabin
Copy link
Member Author

BraisGabin commented Oct 10, 2019

I updated the one proposed by @sowmyav24 and added two more (the other one is the same but with two functions):

class Test {
    val value = unusedFunction()

    private fun unusedFunction(): Int {
        return 5
    }

    private fun unusedFunction(num: Int): Int {
        return num
    }
}

If we have two functions that are called the same and one of them is used, the other one is not reported. I think that this case is a bit worse that the first that I reported. Here you are getting 0 findings.

@schalkms
Copy link
Member

Thanks for formulating new test cases and for investigating this issue further.

@BraisGabin
Copy link
Member Author

I think that to fix this issue we need type resolver. I tried to fix it but I found a wall. If I have this functions:

private fun f(a: Int)
private fun f(a: String)

I can't know if f("".randomExtensionFunction()) is calling the first or the second.

@schalkms
Copy link
Member

Yes!

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.

4 participants