UnnecessaryAbstractClass false positive

[TWiStErRob]

Expected Behavior

Don't report following code:

abstract class Fake : Base() // UnnecessaryAbstractClass
abstract class Base {
	open val order: Int get() = 0
	abstract fun String.foo()
}

Observed Behavior

"An abstract class without a concrete member can be refactored to an interface."

No, it can't:

• Fake interface couldn't extend a class.
• it has concrete members inherited from Base
• I get a detection even if I add an actual class using the "abstractness":

class Foo : Fake() {
	override fun String.foo() = TODO("not implemented")
}

Note: String. might be a red herring, you can ignore that part, just wanted to report what I had. More minimal repro might exist.

Steps to Reproduce

Just copy above code into a test or code checked by Detekt 1.20.0.

Context

Real context is creating fakes for a test:

The abstract method is never invoked in the test, so there's no need to implement it, hence using abstract class. The class must be unique because it's ::class object is used.

Your Environment

• Version of detekt used: 1.20.0
• Version of Gradle used (if applicable): N/A
• Gradle scan link (add --scan option when running the gradle task): N/A
• Operating System and version: all
• Link to your project (if it's a public repository): TWiStErRob/net.twisterrob.cinema@0a43c8e in Update Detekt monorepo from 1.19.0 to 1.20.0 (minor) TWiStErRob/net.twisterrob.cinema#114

[BraisGabin]

Isn't this an edge case? I mean, in general a class that only extends another class and does nothing is unnecessary, you could use the base class. In this case the code uses the type but that seems to me like an edge case.

I'm not against this change but I'm worried that by allowing this we will add a lot of false negatives.

[TWiStErRob]

If you build an expression hierarchy, or similar, you might introduce an intermediate base class for grouping part of the class hierarchy.

[dawidhyzy-ifolor]

We face the same issue when wrapping a third party library classes so it's not an edge case.

[BraisGabin]

I agree, maybe we could add a configuration here but I don't think that it could be even needed. Do you have bandwith to provide a PR?

[TWiStErRob]

Me, not for a month probably.

@dawidhyzy-ifolor can you please share your use case in more detail, it might help with unit tests.

[dawidhyzy-ifolor]

Sure!

We have DataSource defined in the domain layer

abstract class PhotosMediaItemPagedDataSource : PagingSource<NextPageToken, MediaItem>() {

    interface Factory {
        fun create(albumId: AlbumId?, pageSize: Int?): PhotosMediaItemPagedDataSource
    }
}

As androidx.paging.PagingSource is an abstract class we can't refactor PhotosMediaItemPagedDataSource to an interface - interface can't extend abstract class.

Please, let me know if you need more information.

[chao2zhang]

I think the rule of thumb is because Interface can't extend abstract class. I agree that we should at least provide a configuration to this rule.

[gouri-panda]

@BraisGabin May i take this?

[BraisGabin]

Go ahead

[gouri-panda]

@BraisGabin Interface can't extend class too. Should we make configuration that too?

[BraisGabin]

I'm fine with or without configuration. I suppose that in that case without is better because it keeps the api cleaner but that's your call. I think that no one here have a hard opinion about this topic.

[TWiStErRob]

Ask yourself if there's a possibility for the case you're flagging. Is it possible to write code that passes/fails the rule? And are both of them valid engineering choices? (i.e. due to decisions on preferences or restricted by constraints)

[EdwarDDay]

This issue is closed, but it still happens in 1.21.0 (gradle plugin). Am I doing something wrong?
Example run: https://github.com/EdwarDDay/detekt-abstract-class-unnecessary/runs/7410220388?check_suite_focus=true
Example code:

abstract class Foo {
    abstract val i: Int
    val j = 1
}

abstract class Bar : Foo() // UnnecessaryAbstractClass issue

[TWiStErRob]

I can confirm that the fix did not fix the reported problem:
https://github.com/TWiStErRob/net.twisterrob.cinema/pull/173/files

[TWiStErRob]

@BraisGabin can you please reopen?

[atulgpt]

Hi, @EdwarDDay the TC given by you is passing when you run detekt with type resolution on but if you run w/o type resolution then it is failing. Can you confirm in which mode you are running the detekt?
Hi @TWiStErRob can you confirm in the PR you run detekt /w type resolution or w/o it? cc @BraisGabin

[EdwarDDay]

Yes, I just ran the detekt task and not detektMain. When I read the docs right, this means, that the task runs w/o type resolution.

[TWiStErRob]

@atulgpt If you meant #4753 (comment), the linked PR TWiStErRob/net.twisterrob.cinema#173 has a commit whose build found this. Navigating the build you can find the workflow file which refers to this hacky w/o type resolution task (Since then, I've moved on to per-module type resolution, but this is an old build.)

[atulgpt]

Hi @TWiStErRob yup talking bout #4753 (comment)
if this is resolved we can close this issue

[TWiStErRob]

This issue was closed in #5009 released in 1.21.0, and you can see the PR is updating to that version, yet still flagging when I removed the suppression. (screenshot)

Since UnnecessaryAbstractClass does not require type resolution:

detekt/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryAbstractClass.kt

Lines 47 to 49 in 2e6b576

	@Suppress("ViolatesTypeResolutionRequirements")
	@ActiveByDefault(since = "1.2.0")
	class UnnecessaryAbstractClass(config: Config = Config.empty) : Rule(config) {

The issue is not solved yet. I re-ran on the latest code I have and it still flags as described in OP w/o type resolution:

net.twisterrob.cinema\Heroku>gradlew :backend:endpoint:detekt
> Task :backend:endpoint:detekt FAILED
style - 10min debt
        UnnecessaryAbstractClass - [An abstract class without a concrete member can be refactored to an interface.] at Heroku\backend\endpoint\src\test\kotlin\net\twisterrob\cinema\cineworld\backend\kto
r\RouteControllerRegistrarTest.kt:67:18
        UnnecessaryAbstractClass - [An abstract class without a concrete member can be refactored to an interface.] at Heroku\backend\endpoint\src\test\kotlin\net\twisterrob\cinema\cineworld\backend\kto
r\RouteControllerRegistrarTest.kt:68:18

Overall debt: 10min

with type resolution:

net.twisterrob.cinema\Heroku>gradlew :backend:endpoint:detektTest

BUILD SUCCESSFUL in 2s

Based on this, the rule just needs an annotation I think, because it's using bindingContext:
https://github.com/detekt/detekt/pull/5009/files#diff-beab67051ff8f0c1ed0d1a7bc12aaa9f3bb320efd8a409cee7801d1cf3a16945R133

@BraisGabin do we need to revisit those @Suppress("ViolatesTypeResolutionRequirements")?