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

Fix NPE with commented import in kotlin scripts #999

Merged
merged 6 commits into from
Jul 27, 2021
Merged

Fix NPE with commented import in kotlin scripts #999

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bceedd9
Fix NPE with commented import in kotlin scripts
petertrr Jul 26, 2021
0584873
Update build_and_test.yml
petertrr Jul 26, 2021
a5de87f
Merge remote-tracking branch 'origin/master' into bugfix/gradle-script
petertrr Jul 26, 2021
c26283b
Fix NPE with commented import in kotlin scripts
petertrr Jul 26, 2021
186ed6e
Merge remote-tracking branch 'origin/bugfix/gradle-script' into bugfi…
petertrr Jul 26, 2021
70e505e
Merge branch 'master' into bugfix/gradle-script
petertrr Jul 27, 2021
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
2 changes: 1 addition & 1 deletion .github/workflows/build_and_test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ on:
jobs:
build_and_test_with_code_coverage:
name: Build, test and upload code coverage
runs-on: ubuntu-18.04
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v2.3.4
Expand Down
13 changes: 7 additions & 6 deletions ...t-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter2/kdoc/CommentsFormatting.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ class CommentsFormatting(configRules: List<RulesConfig>) : DiktatRule(
// Checking if comment is inside a code block like fun{}
// Not checking last comment as well
if (isFirstComment(node)) {
if (node.isBlockOrClassBody()) {
if (node.isChildOfBlockOrClassBody()) {
// Just check white spaces before comment
checkFirstCommentSpaces(node)
}
Expand Down Expand Up @@ -166,7 +166,7 @@ class CommentsFormatting(configRules: List<RulesConfig>) : DiktatRule(

private fun checkCommentsInCodeBlocks(node: ASTNode) {
if (isFirstComment(node)) {
if (node.isBlockOrClassBody()) {
if (node.isChildOfBlockOrClassBody()) {
// Just check white spaces before comment
checkFirstCommentSpaces(node)
}
Expand Down Expand Up @@ -289,7 +289,7 @@ class CommentsFormatting(configRules: List<RulesConfig>) : DiktatRule(

private fun checkClassComment(node: ASTNode) {
if (isFirstComment(node)) {
if (node.isBlockOrClassBody()) {
if (node.isChildOfBlockOrClassBody()) {
checkFirstCommentSpaces(node)
} else {
checkFirstCommentSpaces(node.treeParent)
Expand Down Expand Up @@ -317,10 +317,11 @@ class CommentsFormatting(configRules: List<RulesConfig>) : DiktatRule(
}
}

private fun isFirstComment(node: ASTNode) = if (node.isBlockOrClassBody()) {
private fun isFirstComment(node: ASTNode) = if (node.isChildOfBlockOrClassBody()) {
// In case when comment is inside of a function or class
if (node.treePrev.isWhiteSpace()) {
node.treePrev.treePrev.elementType == LBRACE
// in some cases (e.g. kts files) BLOCK doesn't have a leading brace
node.treePrev?.treePrev?.elementType == LBRACE
Copy link
Member

Choose a reason for hiding this comment

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

treePrev, treeParent are nullable types, however, they don't marked as such in source code, so IDEA don't advise to make such calls 'save' automatically, and so we use them is some dangerous way
May be we need to change such calls in whole project to appropriate !! or ?. in aim to avoid some problems in future?

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem here is that we won't get any compiler checks, unless source of ASTNode is annotated with proper annotations. For sure, adding safe access to all such unvokations may save us from some NPEs, but I don't see a good way to enforce this style.

} else {
node.treePrev == null || node.treePrev.elementType == LBRACE // null is handled for functional literal
}
Expand All @@ -335,7 +336,7 @@ class CommentsFormatting(configRules: List<RulesConfig>) : DiktatRule(
node.treeParent.getAllChildrenWithType(node.elementType).first() == node
}

private fun ASTNode.isBlockOrClassBody(): Boolean = treeParent.elementType == BLOCK || treeParent.elementType == CLASS_BODY
private fun ASTNode.isChildOfBlockOrClassBody(): Boolean = treeParent.elementType == BLOCK || treeParent.elementType == CLASS_BODY

/**
* [RuleConfiguration] for [CommentsFormatting] rule
Expand Down
1 change: 0 additions & 1 deletion diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/utils/indentation/Checkers.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@ internal class ValueParameterListChecker(configuration: IndentationConfig) : Cus
it.node.elementType.run { this == VALUE_ARGUMENT || this == VALUE_PARAMETER }
}

@Suppress("ANNOTATION_NEW_LINE") // https://github.com/cqfn/diKTat/issues/609
override fun checkNode(whiteSpace: PsiWhiteSpace, indentError: IndentationError): CheckResult? {
if (isCheckNeeded(whiteSpace)) {
val parameterList = whiteSpace.parent.node
Expand Down
15 changes: 12 additions & 3 deletions diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/smoke/DiktatSmokeTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,9 +197,7 @@ class DiktatSmokeTest : FixTestBase("test/smoke/src/main/kotlin",
@Tag("DiktatRuleSetProvider")
fun `smoke test with kts files`() {
overrideRulesConfig(
listOf(
HEADER_MISSING_IN_NON_SINGLE_CLASS_FILE // because build.gradle.kts doesn't need extra comments, and this rule can be manually disabled if needed
),
emptyList(),
mapOf(
Warnings.WRONG_INDENTATION.name to mapOf(
"newlineAtEnd" to "false",
Expand All @@ -219,6 +217,17 @@ class DiktatSmokeTest : FixTestBase("test/smoke/src/main/kotlin",
tmpTestFile.delete()
}

@Test
@Tag("DiktatRuleSetProvider")
fun `smoke test with gradle script plugin`() {
fixAndCompare("kotlin-library-expected.gradle.kts", "kotlin-library.gradle.kts")
Assertions.assertEquals(
LintError(2, 1, "$DIKTAT_RULE_SET_ID:comments", "[COMMENTED_OUT_CODE] you should not comment out code, " +
"use VCS to save it in history and delete this block: import org.jetbrains.kotlin.gradle.dsl.jvm", false),
unfixedLintErrors.single()
)
}

@Test
@Tag("DiktatRuleSetProvider")
fun `smoke test with kts files #2`() {
Expand Down
7 changes: 7 additions & 0 deletions ...at-rules/src/test/resources/test/smoke/src/main/kotlin/kotlin-library-expected.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import org.gradle.kotlin.dsl.plugins
// import org.jetbrains.kotlin.gradle.dsl.jvm

plugins {
kotlin("jvm")
}

6 changes: 6 additions & 0 deletions diktat-rules/src/test/resources/test/smoke/src/main/kotlin/kotlin-library.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import org.gradle.kotlin.dsl.plugins
//import org.jetbrains.kotlin.gradle.dsl.jvm

plugins {
kotlin("jvm")
}