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

Fixes for Kotlin 1.6.0-M1 #4139

Merged
merged 2 commits into from
Sep 30, 2021
Merged

Fixes for Kotlin 1.6.0-M1 #4139

merged 2 commits into from
Sep 30, 2021

Conversation

3flex
Copy link
Member

@3flex 3flex commented Sep 30, 2021

Fixing a couple of issues identified with a test build using Kotlin 1.6.0-M1

Using org.jetbrains.kotlin.utils.addToStdlib.safeAs caused type inference
failures in Kotlin 1.6.0-M1
Required in progressive mode on Kotlin 1.6.0-M1
@3flex 3flex changed the title Fixes for Kotlin 1.6 Fixes for Kotlin 1.6.0-M1 Sep 30, 2021
@codecov
Copy link

codecov bot commented Sep 30, 2021

Codecov Report

Merging #4139 (c9706c7) into main (4e265df) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               main    #4139   +/-   ##
=========================================
  Coverage     83.45%   83.45%           
  Complexity     3185     3185           
=========================================
  Files           463      463           
  Lines          9095     9095           
  Branches       1768     1768           
=========================================
  Hits           7590     7590           
  Misses          571      571           
  Partials        934      934           
Impacted Files Coverage Δ
...n/kotlin/io/gitlab/arturbosch/detekt/rules/Junk.kt 62.50% <ø> (ø)
.../arturbosch/detekt/rules/style/UseIsNullOrEmpty.kt 57.33% <ø> (ø)

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 4e265df...c9706c7. Read the comment docs.

@cortinico cortinico added this to the 1.19.0 milestone Sep 30, 2021
@cortinico cortinico added the housekeeping Marker for housekeeping tasks and refactorings label Sep 30, 2021
@chao2zhang chao2zhang merged commit 89b6cd2 into detekt:main Sep 30, 2021
@3flex 3flex deleted the kotlin-1.6.0-M1 branch September 30, 2021 21:47
}
exitProcess(result.exitCode())
Copy link
Member

Choose a reason for hiding this comment

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

Why? This doesn't seem right. If we are in the case of HandledArgumentViolation we want to exit the process with the correct exitCode. Right?

I imagine that this is because of the exhaustive enums. But I think that instead of else that doesn't give any information about which other cases go there we could use: null -> Unit or null -> { /* no-op */ }. I prefer the first one because with the second I always have a bad time: Does this case do nothing or does it return a lambda that does nothing?

Copy link
Member

Choose a reason for hiding this comment

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

Good catch. This is indeed a miss from my side about the behavioral change.

@@ -37,7 +37,7 @@ fun getIntValueForPsiElement(element: PsiElement): Int? {

fun KtClass.companionObject() = this.companionObjects.singleOrNull { it.isCompanion() }

inline fun <reified T : Any> Any.safeAs(): T? = this as? T
inline fun <reified T : Any> Any?.safeAs(): T? = this as? T
Copy link
Member

Choose a reason for hiding this comment

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

I imagine that you are adding this ? to match what org.jetbrains.kotlin.utils.addToStdlib.safeAs did, right? If it's that I'm fine. But I think that an extension function that when receives null always return null shouldn't handle that case. That responsability should be handled by the caller.

@BraisGabin
Copy link
Member

😂 I didn't realised that this was already merged

@3flex
Copy link
Member Author

3flex commented Oct 1, 2021

@BraisGabin thanks for your feedback, I've raised #4143 to address it.

3flex added a commit that referenced this pull request Oct 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
housekeeping Marker for housekeeping tasks and refactorings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants