Skip to content

Commit

Permalink
Fixes false negatives in UnnecessaryAbstractClass (#4399)
Browse files Browse the repository at this point in the history 
* Improve tests

* Remove UnnecessaryAbstractClass

* Empty classes that inherits from classes or interfaces are flagged too

* Improve messages

* Apply suggestions from code review

Co-authored-by: M Schalk <30376729+schalkms@users.noreply.github.com>

Co-authored-by: M Schalk <30376729+schalkms@users.noreply.github.com>
  • Loading branch information
@BraisGabin @schalkms
BraisGabin and schalkms committed Jan 2, 2022
1 parent a7184d1 commit eb0f1cf
Show file tree
Hide file tree
Showing 4 changed files with 76 additions and 58 deletions.
5 changes: 0 additions & 5 deletions ...re/src/main/kotlin/io/gitlab/arturbosch/detekt/core/baseline/DelegatingXMLStreamWriter.kt
View file

This file was deleted.

42 changes: 21 additions & 21 deletions ...ore/src/main/kotlin/io/gitlab/arturbosch/detekt/core/baseline/IndentingXMLStreamWriter.kt
View file
Expand Up @@ -5,9 +5,9 @@ import javax.xml.stream.XMLStreamWriter

@Suppress("TooManyFunctions")
internal class IndentingXMLStreamWriter(
writer: XMLStreamWriter,
private val writer: XMLStreamWriter,
private val indent: String = " "
) : DelegatingXMLStreamWriter(writer) {
) : XMLStreamWriter by writer {

private var currentState = NOTHING
private val stateStack = Stack<Any>()
Expand All @@ -25,7 +25,7 @@ internal class IndentingXMLStreamWriter(
private fun onEndTag() {
indentationDepth--
if (currentState === TAG) {
super.writeCharacters("\n")
writer.writeCharacters("\n")
writeIndent()
}
currentState = stateStack.pop()
Expand All @@ -39,79 +39,79 @@ internal class IndentingXMLStreamWriter(

private fun writeNL() {
if (indentationDepth > 0) {
super.writeCharacters("\n")
writer.writeCharacters("\n")
}
}

private fun writeIndent() {
if (indentationDepth > 0) {
(0 until indentationDepth).forEach { _ -> super.writeCharacters(indent) }
(0 until indentationDepth).forEach { _ -> writer.writeCharacters(indent) }
}
}

override fun writeStartDocument() {
super.writeStartDocument()
super.writeCharacters("\n")
writer.writeStartDocument()
writer.writeCharacters("\n")
}

override fun writeStartDocument(version: String) {
super.writeStartDocument(version)
super.writeCharacters("\n")
writer.writeStartDocument(version)
writer.writeCharacters("\n")
}

override fun writeStartDocument(encoding: String, version: String) {
super.writeStartDocument(encoding, version)
super.writeCharacters("\n")
writer.writeStartDocument(encoding, version)
writer.writeCharacters("\n")
}

override fun writeStartElement(localName: String) {
onStartTag()
super.writeStartElement(localName)
writer.writeStartElement(localName)
}

override fun writeStartElement(namespaceURI: String, localName: String) {
onStartTag()
super.writeStartElement(namespaceURI, localName)
writer.writeStartElement(namespaceURI, localName)
}

override fun writeStartElement(prefix: String, localName: String, namespaceURI: String) {
onStartTag()
super.writeStartElement(prefix, localName, namespaceURI)
writer.writeStartElement(prefix, localName, namespaceURI)
}

override fun writeEmptyElement(namespaceURI: String, localName: String) {
onEmptyTag()
super.writeEmptyElement(namespaceURI, localName)
writer.writeEmptyElement(namespaceURI, localName)
}

override fun writeEmptyElement(prefix: String, localName: String, namespaceURI: String) {
onEmptyTag()
super.writeEmptyElement(prefix, localName, namespaceURI)
writer.writeEmptyElement(prefix, localName, namespaceURI)
}

override fun writeEmptyElement(localName: String) {
onEmptyTag()
super.writeEmptyElement(localName)
writer.writeEmptyElement(localName)
}

override fun writeEndElement() {
onEndTag()
super.writeEndElement()
writer.writeEndElement()
}

override fun writeCharacters(text: String) {
currentState = DATA
super.writeCharacters(text)
writer.writeCharacters(text)
}

override fun writeCharacters(text: CharArray, start: Int, len: Int) {
currentState = DATA
super.writeCharacters(text, start, len)
writer.writeCharacters(text, start, len)
}

override fun writeCData(data: String) {
currentState = DATA
super.writeCData(data)
writer.writeCData(data)
}

companion object {
Expand Down
12 changes: 6 additions & 6 deletions ...style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryAbstractClass.kt
View file
Expand Up @@ -73,12 +73,12 @@ class UnnecessaryAbstractClass(config: Config = Config.empty) : Rule(config) {

override fun visitClass(klass: KtClass) {
if (!klass.isInterface() && klass.isAbstract()) {
val body = klass.body
if (body != null) {
val namedMembers = body.children.filterIsInstance<KtNamedDeclaration>()
val namedClassMembers = NamedClassMembers(klass, namedMembers)
namedClassMembers.detectAbstractAndConcreteType()
} else if (klass.superTypeListEntries.isEmpty()) {
val namedMembers = klass.body?.children.orEmpty().filterIsInstance<KtNamedDeclaration>()
if (namedMembers.isNotEmpty()) {
NamedClassMembers(klass, namedMembers).detectAbstractAndConcreteType()
} else if (!klass.hasConstructorParameter()) {
report(CodeSmell(issue, Entity.from(klass), noConcreteMember), klass)
} else {
report(CodeSmell(issue, Entity.from(klass), noAbstractMember), klass)
}
}
Expand Down
75 changes: 49 additions & 26 deletions ...e/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryAbstractClassSpec.kt
View file
Expand Up @@ -16,18 +16,13 @@ class UnnecessaryAbstractClassSpec : Spek({

val env: KotlinCoreEnvironment by memoized()
val subject by memoized {
UnnecessaryAbstractClass(
TestConfig(
mapOf(
EXCLUDE_ANNOTATED_CLASSES to listOf("Deprecated")
)
)
)
UnnecessaryAbstractClass(TestConfig(mapOf(EXCLUDE_ANNOTATED_CLASSES to listOf("Deprecated"))))
}

describe("UnnecessaryAbstractClass rule") {

context("abstract classes with no concrete members") {
val message = "An abstract class without a concrete member can be refactored to an interface."

it("reports an abstract class with no concrete member") {
val code = """
Expand All @@ -38,28 +33,56 @@ class UnnecessaryAbstractClassSpec : Spek({
}
"""
val findings = subject.compileAndLintWithContext(env, code)
assertFindingMessage(
findings,
"An abstract class without a concrete member can be refactored to an interface."
)
assertFindingMessage(findings, message)
}

it("reports completely-empty abstract classes") {
val code = """
abstract class A
abstract class B()
"""
assertThat(subject.compileAndLintWithContext(env, code)).hasSize(2)
}
context("reports completely-empty abstract classes") {
it("case 1") {
val code = "abstract class A"
val findings = subject.compileAndLintWithContext(env, code)
assertFindingMessage(findings, message)
}

it("does not report a completely-empty abstract class that inherits from an interface") {
val code = """
interface A {
val i: Int
}
abstract class B : A
""".trimIndent()
assertThat(subject.compileAndLintWithContext(env, code)).isEmpty()
it("case 2") {
val code = "abstract class A()"
val findings = subject.compileAndLintWithContext(env, code)
assertFindingMessage(findings, message)
}

it("case 3") {
val code = "abstract class A {}"
val findings = subject.compileAndLintWithContext(env, code)
assertFindingMessage(findings, message)
}

it("case 4") {
val code = "abstract class A() {}"
val findings = subject.compileAndLintWithContext(env, code)
assertFindingMessage(findings, message)
}

it("that inherits from an interface") {
val code = """
interface A {
val i: Int
}
abstract class B : A
"""
val findings = subject.compileAndLintWithContext(env, code)
assertFindingMessage(findings, message)
}

it("that inherits from another abstract class") {
val code = """
@Deprecated("We don't care about this first class")
abstract class A {
abstract val i: Int
}
abstract class B : A()
"""
val findings = subject.compileAndLintWithContext(env, code)
assertFindingMessage(findings, message)
}
}

it("does not report an abstract class with concrete members derived from a base class") {
Expand Down

0 comments on commit eb0f1cf

Please sign in to comment.