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

Improve checking on backing property #2346

Merged
merged 1 commit into from
Nov 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import com.pinterest.ktlint.rule.engine.core.api.ElementType.OVERRIDE_KEYWORD
import com.pinterest.ktlint.rule.engine.core.api.ElementType.PRIVATE_KEYWORD
import com.pinterest.ktlint.rule.engine.core.api.ElementType.PROPERTY
import com.pinterest.ktlint.rule.engine.core.api.ElementType.PROPERTY_ACCESSOR
import com.pinterest.ktlint.rule.engine.core.api.ElementType.PROTECTED_KEYWORD
import com.pinterest.ktlint.rule.engine.core.api.ElementType.VAL_KEYWORD
import com.pinterest.ktlint.rule.engine.core.api.RuleId
import com.pinterest.ktlint.rule.engine.core.api.SinceKtlint
Expand Down Expand Up @@ -53,7 +54,7 @@ public class PropertyNamingRule : StandardRule("property-naming") {
property.hasCustomGetter() || property.isTopLevelValue() || property.isObjectValue() -> {
// Can not reliably determine whether the value is immutable or not
}
property.isBackingProperty() -> {
identifier.text.startsWith("_") -> {
visitBackingProperty(identifier, emit)
}
else -> {
Expand All @@ -73,6 +74,27 @@ public class PropertyNamingRule : StandardRule("property-naming") {
?.let {
emit(identifier.startOffset, "Backing property name should start with underscore followed by lower camel case", false)
}

if (!identifier.treeParent.hasModifier(PRIVATE_KEYWORD)) {
emit(identifier.startOffset, "Backing property name not allowed when 'private' modifier is missing", false)
}

identifier
.treeParent
?.treeParent
?.children()
?.filter { it.elementType == PROPERTY }
?.mapNotNull { it.findChildByType(IDENTIFIER) }
?.firstOrNull { it.text == identifier.text.removePrefix("_") }
?.treeParent
.let { otherProperty ->
if (otherProperty == null ||
otherProperty.hasModifier(PRIVATE_KEYWORD) ||
otherProperty.hasModifier(PROTECTED_KEYWORD)
) {
emit(identifier.startOffset, "Backing property not allowed when matching public property is missing", false)
}
}
}

private fun visitConstProperty(
Expand Down Expand Up @@ -129,22 +151,6 @@ public class PropertyNamingRule : StandardRule("property-naming") {
containsValKeyword() &&
!hasModifier(OVERRIDE_KEYWORD)

private fun ASTNode.isBackingProperty() =
takeIf { hasModifier(PRIVATE_KEYWORD) }
?.findChildByType(IDENTIFIER)
?.takeIf { it.text.startsWith("_") }
?.let { identifier ->
this.hasPublicProperty(identifier.text.removePrefix("_"))
}
?: false

private fun ASTNode.hasPublicProperty(identifier: String) =
treeParent
.children()
.filter { it.elementType == PROPERTY }
.mapNotNull { it.findChildByType(IDENTIFIER) }
.any { it.text == identifier }

private companion object {
val LOWER_CAMEL_CASE_REGEXP = "[a-z][a-zA-Z0-9]*".regExIgnoringDiacriticsAndStrokesOnLetters()
val SCREAMING_SNAKE_CASE_REGEXP = "[A-Z][_A-Z0-9]*".regExIgnoringDiacriticsAndStrokesOnLetters()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ class PropertyNamingRuleTest {
}

@Test
fun `Given a backing val property name having a custom get function and not in screaming case notation then do not emit`() {
fun `Given a backing val property name, not in screaming case notation, and having another property with a custom get function then do not emit`() {
val code =
"""
class Foo {
Expand All @@ -132,6 +132,43 @@ class PropertyNamingRuleTest {
propertyNamingRuleAssertThat(code).hasNoLintViolations()
}

@Test
fun `Given a backing val property name, not in screaming case notation, and having another property with a custom get function with public modifier then do not emit`() {
val code =
"""
class Foo {
private val _elementList = mutableListOf<Element>()

public val elementList: List<Element>
get() = _elementList
}
""".trimIndent()
propertyNamingRuleAssertThat(code).hasNoLintViolations()
}

@ParameterizedTest(name = "Modifier: {0}")
@ValueSource(
strings = [
"private",
"protected",
],
)
fun `Given a backing val property name, not in screaming case notation, and having having another property with a custom get function with non-public modifier then do emit`(
modifier: String,
) {
val code =
"""
class Foo {
private val _elementList = mutableListOf<Element>()

$modifier val elementList: List<Element>
get() = _elementList
}
""".trimIndent()
propertyNamingRuleAssertThat(code)
.hasLintViolationWithoutAutoCorrect(2, 17, "Backing property not allowed when matching public property is missing")
}

@Test
fun `Given a backing val property name containing diacritics having a custom get function and not in screaming case notation then do not emit`() {
val code =
Expand Down Expand Up @@ -266,8 +303,8 @@ class PropertyNamingRuleTest {
LintViolation(1, 11, "Property name should use the screaming snake case notation when the value can not be changed", canBeAutoCorrected = false),
LintViolation(3, 5, "Property name should start with a lowercase letter and use camel case", canBeAutoCorrected = false),
LintViolation(6, 9, "Property name should start with a lowercase letter and use camel case", canBeAutoCorrected = false),
LintViolation(9, 17, "Property name should start with a lowercase letter and use camel case", canBeAutoCorrected = false),
LintViolation(12, 9, "Property name should start with a lowercase letter and use camel case", canBeAutoCorrected = false),
LintViolation(9, 17, "Backing property not allowed when matching public property is missing", canBeAutoCorrected = false),
LintViolation(12, 9, "Backing property name not allowed when 'private' modifier is missing", canBeAutoCorrected = false),
)
}
}