False positive on rule UnnecessaryAbstractClass

[xgouchet]

Take for instance the following classes :

abstract class Foo {
    abstract fun bar() : Int

    fun baz() {
    }
}

abstract class Oof : Foo() {
    fun spam() {
    }
}

The class Oof is reported as being an UnnecessaryAbstractClass, because it only contains concrete methods. But in fact, because it inherits from Foo, it also have an abstract, unimplemented method (bar).

[schalkms]

Thanks for using detekt!
Detekt does not have type resolution (yet). For now I think this rule should ignore classes which derive from an abstract class. What do you think? @arturbosch @Mauin

[arturbosch]

Yep, it makes sense for now.
Will you take care of the fix @schalkms ?
Maybe we should open an issue and collect all the improvements to the rules which can be done after type resolution?

[schalkms]

I can do that. I think the type-resolution tag is sufficient.

[vanniktech]

Got something similar that should not be flagged:

abstract class Screenshots {
  @get:Rule val demoModeRule = DemoModeRule()

  @Before fun setUp() {
    Screengrab.setDefaultScreenshotStrategy(UiAutomatorScreenshotStrategy())

    tearDown()
  }

  @After fun tearDown() {
    nukeSharedPreferences()
  }

  fun screenshot(name: String) {
    Screengrab.screenshot(name, Screengrab.getDefaultScreenshotStrategy(), ConstantFileWritingScreenshotCallback(application()))
  }

  fun isUsingFastlane() = InstrumentationRegistry.getArguments().getString("isUsingFastlane")?.toBoolean() == true

  private class ConstantFileWritingScreenshotCallback(applicationContext: Context) : FileWritingScreenshotCallback(applicationContext) {
    override fun getScreenshotFile(screenshotDirectory: File, screenshotName: String) =
      File(screenshotDirectory, "$screenshotName.png")
  }
}

There are multiple reasons:

• using JUnit annotations that can be used downstream in the classes
• one can still use the provided methods (screenshot & isUsingFastlane)

[schalkms]

@vanniktech Hmm, I'm a bit skeptical and want to raise some questions. Should detekt really include framework specific things (JUnit in your case)? I'm a bit worried that this leads to implementing different edge cases which are only relevant for a minor audience.

[vanniktech]

I guess it should but I'm biased!