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.

Sign up for GitHub

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

Jump to bottom

Fixes false negatives in UnnecessaryAbstractClass #4399

Merged
merged 5 commits into from Jan 2, 2022
Merged

Fixes false negatives in UnnecessaryAbstractClass #4399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9246b14
Improve tests
BraisGabin Dec 26, 2021
ad9739a
Remove UnnecessaryAbstractClass
BraisGabin Dec 26, 2021
3209da0
Empty classes that inherits from classes or interfaces are flagged too
BraisGabin Dec 26, 2021
9a646f2
Improve messages
BraisGabin Dec 26, 2021
cfcaf6d
Apply suggestions from code review
BraisGabin Dec 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 0 additions & 5 deletions ...re/src/main/kotlin/io/gitlab/arturbosch/detekt/core/baseline/DelegatingXMLStreamWriter.kt
View file
Open in desktop

This file was deleted.

42 changes: 21 additions & 21 deletions ...ore/src/main/kotlin/io/gitlab/arturbosch/detekt/core/baseline/IndentingXMLStreamWriter.kt
View file
Open in desktop
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 {
schalkms marked this conversation as resolved.
Show resolved Hide resolved

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
Open in desktop
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)
Copy link
Member

Choose a reason for hiding this comment

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

Where did you improve the message description?

This PR also improves the messages reported for this rule.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. I didn't "improve the message". Before there were some issues that had the message "An abstract class without an abstract member can be refactored to a concrete class." but it should be "An abstract class without a concrete member can be refactored to an interface." and the improvement was that. I set the correct message to some issues that were getting the wrong one.

} 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
Open in desktop
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