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

Kotlin inline functions are not marked as covered #654

Open
cplain opened this issue Feb 25, 2018 · 30 comments
Open

Kotlin inline functions are not marked as covered #654

cplain opened this issue Feb 25, 2018 · 30 comments

Comments

@cplain
Copy link

cplain commented Feb 25, 2018

A similar issue was raised here for Jetbrains coverage tools:
https://youtrack.jetbrains.com/issue/KT-12605
with the ultimate resolution being tracked here:
https://youtrack.jetbrains.com/issue/IDEA-185485

Although the former ticket mentions raising an issue on this project as far as I could see none has been

Steps to reproduce

fun contrivedExample(condition: Boolean) {
    anInlineMethod(condition) { printLn("A thing happened") }
}

inline fun anInlineMethod(condition: Boolean, callback: () -> Unit) {
  if (condition) {
    callback.invoke()
  }
}

Expected behaviour

If I write a test with contrivedExample(condition = true), anInlineMethod should be considered covered

Actual behaviour

It isn't

JaCoCo version: 0.7.9
Operating system: macOS 10.13.2
Tool integration: Gradle

@marchof
Copy link
Member

marchof commented Feb 26, 2018

Some remarks:

  1. When the compiler assignes correct line numbers to the inlined code source highlighting should work. javac does so for lambda bodies
  2. If the inlined method is still present in the compiled class file we need to get some hint that the method should be excluded (See Recognize and ignore Kotlin-generated code #552).
  3. If the method is inlined in multiple methods and not fully executed in one of the methods this will result in partial coverage.

@yole
Copy link

yole commented Mar 16, 2018

Just to clarify: The main issue with line numbers and inline functions is that inline functions can be declared in different source files, so the .class file contains line numbers from different source files. This can't be expressed in a LineNumberTable, so Kotlin creates synthetic line numbers for lines that were inlined from other source files; those line numbers go beyond the line numbers in the original source file. The actual mapping between line numbers and original source files is stored in JSR-45 strata, which the compiler also generates.

JaCoCo needs to understand this; it's not something that can be changed on the Kotlin compiler side.

@snowe2010

This comment has been minimized.

@mformetal

This comment has been minimized.

@snowe2010
Copy link

... Why were our comments marked as spam? There have been no updates on this issue for a year, it's not unreasonable to ask for an update from the maintainers. I'm not saying you have to fix anything, but marking stuff as spam seems very uncalled for.

@Godin
Copy link
Member

Godin commented Apr 10, 2019

@snowe2010 given that development happens publicly, GitHub does damn good job in showing all updates (see "Assignes" and "Milestones" field, linked tickets and other events, etc), so all repetative questions "any update?" that can only be followed by answer "no, all updates already shown in GitHub UI" do not add any useful information (unlike for example #654 (comment)) and so were hidden.

@snowe2010
Copy link

I can clearly see the Milestones field is not filled out, therefore there is no tracking. I have also visited the other linked tickets (before commenting the first time, mind you), and they are not talking about progress on this ticket, they are talking about progress on their own tickets and just happen to be referencing this one saying that they are in fact blocked on this ticket.

I see in no way that development on this ticket is happening publicly. Either you have made a decision to work on this and have not updated the Milestones/Issue/Assignees to reflect it, or have decided this is not worth doing and also not updated the Issue. Both of which are not updates. Lack of information is not information.

In the future a better way of responding would be to state that the architecture for the problem at hand is still being worked on and to invite conversation. Anyone coming into this ticket has no clue why development has stalled or what they can do to help out. If you need help with a ticket then mark it as needing help.

Finally, I know being an open source maintainer is hard. I maintain some small projects (that have a small user base) and I understand that you are doing this of your own good will. But, flagging comments that are requesting information doesn't help the ticket move along, it just makes people not want to help out.

@Godin
Copy link
Member

Godin commented Apr 11, 2019

IMO statement "I'd like to work on this ticket" sounds quite different and seems more close to initial intention than the question "is this any closer to being resolved".

Tickets #763 and #764 are not blocked on this, but opposite - they added ability to read "JSR-45 strata" which was mentioned here in #654 (comment)

Absence of information literally means that there is no other information to add - there is no hidden work by us on architecture for this ticket outside of GitHub, as well as for any other open ticket. And we have not decided that this ticket is not worth to work on/look at, because in this case it will be simply closed. And yes tickets can stay open without milestone and assignee for some time because we work on this project in our spare time when we have it.

"help wanted" label can be added here, but then one can say that this is kind of unfair in respect to all other open ticket, and another can say that history shows that such label doesn't help at all.

Wanna help? Please welcome - have a look at how Kotlin compiler inlines functions, what is provided via JSR-45 and how JaCoCo performs analysis of class files to come up with an idea or implementation of how this can be solved.

Hope this addresses your question and thank you for your understanding.

@Godin Godin added the help wanted 🖖 We'd like help with this issue label Apr 11, 2019
@snowe2010
Copy link

WHOOPS. forgot to respond to this! haha. Here's what I had written out a month ago and not clicked send on.


Maybe in the future instead of asking for 'any update?' I should ask "what is required for this ticket to move forward" as usually that is exactly what I mean. I do understand that adding "help wanted" can be unfair, because maybe that deprioritizes other tickets.

elizarov added a commit to Kotlin/kotlinx.coroutines that referenced this issue Aug 27, 2019
It has the following limitations:

* Jacoco does not properly understand Kotlin inline classes and they
  are not considered to be covered. See:
  jacoco/jacoco#654
* Jacoco does not support Kotlin's convention for omitting common
  package prefix in source files, so it cannot find the corresponding
  source files and does not provide navigation to them.
@senioroman4uk
Copy link

Hi, I would like to try implementing it, if noone objects

@jhannink
Copy link

jhannink commented Aug 19, 2020

Any progress on this?
https://youtrack.jetbrains.com/issue/KT-12605#focus=Comments-27-2680423.0-0 seems to have found a fix for the intellij-coverage tool and claims the same could be done for jacoco?

@dasAnderl
Copy link

this was actually working with kotlin < 1.4 and jacoco 0.8.5.201910111838 gradle > 6
changing kotlin to 1.4 leaves inline funcs as uncovered

gnarea added a commit to relaycorp/awala-poweb-jvm that referenced this issue Sep 8, 2020
gnarea added a commit to relaycorp/awala-poweb-jvm that referenced this issue Sep 8, 2020
gnarea added a commit to relaycorp/awala-poweb-jvm that referenced this issue Sep 8, 2020
@CoenQian
Copy link

CoenQian commented Nov 2, 2020

IDEA code coverage supports inline function now.
image

@andrejordan
Copy link

Any progress on this?

ulfsauer0815 added a commit to ulfsauer0815/assertj-jsoup that referenced this issue Nov 14, 2020
Inline fail methods that throw exceptions to work around Jacoco limitation of not being able to detect coverage of those calls.
Ignoring coverage for inlined functions with @generated annotation until support is added in Jacoco (jacoco/jacoco#654)
Performance impact of inline funs should be negligible
@adrianotelesc
Copy link

Any progress?

@marchof
Copy link
Member

marchof commented Jul 25, 2021

Absence of information literally means that there is no other information to add - there is no hidden work by us on architecture for this ticket outside of GitHub, as well as for any other open ticket.

@adrianotelesc Let us know what exactly is not clear about this statement. 😉

@cristan
Copy link

cristan commented Nov 23, 2021

A workaround is to use Kotlin/Kotlinx-Kover instead. It's name starts with a K, so you can be sure it's fully compatible with Kotlin 😁. The reports are compatible with JaCoCo, so it should be easy to migrate. It's very new though: currently, the latest released version is only 0.4.2.

@joost-klitsie
Copy link

I just ran into this issue as well, it is a bit annoying as it can break the Sonar quality gates. Is there anything we can do about this?

@pandelisgreen13
Copy link

any update ?

@binkley
Copy link

binkley commented Apr 22, 2023

This is a tough card to evaluate.

  • One the one hand, it makes sense that JaCoCo doesn't spot this as a general JVM coverage tool with special support for Java.
  • On the other hand, Kotlin provides metadata bytecode class headers to help tooling like JaCoCo (also to support reflection). How would JaCoCo know that bytecode sequence "X" were inlined a function, or were written out in the source of the function under coverage? (I'm unclear on how Kotlin metadata works, so I may have the details wrong -- more here: https://kotlinlang.org/api/latest/jvm/stdlib/kotlin/-metadata/.)
  • On the gripping hand, Kover is in alpha stage, so not ready for primetime in production projects.

As many before have asked, any update on this card?

@marchof
Copy link
Member

marchof commented Apr 24, 2023

As many before have asked, any update on this card?

@binkley See #654 (comment)

@tasomaniac
Copy link

Maybe then this should be just closed and mark as "Won't Do"

LZRS added a commit to opensrp/fhircore that referenced this issue Jul 29, 2023
Since they're not getting tracked in our codcov
jacoco/jacoco#654
LZRS added a commit to opensrp/fhircore that referenced this issue Jul 29, 2023
Since they're not getting tracked in our codcov
jacoco/jacoco#654
evance-mose added a commit to opensrp/fhircore that referenced this issue Jul 31, 2023
* Update sdc to v1.0.0-preview14.3-SNAPSHOT

* Add tests in android extension

* update tests in android extension

* Replace inline launchQuestionnaire extension functions

Since they're not getting tracked in our codcov
jacoco/jacoco#654

---------

Co-authored-by: L≡ZRS <12814349+LZRS@users.noreply.github.com>
@jtaub
Copy link

jtaub commented Aug 9, 2023

What is required for this ticket to move forward?

@marchof
Copy link
Member

marchof commented Aug 9, 2023

@jtaub A good idea how JaCoCo can be reworked to support a N:M relationship between class files and source files. JaCoCo's coverage model assumes that each class file comes from at most one source file. Inlining means that a single class file is sourced from multiple source files.

Another problem with inlining is that code is executed in classes which are not analyzed today: For example if you write unit tests the code will be inlined in the test classes itself -- which are not in scope of the coverage analysis at all.

So from the JaCoCo project side is it very unlikely that we will come up with a solution. Maybe Kotlin compiler has a compiler option to avoid inlining? Then this option can be used when building for unit testing.

@yole
Copy link

yole commented Aug 9, 2023

In Kotlin, inlining is a language feature that affects the semantics of the code, and not just a performance optimization. Therefore, an option to avoid inlining does not exist and cannot be added.

@marchof
Copy link
Member

marchof commented Aug 10, 2023

In Kotlin, inlining is a language feature that affects the semantics of the code, and not just a performance optimization. Therefore, an option to avoid inlining does not exist and cannot be added.

In this case it is even more unlikely we can solve this on bytecode level. Maybe one day a Kotlin experts shows up here and solves it.

@Keith-Albright-Bose
Copy link

adding a vote to please address this if possible!!! Must use inlining due to use of reified...

@yk-jemmic
Copy link

as a workaround, I'm writing a java unit test that calls these kotlin inline functions

@viva-la-v
Copy link

viva-la-v commented Nov 3, 2023

as a workaround, I'm writing a java unit test that calls these kotlin inline functions

Thanks a lot, this workaround is the closest approach to elegance I found.

And since this does work, it indicates that kotlin compiler did compiled the inline functions into static functions in bytecode (at least when it's called in java). But who knows why they hide the access/visibility to it from kotlin itself. If only they did not do this.

@jtaub
Copy link

jtaub commented Feb 1, 2024

Unfortunately, calling the function from Java is not an option if you're using reified types, which is one of the main motivations for inlining a function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests