arrow icon indicating copy to clipboard operation
arrow copied to clipboard

["Pitfall"] Inline functions lower coverage reports

Open pakoito opened this issue 2 years ago • 3 comments

What version are you currently using? 1.0.1

What would you like to see? When using Validated#zip for my branch coverage not to drop.

We found this one semi-accidentally on our jacoco reports. Turns out that the deep inline of zip

https://github.com/arrow-kt/arrow/blob/main/arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/Validated.kt#L1026 https://github.com/arrow-kt/arrow/blob/main/arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/Validated.kt#L830-L848

ends in this single function

https://github.com/arrow-kt/arrow/blob/9928d163aaa225df3dc050da6914f603ff8c650e/arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/Validated.kt#L998-L1023

that once inlined introduces many branches that aren't executed...in every call!

My suggestion is to not inline those termination points.

pakoito avatar Nov 11 '21 11:11 pakoito

Sounds great! We should probably do same for all other zip functions.

nomisRev avatar Nov 11 '21 11:11 nomisRev

My suggestion is to not inline those termination points.

We should still inline to allow suspension, so we need concrete implementations for all of them. Right?

nomisRev avatar Nov 11 '21 14:11 nomisRev

Make the failure case (L1001-10021) a non-inline function and that should fix it. Either it's success or has some error, which calls a single non-inline function.

pakoito avatar Nov 11 '21 19:11 pakoito

I don't think we want to optimise for coverage reports. I've noticed in some cases it still behaves weirdly, but I prefer to have the API more optimal than the coverage 😅. Closing this issue for now, feel free top open it again or re-ignite a discussion.

Doing some housekeeping. 😘

nomisRev avatar Apr 05 '23 07:04 nomisRev