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

Removed UnnecessaryAbstractClass if it inherits from a abstract class #5009

Merged
merged 6 commits into from Jul 10, 2022

Conversation

gouri-panda
Copy link
Contributor

@gouri-panda gouri-panda commented Jun 29, 2022

closes: #4753

Removed UnnecessaryAbstractClass if it inherits from abstract class an interface or that has concrete members.

@@ -73,8 +73,7 @@ class UnnecessaryAbstractClassSpec(val env: KotlinCoreEnvironment) {
}
abstract class B : A
"""
val findings = subject.compileAndLintWithContext(env, code)
assertFindingMessage(findings, message)
assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
Copy link
Collaborator

@BraisGabin BraisGabin Jun 29, 2022

Choose a reason for hiding this comment

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

This test shouldn't change, right?

Copy link
Contributor Author

@gouri-panda gouri-panda Jun 29, 2022

Choose a reason for hiding this comment

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

@BraisGabin If we don't change it, the tests will fail. I don't understand your question.

Copy link
Collaborator

@BraisGabin BraisGabin Jun 29, 2022

Choose a reason for hiding this comment

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

But this one was right. An abstract class that only extends an interface is an unnecessary abstract class. The only one that should change is the case where an abstract class extend another abstract class because there you can't use an interface. (maybe I'm missing something here)

Copy link
Contributor Author

@gouri-panda gouri-panda Jun 30, 2022

Choose a reason for hiding this comment

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

Should I revert that?

Copy link
Collaborator

@BraisGabin BraisGabin Jun 30, 2022

Choose a reason for hiding this comment

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

Yes. This test shouldn't change.

Copy link
Member

@schalkms schalkms left a comment

Thanks for the fix - appreciate it!

@schalkms
Copy link
Member

schalkms commented Jun 29, 2022

@gouri-panda please try to rebase this branch and fix the following rule violation. Then this PR is good to go. 👍

UnnecessaryAbstractClass.kt:83:25: Function check has 3 return statements which exceeds the limit of 2. [ReturnCount]

@@ -98,6 +98,7 @@ class UnnecessaryAbstractClass(config: Config = Config.empty) : Rule(config) {
report(CodeSmell(issue, Entity.from(nameIdentifier), noConcreteMember))
}
}
hasInheritedMember(true) -> return
Copy link
Contributor

@amitdash291 amitdash291 Jul 2, 2022

Choose a reason for hiding this comment

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

The following enhancement to this logic will ensure the rule still fails for interface super types (as discussed in @BraisGabin's comment):

hasInheritedMember(true) && !isParentInterface() -> return

private fun KtClass.isParentInterface() =
    (bindingContext[BindingContext.CLASS, this]?.unsubstitutedMemberScope as? LazyClassMemberScope)
        ?.supertypes?.firstOrNull()?.isInterface() == true

Copy link
Contributor Author

@gouri-panda gouri-panda Jul 3, 2022

Choose a reason for hiding this comment

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

@amitdash291 Thanks for advice and code :)

Copy link
Contributor

@amitdash291 amitdash291 Jul 4, 2022

Choose a reason for hiding this comment

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

I think there's a corner case to test here. What if the class implements an interface and inherits from an abstract class in that order. For eg the following may fail the rule whereas it's expected to pass:

interface I

abstract class A
// some abstract fields here

abstract class B: I, A()

cc @gouri-panda @BraisGabin

Copy link
Collaborator

@BraisGabin BraisGabin Jul 5, 2022

Choose a reason for hiding this comment

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

Is that even valid Kotlin code? Shouldn't the class be defined the first one? I can't check right now. Anyway, a test extending both, a class and an interface seems like a really good case to have a test. @guori-panda, can you add such test?

Copy link
Contributor

@amitdash291 amitdash291 Jul 5, 2022

Choose a reason for hiding this comment

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

@BraisGabin yes interestingly this is valid Kotlin code, given below is the test case (that compiles):

@Test
fun `does not report abstract class that inherits from an interface and an abstract class in that order`() {
    val code = """
        interface I

        @Deprecated("We don't care about this first class")
        abstract class A {
            abstract val i: Int
        }
        
        abstract class B: I, A()
    """
    val findings = subject.compileAndLintWithContext(env, code)
    assertThat(findings).isEmpty()
}

…abstract class that has concrete members

Removed deprecated annotations

Revert "Removed deprecated annotations"

This reverts commit 326a356

fix return count rule violation
Copy link
Contributor

@amitdash291 amitdash291 left a comment

Refer reply to the code suggestion earlier.

@cortinico cortinico added this to the 1.21.0 milestone Jul 5, 2022
@BraisGabin
Copy link
Collaborator

BraisGabin commented Jul 6, 2022

@gouri-panda sorry for all the changes. Can you fix the conflicts and add the tests that @amitdash291 point out here: #5009 (comment) ?

After that (and if that test passes) I think that this is ready to merge.

@gouri-panda
Copy link
Contributor Author

gouri-panda commented Jul 6, 2022

@BraisGabin @amitdash291 Sure :)

gouri-panda added 2 commits Jul 7, 2022
…abstract class that has concrete members

Removed deprecated annotations

Revert "Removed deprecated annotations"

This reverts commit 326a356

fix return count rule violation
# Conflicts:
#	detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryAbstractClassSpec.kt
Copy link
Collaborator

@BraisGabin BraisGabin left a comment

Thanks for the updates :). This still has conflicts, could you check that?

abstract class A {
abstract val i: Int
}
abstract class B: A(), I
Copy link
Collaborator

@BraisGabin BraisGabin Jul 7, 2022

Choose a reason for hiding this comment

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

Suggested change
abstract class B: A(), I
abstract class B: A(), I
abstract class C: I, A()

We should check both cases.

Copy link
Contributor Author

@gouri-panda gouri-panda Jul 7, 2022

Choose a reason for hiding this comment

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

Sure! Let me add another test case as well!

Copy link
Contributor

@amitdash291 amitdash291 Jul 9, 2022

Choose a reason for hiding this comment

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

@gouri-panda I have an idea to solve this in a generic manner. If you're okay I can raise a PR on your fork branch gouri-panda:fix-4753?

Copy link
Contributor Author

@gouri-panda gouri-panda Jul 9, 2022

Choose a reason for hiding this comment

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

I was thinking if we check one of the parent is abstract, then we can return. But go for it :)

Copy link
Contributor

@amitdash291 amitdash291 Jul 9, 2022

Choose a reason for hiding this comment

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

@gouri-panda
I have pushed my changes as a PR to your fork's branch here
In summary I have checked if not all the parents are interfaces, which is equivalent of the fact that at least one parent is abstract.
Please refer to this commit (there are a few other merge commits)

Handle corner case of multiple inheritance in UnnecessaryAbstractClass rule
@BraisGabin
Copy link
Collaborator

BraisGabin commented Jul 10, 2022

Thanks for the fix @gouri-panda and @amitdash291

@BraisGabin BraisGabin merged commit 1b330e9 into detekt:main Jul 10, 2022
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants