Cleanup: reduce the number of warnings during a build #4547

dkhalanskyjb · 2025-10-17T08:20:55Z

No description provided.

murfel · 2025-10-20T10:55:09Z

Any advice on how to review this?

E.g. which parts were not the obvious fix for a warning?

dkhalanskyjb · 2025-10-20T11:09:08Z

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:

Removed @Suppress: if no new warnings/errors surfaced, then this @Suppress was already redundant, as its whole purpose is to hide warnings and errors we disagree with.
Removed type annotation on a function call: likewise, this is just historical baggage from when Kotlin was worse at inferring types.
@Suppress("DEPRECATION") added to tests that check the behavior of a deprecated API (using which causes a warning) whose correctness we still want to ensure.
Added ? to types coming from Java that the compiler now knows can be null.

There are a few non-trivial fixes, but most changes are just boilerplate.

murfel · 2025-10-20T12:36:38Z

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 @Suppress / type annotations did you attempt removing?

Removed @Suppress: if no new warnings/errors surfaced, then this @Suppress was already redundant, as its whole purpose is to hide warnings and errors we disagree with.

Removed type annotation on a function call: likewise, this is just historical baggage from when Kotlin was worse at inferring types.

dkhalanskyjb · 2025-10-20T12:56:58Z

Sure.

For @Suppress, the process was:

Observe a warning like "suppressing errors may break the behavior".
Attempt removing the @Suppress.
If a new error appears that was hidden by the @Suppress, restore the @Suppress.

Steps 2 and 3 can be replaced with just looking at the code or preliminary knowledge or assumptions, so some @Suppress annotations have been left untouched.

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.

murfel

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 (!!)
public modifiers 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)

murfel · 2025-10-27T10:51:07Z

kotlinx-coroutines-core/common/test/AtomicCancellationCommonTest.kt

        val mutex = Mutex(true) // locked mutex
        val job = launch(start = CoroutineStart.UNDISPATCHED) {
            expect(2)
            select<String> { // suspends


select<String> - <String> can also be removed

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?

murfel · 2025-10-27T10:53:56Z

kotlinx-coroutines-core/common/test/AtomicCancellationCommonTest.kt

        finish(4)
    }
-}
+}


There's a checkbox in Idea, "ensure every saved file ends with a line break"

murfel · 2025-10-27T10:59:11Z

kotlinx-coroutines-core/common/test/AtomicCancellationCommonTest.kt

        finish(4)
    }

+    @Suppress("DEPRECATION")


Could you add // Mutex.onLock, since there's no hint neither on github, nor in Idea after the suppress has been added

Here and in other entries of @Suppress("DEPRECATION")

Such comments can become outdated when additional APIs used in the test get deprecated. I'd rather not risk misleading the reader. The more reliable method to learn which particular API needs the suppression is to temporarily remove the annotation.

I see, but without the comment, there's no way to know which suppress the author meant when they wrote that suppress.

(Either way, would be nice to have all suppresses in the codebase either commented, or un-commented. Not necessarily by your PR, of course.)

murfel · 2025-10-27T11:05:27Z

kotlinx-coroutines-core/common/test/JobStatesTest.kt

 */
 class JobStatesTest : TestBase() {
    @Test
-    public fun testNormalCompletion() = runTest {


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?

kotlinx-coroutines-core/common/test/WithTimeoutDurationTest.kt

kotlinx-coroutines-test/common/test/RunTestTest.kt

reactive/kotlinx-coroutines-reactor/test/Check.kt

reactive/kotlinx-coroutines-reactor/test/ReactorContextTest.kt

ui/kotlinx-coroutines-android/testdata/r8-test-common.pro

ui/kotlinx-coroutines-android/testdata/r8-test-rules.pro

Command was: git grep -l '^ *public ' | grep /test/ | xargs sed -i 's/^\( *\)public /\1/g'

Upgrading to Kotlin 2.2.20 brought new warnings, which this commit fixes.

murfel · 2025-11-04T14:44:43Z

kotlinx-coroutines-core/jvm/test/exceptions/Stacktraces.kt

    .lines().joinToString("\n") // normalize line separators

-public fun String.count(substring: String): Int = split(substring).size - 1
+fun String.count(substring: String): Int = split(substring).size - 1


murfel

Approved subject to explicitly accepting/rejecting unresolved comments.

dkhalanskyjb requested a review from murfel October 17, 2025 08:20

dkhalanskyjb force-pushed the dkhalanskyjb/fix-some-warnings branch from 047a527 to 2c2bb7c Compare October 20, 2025 10:34

murfel reviewed Oct 27, 2025

View reviewed changes

dkhalanskyjb added 4 commits October 30, 2025 08:43

Cleanup: reduce the number of warnings during a build

3591801

Remove the public from test source sets

44927b6

Command was: git grep -l '^ *public ' | grep /test/ | xargs sed -i 's/^\( *\)public /\1/g'

Fix new warnings after a rebase

4b881f3

Upgrading to Kotlin 2.2.20 brought new warnings, which this commit fixes.

Remove one more unnecessary type boundary

b450ca9

dkhalanskyjb force-pushed the dkhalanskyjb/fix-some-warnings branch from 2c2bb7c to b450ca9 Compare October 30, 2025 10:21

dkhalanskyjb requested a review from murfel October 30, 2025 10:22

murfel reviewed Nov 4, 2025

View reviewed changes

murfel approved these changes Nov 4, 2025

View reviewed changes

Cleanup: reduce the number of warnings during a build #4547

Are you sure you want to change the base?

Cleanup: reduce the number of warnings during a build #4547

Uh oh!

Conversation

dkhalanskyjb commented Oct 17, 2025

Uh oh!

murfel commented Oct 20, 2025

Uh oh!

dkhalanskyjb commented Oct 20, 2025

Uh oh!

murfel commented Oct 20, 2025

Uh oh!

dkhalanskyjb commented Oct 20, 2025

Uh oh!

murfel left a comment

Choose a reason for hiding this comment

Easy to check

Harder to check

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

murfel left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants