Cleanup: reduce the number of warnings during a build #4547
Conversation
dkhalanskyjb
force-pushed
the
dkhalanskyjb/fix-some-warnings
branch
from
047a527 to
2c2bb7c
Compare
|
Any advice on how to review this? E.g. which parts were not the obvious fix for a warning? |
|
The majority of changed lines follow one of several patterns, so it's enough to understand an edit once, and then the rest of the instances become clear as well. Examples:
There are a few non-trivial fixes, but most changes are just boilerplate. |
|
Could you describe the process for these two? The process itself is partly proof of correctness (and the other part is the clean compilation). E.g. which
|
|
Sure. For
Steps 2 and 3 can be replaced with just looking at the code or preliminary knowledge or assumptions, so some For removing type annotations: when I was fixing some warnings in a file, I also went through the suggestion provided by the IDE, considered if applying them is proper, and then I either did that or reported a broken suggestion to the IDE team. |
There was a problem hiding this comment.
Thank you for taking this cleanup on! I was pretty annoyed with all the warnings.
? in my comments mean please provide context/reference/proof for the correctness of the change.
I'd also like a prove of completeness of certain replacements. E.g. as if with the publc modifier in test, the proof is empty grep in Idea. The proof can come just as a confirmation statement, "grep for xxx in directory yyy is empty", then I can check it in O(1).
For now I'm not sure when you did or didn't aim for completeness.
I also not sure how you discovered the warnings. Did you use compilation warnings only? If I run "inspect project" in Idea, I see many more warnings, some of them similar to the ones fixed here.
Just FYI: here's a context to which changes are easier to check during review
Easy to check
Removals
@Suppress- inferable type parameters
- not-null assertions (!!)
publicmodifiers from tests- imports
These removals are easy because if the CI passes after the change, then the change is valid. Hence, I don't need to think about it.
And other things
- Rename the unused parameter in the catch clause to
_
Harder to check
- Adding suppresses (since we don't have a check "this suppress doesn't suppress anything")
- Loosening the type to nullable (adding ?)
- Adding not-null assertion (!!)
- Changes not triggered by inspections (If I undo the change, I don't see any static warning that triggers me)
| val mutex = Mutex(true) // locked mutex | ||
| val job = launch(start = CoroutineStart.UNDISPATCHED) { | ||
| expect(2) | ||
| select<String> { // suspends |
There was a problem hiding this comment.
select<String> - <String> can also be removed
There was a problem hiding this comment.
Not sure how you discovered the other entries for the "Remove explicit type arguments" inspection, by hand or by following warnings. If the latter, not sure why this and other few cases were missed. Possibly also clean them up?
| finish(4) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
There's a checkbox in Idea, "ensure every saved file ends with a line break"
| finish(4) | ||
| } | ||
|
|
||
| @Suppress("DEPRECATION") |
There was a problem hiding this comment.
Could you add // Mutex.onLock, since there's no hint neither on github, nor in Idea after the suppress has been added
There was a problem hiding this comment.
Here and in other entries of @Suppress("DEPRECATION")
| */ | ||
| class JobStatesTest : TestBase() { | ||
| @Test | ||
| public fun testNormalCompletion() = runTest { |
There was a problem hiding this comment.
There's another 15 entries for public fun in common/test, and most of them don't need the public modifier. Could you get rid of them as well?
| @Test | ||
| fun testExceptionFromWithinTimeout() = runTest { | ||
| expect(1) | ||
| @Suppress("UNREACHABLE_CODE") |
There was a problem hiding this comment.
Please add // expectUnreached() here and everywhere else
There was a problem hiding this comment.
Do you happen to know why this particular expectUnreached() triggered the inspection, but many others didn't?
There was a problem hiding this comment.
expectUnreached() is not some specific construct, it's just a function call. The inspection gets triggered when a value of type Nothing gets returned. Nothing is a type with no values, and you can only return it by throwing an exception or having an infinite loop in your code. Here, withTimeout returns Nothing, because its body never produces a normal value, and the compiler can recognize that. If we add a type parameter (withTimeout<Int>, for example), the compiler won't be able to do that.
| } | ||
| } | ||
|
|
||
| /** Tests that even the dispatch timeout of `0` is fine if all the dispatches go through the same scheduler. */ |
There was a problem hiding this comment.
To have one Suppress for a group of tests.
|
|
||
| # We are cheating a bit by not having android.jar on R8's library classpath. Ignore those warnings. | ||
| -ignorewarnings | ||
| -dontwarn |
There was a problem hiding this comment.
Try running :kotlinx-coroutines-android:runR8 with and without this change, and the difference should be clear. ignorewarnings doesn't suppress the warnings we're not interested in.
| -checkdiscard class kotlinx.coroutines.DebugKt | ||
| -checkdiscard class kotlinx.coroutines.internal.StackTraceRecoveryKt | ||
| -checkdiscard class kotlinx.coroutines.debug.DebugProbesKt | ||
| -checkdiscard class kotlinx.coroutines.debug.internal.DebugProbesKt |
There was a problem hiding this comment.
There's no kotlinx.coroutines.debug.DebugProbesKt now, ever since it DebugProbes.kt got moved to the internal package, so the rule was meaningless before the change.
No description provided.