Kotlin language version handling #1748

remal · 2019-07-02T14:42:00Z

This PR helps to solve an issue when a user have a Kotlin project with Kotlin version less then Detekt's Kotlin version. Without these fixes a lot of Kotlin embedded compiler warnings occur.

Also a lot of coding style issues have been fixed.

Signed-off-by: Semyon Levin <levin.semen@gmail.com>

codecov-io · 2019-07-02T15:22:24Z

Codecov Report

Merging #1748 into master will decrease coverage by 0.08%.
The diff coverage is 80.43%.

@@             Coverage Diff              @@
##             master    #1748      +/-   ##
============================================
- Coverage     79.86%   79.78%   -0.09%     
- Complexity     1936     1937       +1     
============================================
  Files           325      324       -1     
  Lines          5478     5500      +22     
  Branches       1013     1015       +2     
============================================
+ Hits           4375     4388      +13     
- Misses          581      587       +6     
- Partials        522      525       +3

Impacted Files	Coverage Δ	Complexity Δ
.../io/gitlab/arturbosch/detekt/cli/runners/Runner.kt	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...n/io/gitlab/arturbosch/detekt/core/DetektFacade.kt	`48.78% <ø> (-2.39%)`	`3 <0> (ø)`
.../io/gitlab/arturbosch/detekt/cli/PathConverters.kt	`72% <0%> (-6.27%)`	`0 <0> (ø)`
...lin/io/gitlab/arturbosch/detekt/core/KtCompiler.kt	`69.69% <100%> (-1.74%)`	`6 <0> (-1)`
...itlab/arturbosch/detekt/core/ProcessingSettings.kt	`79.06% <100%> (+1.02%)`	`16 <1> (+1)`	⬆️
.../kotlin/io/gitlab/arturbosch/detekt/cli/CliArgs.kt	`84.61% <80%> (-3.39%)`	`7 <5> (ø)`
...osch/detekt/api/internal/KotlinEnvironmentUtils.kt	`86% <84.61%> (-2.89%)`	`0 <0> (ø)`
...b/arturbosch/detekt/rules/empty/EmptyCatchBlock.kt	`57.14% <0%> (-26.2%)`	`3% <0%> (ø)`
...n/kotlin/io/gitlab/arturbosch/detekt/api/Metric.kt	`73.33% <0%> (-4.45%)`	`8% <0%> (ø)`
...osch/detekt/rules/exceptions/SwallowedException.kt	`65.38% <0%> (-2.48%)`	`13% <0%> (-3%)`
... and 16 more

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 e7aa71e...e003512. Read the comment docs.

remal · 2019-07-02T18:36:39Z

@arturbosch @schalkms Could you please take a look?

remal · 2019-07-02T19:40:24Z

This PR fixes this warning:

warning: runtime JAR files in the classpath have the version 1.2, which is older than the API version 1.3. Consider using the runtime of version 1.3, or pass '-api-version 1.2' explicitly to restrict the available APIs to the runtime of version 1.2. You can also pass '-language-version 1.2' instead, which will restrict not only the APIs to the specified version, but also the language features
~/.gradle/wrapper/dists/gradle-4.10.3-all/81msde2dx9p4vji0mjgtvxkcb/gradle-4.10.3/lib/kotlin-reflect-1.2.61.jar: warning: runtime JAR file has version 1.2 which is older than required for API version 1.3
~/.gradle/wrapper/dists/gradle-4.10.3-all/81msde2dx9p4vji0mjgtvxkcb/gradle-4.10.3/lib/kotlin-script-runtime-1.2.61.jar: warning: runtime JAR file has version 1.2 which is older than required for API version 1.3
~/.gradle/wrapper/dists/gradle-4.10.3-all/81msde2dx9p4vji0mjgtvxkcb/gradle-4.10.3/lib/kotlin-stdlib-1.2.61.jar: warning: runtime JAR file has version 1.2 which is older than required for API version 1.3
~/.gradle/wrapper/dists/gradle-4.10.3-all/81msde2dx9p4vji0mjgtvxkcb/gradle-4.10.3/lib/kotlin-stdlib-common-1.2.61.jar: warning: runtime JAR file has version 1.2 which is older than required for API version 1.3
~/.gradle/wrapper/dists/gradle-4.10.3-all/81msde2dx9p4vji0mjgtvxkcb/gradle-4.10.3/lib/kotlin-stdlib-jdk7-1.2.61.jar: warning: runtime JAR file has version 1.2 which is older than required for API version 1.3
~/.gradle/wrapper/dists/gradle-4.10.3-all/81msde2dx9p4vji0mjgtvxkcb/gradle-4.10.3/lib/kotlin-stdlib-jdk8-1.2.61.jar: warning: runtime JAR file has version 1.2 which is older than required for API version 1.3
~/.gradle/caches/modules-2/files-2.1/org.jetbrains.kotlin/kotlin-reflect/1.2.61/6fd5bcfc9ffc446dd147ea006bee7ef5f0ad8ca4/kotlin-reflect-1.2.61.jar: warning: runtime JAR file has version 1.2 which is older than required for API version 1.3
~/.gradle/caches/modules-2/files-2.1/org.jetbrains.kotlin/kotlin-stdlib/1.2.61/5bc44acc4b3f0d19166ae3e50454b41e8ff29335/kotlin-stdlib-1.2.61.jar: warning: runtime JAR file has version 1.2 which is older than required for API version 1.3
~/.gradle/caches/modules-2/files-2.1/org.jetbrains.kotlin/kotlin-stdlib-common/1.2.61/772de03e12d932f489e41aef997d26c20a4ebee6/kotlin-stdlib-common-1.2.61.jar: warning: runtime JAR file has version 1.2 which is older than required for API version 1.3

schalkms

Looks good. I think this is a nice addition.
I have added some questions to this review.
The cli flag is just a check right. There's no other behavior
I think documentation and tests are missing for this cli flag.

docs/pages/gettingstarted/cli.md

detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/invoke/CliArgument.kt

detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/Detekt.kt

detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/ProcessingSettings.kt

detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/KtCompiler.kt

detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/DetektFacade.kt

detekt-cli/src/main/kotlin/io/gitlab/arturbosch/detekt/cli/CliArgs.kt

schalkms · 2019-07-02T20:00:28Z

@remal you need to use the --language-version flag in order to fix this warning, right? Otherwise, the warning still gets displayed, which isn't a bad thing IMHO.

Fix a misprint

remal · 2019-07-02T20:45:11Z

@schalkms Yes, you're right. I'm trying to analyze sources of a project that depends on an older version of Kotlin (1.2). So I see this warning every time I run Detekt.

schalkms

Good.
Regarding the tests, I think it's okay then.
With my PR in #1752 codecov doesn't fail anymore when the test coverage decreases.
I'm resolving the remaining discussions.
Please take a look at my last question. Otherwise this is then good to go, I think.

schalkms · 2019-07-02T20:51:01Z

docs/pages/gettingstarted/cli.md

@@ -77,6 +77,11 @@ Usage: detekt [options]
    --input, -i
      Input paths to analyze. Multiple paths are separated by comma. If not
      specified the current working directory is used.
+    --language-version
+      EXPERIMENTAL: Compatibility mode for Kotlin language version X.Y, reports


reports errors for all language features that came out later.

What do you mean by that? Could you please show an example to help me understand the problem?

Actually I haven't seen such warnings. I suppose we can get them if Kotlin JARs in the classpath are not aligned, for example.

remal · 2019-07-02T20:57:49Z

@schalkms I have an idea to move automated Kotlin version detection from Detekt's Gradle plugin to detekt-cli. It can be done by processing --classpath parameter. I'll try to make it tomorrow.

schalkms · 2019-07-02T21:00:39Z

I have an idea to move automatically Kotlin version detection from Detekt's Gradle plugin to detekt-cli. It can be done by processing --classpath parameter. I'll try to make it tomorrow.

@remal is this PR then obsolete?

…t-api Signed-off-by: Semyon Levin <levin.semen@gmail.com>

remal · 2019-07-03T11:54:12Z

@schalkms I've pushed a new commit with moving version retrieval logic.

BTW I haven't tested it in Java >= 9 environment.

Signed-off-by: Semyon Levin <levin.semen@gmail.com>

schalkms · 2019-07-03T12:09:50Z

detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/internal/KotlinEnvironmentUtils.kt

@@ -95,18 +95,18 @@ fun createCompilerConfiguration(
 private fun Iterable<File>.getKotlinLanguageVersion(): LanguageVersion? {


You might want to unit test this single method.
Then we are also sure that it works on all JVM versions with the help of CI.

I've added a test. See KotlinEnvironmentUtilsTest.

This test does not:

Test the code against different versions of Kotlin

Test the code against different versions of JVM

PS: Asking for tests was very useful - I catched a bug :)

Signed-off-by: Semyon Levin <levin.semen@gmail.com>

arturbosch

Looks good to me, seems reasonable to support older kotlin versions when 1.4 comes out.
Please revert the continuous indent to 4 again, see this summary ticket for example JuulLabs/kotlin-code-styles#1 (comment)

KtLint deleted the option to configure the continuous indent and android kotlin guide uses 4. Also jetbrains reverted many places where 8 was used for example parameter lists. We will just stick with 4.
Kotlin/kotlin-style-guide#38

arturbosch · 2019-07-06T13:10:40Z

The test seems to fail on Java 9+, any idea why?

io.gitlab.arturbosch.detekt.api.internal.KotlinEnvironmentUtilsTest > should match FAILED
    kotlin.KotlinNullPointerException
        at io.gitlab.arturbosch.detekt.api.internal.KotlinEnvironmentUtilsTest$1$1$1.invoke(KotlinEnvironmentUtilsTest.kt:18)
        at io.gitlab.arturbosch.detekt.api.internal.KotlinEnvironmentUtilsTest$1$1$1.invoke(KotlinEnvironmentUtilsTest.kt:9)

Signed-off-by: Semyon Levin <levin.semen@gmail.com>

remal · 2019-07-09T20:04:51Z

@arturbosch I'm afraid I didn't understand what you meant by "revert the continuous indent to 4 again". Could you please give a specific example? I'd appreciate it if you could make a comment to a specific line.

I've fixed the test. Please take a look. It seems that Application ClassLoader is not an instance of URLClassLoader in Java 9...

schalkms

Thanks for adding the test.
IMHO this PR looks fine now! Thank you for that!
We will have to wait for Artur's final review.

arturbosch · 2019-07-14T17:48:48Z

detekt-cli/src/main/kotlin/io/gitlab/arturbosch/detekt/cli/CliArgs.kt

-            "Available 'report-id' values: 'txt', 'xml', 'html'. " +
-            "These can also be used in combination with each other " +
-            "e.g. '-r txt:reports/detekt.txt -r xml:reports/detekt.xml'")
+                "Entry should consist of: [report-id:path]. " +


The whole indentation in this file for example got reformatted to 8 continuous spaces instead of four.
I like the annotation parameter formatting though :).

Take a look. Is it OK now?

arturbosch · 2019-07-14T17:49:05Z

detekt-cli/src/main/kotlin/io/gitlab/arturbosch/detekt/cli/CliArgs.kt

        https://github.com/JetBrains/kotlin/blob/master/compiler/cli/cli-common/src/org/jetbrains/kotlin/cli/common/arguments/K2JVMCompilerArguments.kt
    */
    @Parameter(
        names = ["--classpath", "-cp"],
        description = "EXPERIMENTAL: Paths where to find user class files and depending jar files. " +
-            "Used for type resolution."
+                "Used for type resolution."


Like this ^^

Signed-off-by: Semyon Levin <levin.semen@gmail.com>

remal · 2019-07-17T17:06:20Z

@arturbosch Please take a look

arturbosch

Looks good, thank you very much!

Kotlin language version handling

63b2e23

Signed-off-by: Semyon Levin <levin.semen@gmail.com>

schalkms reviewed Jul 2, 2019

View reviewed changes

Update cli.md

cc6b804

Fix a misprint

schalkms reviewed Jul 2, 2019

View reviewed changes

remal added 2 commits July 3, 2019 14:36

Merge remote-tracking branch 'orig/master' into api-version-param

e155711

Language version retrieval has been moved from Gradle plugin to detek…

dca2dc2

…t-api Signed-off-by: Semyon Levin <levin.semen@gmail.com>

code formatting

9b93a1d

Signed-off-by: Semyon Levin <levin.semen@gmail.com>

schalkms reviewed Jul 3, 2019

View reviewed changes

getKotlinLanguageVersion() test

76afce4

Signed-off-by: Semyon Levin <levin.semen@gmail.com>

arturbosch reviewed Jul 6, 2019

View reviewed changes

remal added 2 commits July 9, 2019 22:50

Merge remote-tracking branch 'orig/master' into api-version-param

5fdfdcf

KotlinEnvironmentUtilsTest fix

45565d2

Signed-off-by: Semyon Levin <levin.semen@gmail.com>

schalkms approved these changes Jul 13, 2019

View reviewed changes

arturbosch reviewed Jul 14, 2019

View reviewed changes

remal added 3 commits July 15, 2019 17:42

Merge remote-tracking branch 'orig/master' into api-version-param

f0bdb44

code formatting

94d35f3

Signed-off-by: Semyon Levin <levin.semen@gmail.com>

code formatting

55dabaf

Signed-off-by: Semyon Levin <levin.semen@gmail.com>

arturbosch approved these changes Jul 27, 2019

View reviewed changes

Suppress long method

c0f64f0

arturbosch added this to the 1.0.0 milestone Jul 27, 2019

Merge branch 'master' into api-version-param

e003512

arturbosch merged commit 5e35fab into detekt:master Jul 28, 2019

		@@ -95,18 +95,18 @@ fun createCompilerConfiguration(
		private fun Iterable<File>.getKotlinLanguageVersion(): LanguageVersion? {

Uh oh!

Kotlin language version handling #1748

Kotlin language version handling #1748

Uh oh!

Conversation

remal commented Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

remal commented Jul 2, 2019

Uh oh!

remal commented Jul 2, 2019

Uh oh!

schalkms left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

schalkms commented Jul 2, 2019

Uh oh!

remal commented Jul 2, 2019

Uh oh!

schalkms left a comment

Choose a reason for hiding this comment

Uh oh!

schalkms Jul 2, 2019

Choose a reason for hiding this comment

Uh oh!

remal Jul 3, 2019

Choose a reason for hiding this comment

Uh oh!

remal commented Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schalkms commented Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

remal commented Jul 3, 2019

Uh oh!

schalkms Jul 3, 2019

Choose a reason for hiding this comment

Uh oh!

remal Jul 4, 2019

Choose a reason for hiding this comment

Uh oh!

arturbosch left a comment

Choose a reason for hiding this comment

Uh oh!

arturbosch commented Jul 6, 2019

Uh oh!

remal commented Jul 9, 2019

Uh oh!

schalkms left a comment

Choose a reason for hiding this comment

Uh oh!

arturbosch Jul 14, 2019

Choose a reason for hiding this comment

Uh oh!

remal Jul 15, 2019

Choose a reason for hiding this comment

Uh oh!

arturbosch Jul 14, 2019

Choose a reason for hiding this comment

Uh oh!

remal commented Jul 17, 2019

Uh oh!

arturbosch left a comment

Choose a reason for hiding this comment

Uh oh!

remal commented Jul 2, 2019 •

edited

Loading

codecov-io commented Jul 2, 2019 •

edited

Loading

remal commented Jul 2, 2019 •

edited

Loading

schalkms commented Jul 2, 2019 •

edited

Loading