Fix rule LibraryCodeMustSpecifyReturnType

[chao2zhang]

False-positive: Members without visibility modifiers in an internal class should not be reported.
False-negative: Members with protected modifiers in a public class should be reported.

This PR changes the underlying implementation of LibraryCodeMustSpecifyReturnType to Kotlin Compiler's explicit API mode, which was introduced in Kotlin 1.4
See https://kotlinlang.org/docs/reference/whatsnew14.html#explicit-api-mode-for-library-authors

This is actually blocking my company's repositories to enable LibraryCodeMustSpecifyReturnType rule because of tons of false positives.

[chao2zhang]

Note that there is an error at LibraryCodeMustSpecifyReturnTypeSpec:99
and there was a previous comment // Kotlin Script Engine reports wrongly local functions here

Please let me know if this is okay to ignore.

    io.github.detekt.test.utils.KotlinScriptException: Given Kotlin code is invalid.
        at io.github.detekt.test.utils.KotlinScriptEngine.compile(KotlinScriptEngine.kt:21)
        at io.gitlab.arturbosch.detekt.test.RuleExtensionsKt.compileAndLintWithContext(RuleExtensions.kt:47)
        at io.gitlab.arturbosch.detekt.rules.style.LibraryCodeMustSpecifyReturnTypeSpec$1$1$4$1.invoke(LibraryCodeMustSpecifyReturnTypeSpec.kt:100)
        at io.gitlab.arturbosch.detekt.rules.style.LibraryCodeMustSpecifyReturnTypeSpec$1$1$4$1.invoke(LibraryCodeMustSpecifyReturnTypeSpec.kt:12)

        Caused by:
        javax.script.ScriptException: Error: error: modifier 'internal' is not applicable to 'local function'
                            internal fun bar() = 5
                            ^
        error: modifier 'private' is not applicable to 'local function'
                            private fun foo() = 5
                            ^
            at org.jetbrains.kotlin.cli.common.repl.KotlinJsr223JvmScriptEngineBase.compile(KotlinJsr223JvmScriptEngineBase.kt:74)
            at org.jetbrains.kotlin.cli.common.repl.KotlinJsr223JvmScriptEngineBase.compile(KotlinJsr223JvmScriptEngineBase.kt:35)
            at io.github.detekt.test.utils.KotlinScriptEngine.compile(KotlinScriptEngine.kt:18)
            ... 3 more

[codecov]

Codecov Report

Merging #3155 into master will decrease coverage by 0.02%.
The diff coverage is 86.44%.

@@             Coverage Diff              @@
##             master    #3155      +/-   ##
============================================
- Coverage     79.52%   79.49%   -0.03%     
- Complexity     2592     2595       +3     
============================================
  Files           437      437              
  Lines          7852     7876      +24     
  Branches       1495     1500       +5     
============================================
+ Hits           6244     6261      +17     
- Misses          817      819       +2     
- Partials        791      796       +5     

Impacted Files	Coverage Δ	Complexity Δ
...kt/rules/style/LibraryCodeMustSpecifyReturnType.kt	87.50% <71.42%> (-12.50%)	12.00 <7.00> (-4.00)
...urbosch/detekt/rules/performance/SpreadOperator.kt	81.25% <83.33%> (-5.71%)	12.00 <6.00> (+4.00)	⬇️
...tlab/arturbosch/detekt/rules/naming/ClassNaming.kt	100.00% <100.00%> (ø)	5.00 <2.00> (+1.00)
...ab/arturbosch/detekt/rules/naming/PackageNaming.kt	100.00% <100.00%> (ø)	7.00 <2.00> (+1.00)
.../arturbosch/detekt/rules/style/ForbiddenComment.kt	100.00% <100.00%> (ø)	7.00 <0.00> (ø)
...b/arturbosch/detekt/rules/style/ForbiddenImport.kt	94.44% <100.00%> (ø)	10.00 <0.00> (ø)
...ekt/rules/style/RedundantVisibilityModifierRule.kt	95.55% <100.00%> (+0.10%)	10.00 <1.00> (+1.00)
...b/arturbosch/detekt/rules/style/UseCheckOrError.kt	90.00% <0.00%> (-0.91%)	8.00% <0.00%> (ø%)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d7bd33...78f0bfc. Read the comment docs.

[arturbosch]

Note that there is an error at LibraryCodeMustSpecifyReturnTypeSpec:99
and there was a previous comment // Kotlin Script Engine reports wrongly local functions here

Please let me know if this is okay to ignore.

    io.github.detekt.test.utils.KotlinScriptException: Given Kotlin code is invalid.
        at io.github.detekt.test.utils.KotlinScriptEngine.compile(KotlinScriptEngine.kt:21)
        at io.gitlab.arturbosch.detekt.test.RuleExtensionsKt.compileAndLintWithContext(RuleExtensions.kt:47)
        at io.gitlab.arturbosch.detekt.rules.style.LibraryCodeMustSpecifyReturnTypeSpec$1$1$4$1.invoke(LibraryCodeMustSpecifyReturnTypeSpec.kt:100)
        at io.gitlab.arturbosch.detekt.rules.style.LibraryCodeMustSpecifyReturnTypeSpec$1$1$4$1.invoke(LibraryCodeMustSpecifyReturnTypeSpec.kt:12)

        Caused by:
        javax.script.ScriptException: Error: error: modifier 'internal' is not applicable to 'local function'
                            internal fun bar() = 5
                            ^
        error: modifier 'private' is not applicable to 'local function'
                            private fun foo() = 5
                            ^
            at org.jetbrains.kotlin.cli.common.repl.KotlinJsr223JvmScriptEngineBase.compile(KotlinJsr223JvmScriptEngineBase.kt:74)
            at org.jetbrains.kotlin.cli.common.repl.KotlinJsr223JvmScriptEngineBase.compile(KotlinJsr223JvmScriptEngineBase.kt:35)
            at io.github.detekt.test.utils.KotlinScriptEngine.compile(KotlinScriptEngine.kt:18)
            ... 3 more

Hi, thanks for the PR, it looks good.
I will wait for 1.15 to merge this as it includes changing a rule quite drastically (a good imporvement).
Regarding the test case please introduce a lintWithContext which does skip the script compilation check.
The function should include a comment that this function should not be used normally:

fun BaseRule.lintWithContext(
    environment: KotlinCoreEnvironment,
    @Language("kotlin") content: String,
    @Language("kotlin") vararg additionalContents: String,
): List<Finding> {
    val ktFile = compileContentForTest(content.trimIndent())
    val additionalKtFiles = additionalContents.mapIndexed { index, additionalContent ->
        compileContentForTest(additionalContent.trimIndent(), "AdditionalTest$index.kt")
    }
    val bindingContext = getContextForPaths(environment, listOf(ktFile) + additionalKtFiles)
    val languageVersionSettings = environment.configuration.languageVersionSettings
    @Suppress("DEPRECATION")
    val dataFlowValueFactory = DataFlowValueFactoryImpl(languageVersionSettings)
    val compilerResources = CompilerResources(languageVersionSettings, dataFlowValueFactory)
    return findingsAfterVisit(ktFile, bindingContext, compilerResources)
}

fun BaseRule.compileAndLintWithContext(
    environment: KotlinCoreEnvironment,
    @Language("kotlin") content: String,
    @Language("kotlin") vararg additionalContents: String,
): List<Finding> {
    if (shouldCompileTestSnippets && additionalContents.isEmpty()) {
        KotlinScriptEngine.compile(content)
    }
    return lintWithContext(environment, content, *additionalContents)
}

[schalkms]

Thanks!

[chao2zhang]

Note that there is an error at LibraryCodeMustSpecifyReturnTypeSpec:99
and there was a previous comment // Kotlin Script Engine reports wrongly local functions here
Please let me know if this is okay to ignore.

    io.github.detekt.test.utils.KotlinScriptException: Given Kotlin code is invalid.
        at io.github.detekt.test.utils.KotlinScriptEngine.compile(KotlinScriptEngine.kt:21)
        at io.gitlab.arturbosch.detekt.test.RuleExtensionsKt.compileAndLintWithContext(RuleExtensions.kt:47)
        at io.gitlab.arturbosch.detekt.rules.style.LibraryCodeMustSpecifyReturnTypeSpec$1$1$4$1.invoke(LibraryCodeMustSpecifyReturnTypeSpec.kt:100)
        at io.gitlab.arturbosch.detekt.rules.style.LibraryCodeMustSpecifyReturnTypeSpec$1$1$4$1.invoke(LibraryCodeMustSpecifyReturnTypeSpec.kt:12)

        Caused by:
        javax.script.ScriptException: Error: error: modifier 'internal' is not applicable to 'local function'
                            internal fun bar() = 5
                            ^
        error: modifier 'private' is not applicable to 'local function'
                            private fun foo() = 5
                            ^
            at org.jetbrains.kotlin.cli.common.repl.KotlinJsr223JvmScriptEngineBase.compile(KotlinJsr223JvmScriptEngineBase.kt:74)
            at org.jetbrains.kotlin.cli.common.repl.KotlinJsr223JvmScriptEngineBase.compile(KotlinJsr223JvmScriptEngineBase.kt:35)
            at io.github.detekt.test.utils.KotlinScriptEngine.compile(KotlinScriptEngine.kt:18)
            ... 3 more

Hi, thanks for the PR, it looks good.
I will wait for 1.15 to merge this as it includes changing a rule quite drastically (a good imporvement).
Regarding the test case please introduce a lintWithContext which does skip the script compilation check.
The function should include a comment that this function should not be used normally:

fun BaseRule.lintWithContext(
    environment: KotlinCoreEnvironment,
    @Language("kotlin") content: String,
    @Language("kotlin") vararg additionalContents: String,
): List<Finding> {
    val ktFile = compileContentForTest(content.trimIndent())
    val additionalKtFiles = additionalContents.mapIndexed { index, additionalContent ->
        compileContentForTest(additionalContent.trimIndent(), "AdditionalTest$index.kt")
    }
    val bindingContext = getContextForPaths(environment, listOf(ktFile) + additionalKtFiles)
    val languageVersionSettings = environment.configuration.languageVersionSettings
    @Suppress("DEPRECATION")
    val dataFlowValueFactory = DataFlowValueFactoryImpl(languageVersionSettings)
    val compilerResources = CompilerResources(languageVersionSettings, dataFlowValueFactory)
    return findingsAfterVisit(ktFile, bindingContext, compilerResources)
}

fun BaseRule.compileAndLintWithContext(
    environment: KotlinCoreEnvironment,
    @Language("kotlin") content: String,
    @Language("kotlin") vararg additionalContents: String,
): List<Finding> {
    if (shouldCompileTestSnippets && additionalContents.isEmpty()) {
        KotlinScriptEngine.compile(content)
    }
    return lintWithContext(environment, content, *additionalContents)
}

That is a great suggestion! I have made the changes and all the checks passed now.

[BraisGabin]

Should we add @requiresTypeResolution to the documentation?

[chao2zhang]

requiresTypeResolution

Were you referring to create a new @OptIn annotation class?