Skip to content

KotlinWhenStringFilter: remove filtering for Scala classes #1224

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

KotlinWhenStringFilter: remove filtering for Scala classes #1224

wants to merge 1 commit into from

Conversation

gergelyfabian
Copy link
Contributor

This removes a Jacoco bug for some Scala 2.13 classes.

Bug report in bazel-contrib/rules_scala#1291.

@gergelyfabian
KotlinWhenStringFilter: remove filtering for Scala classes
44d3c98 
This removes a Jacoco bug for some Scala 2.13 classes.
@gergelyfabian gergelyfabian force-pushed the kotlin-when-filter-not-for-scala branch from d44bd7d to 44d3c98 Compare September 23, 2021 18:41
@marchof
Copy link
Member

marchof commented Sep 23, 2021 •
edited
Loading

@gergelyfabian Can you please provide an example class file which causes the NPE described in bazel-contrib/rules_scala#1291?

JaCoCo should never throw a NPE when processing valid class files. We should therefore fix the root cause and not just check the case when the class was compiled from Scala.

@gergelyfabian
Copy link
Contributor Author

@marchof Of course. Sorry I guess maybe I should have first opened an issue for this.

Here is a class that reproduces it (if compiled with Scala 2.13):

https://github.com/gergelyfabian/bazel-scala-example/blob/6d2926b4d6ea55ec4e836d7915f1a19fa45d031b/example-maven/src/main/scala/mypackage/Maven.scala#L9-L19

Should I provide it as a .class file?

@marchof
Copy link
Member

marchof commented Sep 23, 2021

Should I provide it as a .class file?

Yes, please. Thx!

@gergelyfabian
Copy link
Contributor Author

Sure, so how I generated the class file:

# bazel-scala-example, rules_scala_1291 branch:

$ bazel coverage //...

Fails with:
java.io.IOException: Error while analyzing mypackage/RulesScalaIssue1291Repro$.class.uninstrumented.
	at org.jacoco.core.analysis.Analyzer.analyzerError(Analyzer.java:168)
	at org.jacoco.core.analysis.Analyzer.analyzeClass(Analyzer.java:140)
	at com.google.testing.coverage.JacocoCoverageRunner.analyzeStructure(JacocoCoverageRunner.java:180)
	at com.google.testing.coverage.JacocoCoverageRunner.create(JacocoCoverageRunner.java:125)
	at com.google.testing.coverage.JacocoCoverageRunner$2.run(JacocoCoverageRunner.java:545)
Caused by: java.lang.NullPointerException
	at org.jacoco.core.internal.analysis.MethodCoverageCalculator.ignore(MethodCoverageCalculator.java:158)
	at org.jacoco.core.internal.analysis.filter.KotlinWhenStringFilter$Matcher.match(KotlinWhenStringFilter.java:96)
	at org.jacoco.core.internal.analysis.filter.KotlinWhenStringFilter.filter(KotlinWhenStringFilter.java:37)
	at org.jacoco.core.internal.analysis.filter.Filters.filter(Filters.java:57)
	at org.jacoco.core.internal.analysis.ClassAnalyzer.addMethodCoverage(ClassAnalyzer.java:110)
	at org.jacoco.core.internal.analysis.ClassAnalyzer.access$100(ClassAnalyzer.java:31)
	at org.jacoco.core.internal.analysis.ClassAnalyzer$1.accept(ClassAnalyzer.java:99)
	at org.jacoco.core.internal.flow.ClassProbesAdapter$2.visitEnd(ClassProbesAdapter.java:89)
	at com.google.testing.coverage.jarjar.org.objectweb.asm.ClassReader.readMethod(ClassReader.java:1491)
	at com.google.testing.coverage.jarjar.org.objectweb.asm.ClassReader.accept(ClassReader.java:717)
	at com.google.testing.coverage.jarjar.org.objectweb.asm.ClassReader.accept(ClassReader.java:401)
	at org.jacoco.core.analysis.Analyzer.analyzeClass(Analyzer.java:122)
	at org.jacoco.core.analysis.Analyzer.analyzeClass(Analyzer.java:138)
	... 3 more

$ unzip bazel-out/k8-fastbuild/bin/example-maven/example-maven-offline.jar

# File is in mypackage/RulesScalaIssue1291Repro$.class.uninstrumented

What would be the preferred way to provide you the file?

@marchof
Copy link
Member

marchof commented Sep 23, 2021

Just attach the jar here.

@Godin Godin added the reproducer required Further information is requested label Sep 23, 2021
@gergelyfabian
Copy link
Contributor Author

example-maven-offline.zip

I renamed the jar to zip, to be able to attach it.
The reproduction is in mypackage/RulesScalaIssue1291Repro$.class.uninstrumented after unpacking.

@marchof marchof removed the reproducer required Further information is requested label Sep 23, 2021
@Godin
Copy link
Member

Godin commented Sep 23, 2021

Stacktrace

Caused by: java.lang.NullPointerException
	at org.jacoco.core.internal.analysis.MethodCoverageCalculator.ignore(MethodCoverageCalculator.java:158)
	at org.jacoco.core.internal.analysis.filter.KotlinWhenStringFilter$Matcher.match(KotlinWhenStringFilter.java:96)

indicates that there should be call of ignore at line 96 in KotlinWhenStringFilter.

Iterating by our tags from most recent v0.8.7, I see that ignore was there in 3 years old v0.8.3 - https://github.com/jacoco/jacoco/blob/v0.8.3/org.jacoco.core/src/org/jacoco/core/internal/analysis/filter/KotlinWhenStringFilter.java#L96

And between v0.8.3 and v0.8.7

git log v0.8.3...v0.8.7 -- org.jacoco.core/src/org/jacoco/core/internal/analysis/filter/KotlinWhenStringFilter.java

there was fix #942 in version 0.8.5

Also using JaCoCo Command Line Interface

version 0.8.7 for provided mypackage/RulesScalaIssue1291Repro$.class.uninstrumented does not throw NPE:

java -jar jacoco-0.8.7/lib/jacococli.jar report --classfiles example-maven-offline/mypackage/RulesScalaIssue1291Repro$.class.uninstrumented
[WARN] No execution data files provided.
[INFO] Analyzing 1 classes.

whereas version 0.8.3 does:

java -jar jacoco-0.8.3/lib/jacococli.jar report --classfiles example-maven-offline/mypackage/RulesScalaIssue1291Repro$.class.uninstrumented
[WARN] No execution data files provided.
Exception in thread "main" java.io.IOException: Error while analyzing example-maven-offline/mypackage/RulesScalaIssue1291Repro$.class.uninstrumented.
        at org.jacoco.cli.internal.core.analysis.Analyzer.analyzerError(Analyzer.java:168)
        at org.jacoco.cli.internal.core.analysis.Analyzer.analyzeClass(Analyzer.java:140)
        at org.jacoco.cli.internal.core.analysis.Analyzer.analyzeClass(Analyzer.java:163)
        at org.jacoco.cli.internal.core.analysis.Analyzer.analyzeAll(Analyzer.java:199)
        at org.jacoco.cli.internal.core.analysis.Analyzer.analyzeAll(Analyzer.java:232)
        at org.jacoco.cli.internal.commands.Report.analyze(Report.java:109)
        at org.jacoco.cli.internal.commands.Report.execute(Report.java:83)
        at org.jacoco.cli.internal.Main.execute(Main.java:89)
        at org.jacoco.cli.internal.Main.main(Main.java:104)
Caused by: java.lang.NullPointerException
        at org.jacoco.cli.internal.core.internal.analysis.MethodCoverageCalculator.ignore(MethodCoverageCalculator.java:158)
        at org.jacoco.cli.internal.core.internal.analysis.filter.KotlinWhenStringFilter$Matcher.match(KotlinWhenStringFilter.java:96)
        at org.jacoco.cli.internal.core.internal.analysis.filter.KotlinWhenStringFilter.filter(KotlinWhenStringFilter.java:37)
        at org.jacoco.cli.internal.core.internal.analysis.filter.Filters.filter(Filters.java:57)
        at org.jacoco.cli.internal.core.internal.analysis.ClassAnalyzer.addMethodCoverage(ClassAnalyzer.java:110)
        at org.jacoco.cli.internal.core.internal.analysis.ClassAnalyzer.access$100(ClassAnalyzer.java:31)
        at org.jacoco.cli.internal.core.internal.analysis.ClassAnalyzer$1.accept(ClassAnalyzer.java:99)
        at org.jacoco.cli.internal.core.internal.flow.ClassProbesAdapter$2.visitEnd(ClassProbesAdapter.java:89)
        at org.jacoco.cli.internal.asm.ClassReader.readMethod(ClassReader.java:1279)
        at org.jacoco.cli.internal.asm.ClassReader.accept(ClassReader.java:679)
        at org.jacoco.cli.internal.asm.ClassReader.accept(ClassReader.java:391)
        at org.jacoco.cli.internal.core.analysis.Analyzer.analyzeClass(Analyzer.java:122)
        at org.jacoco.cli.internal.core.analysis.Analyzer.analyzeClass(Analyzer.java:138)
        ... 7 more

So @gergelyfabian are you sure that you're using latest released JaCoCo version (0.8.7 as of today)?

@Godin Godin added the feedback pending Further information is requested label Sep 23, 2021
@Godin
Copy link
Member

Godin commented Sep 23, 2021

In addition to inability to reproduce with version 0.8.7, I think that this is really duplicate of #942 also because javap -v -p for provided mypackage/RulesScalaIssue1291Repro$.class.uninstrumented shows the same bytecode pattern that led us to fix #942 :

        10: aload_3
        11: invokevirtual #23                 // Method java/lang/String.hashCode:()I
        14: lookupswitch  { // 0
                 default: 24
            }

@Godin Godin mentioned this pull request Sep 23, 2021
Merged
@gergelyfabian
Copy link
Contributor Author

Thanks @Godin! I'm using Jacoco 0.8.3 that's used by the current Bazel release (4.1.0). To be more exact, more frequently I use a custom version of Jacoco, that contains some fixes on top of 0.8.3 (that's the only compatible version with last released Bazel).

So if this is a duplicate of #942, then I'll try to backport the fix for #942 on the custom branch of Jacoco that I use, and I should be done (and be safe that a later upgrade of Jacoco in Bazel will remove the need for my custom branch of Jacoco).

@gergelyfabian
Copy link
Contributor Author

I can just confirm, that backporting the fix for #942 fixes this issue.

@Godin Godin added declined: duplicate ❌ This issue or pull request already exists and removed feedback pending Further information is requested labels Sep 24, 2021
@Godin Godin closed this Sep 24, 2021
@gergelyfabian gergelyfabian mentioned this pull request Sep 24, 2021
Open
@gergelyfabian gergelyfabian deleted the kotlin-when-filter-not-for-scala branch September 24, 2021 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Godin Godin

Labels
declined: duplicate ❌ This issue or pull request already exists
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gergelyfabian @marchof @Godin