Skip to content

Commit

Permalink
fix: scoverage statement's line number should be 1-base (#18932)
Browse files Browse the repository at this point in the history
fix #18916

This PR makes scoverage statements' line number 1-base, instead of
0-base. FYI @tarao

It would be ideal if we could find specifications of many of coverage
report fomats (such as Jacoco XML, cobertura.xml, lcov.txt...) and
confirm most of them expect the line numbers 1-base.

However, it seems there's no formal specification for them AFAIK. Since
we've been using 1-base so far in Scala2 and it hasn't been a problem,
so I guess it's OK to change to 1-base (and most of coverage report
format and visualizer expect 1-base probably).

I believe we should make coverage report 1-base by default, and it
should be reporter / aggregator's responsibility to adjust the line
numbers if some coverage format expects 0-base.

---

I tested on https://github.com/scoverage/sbt-scoverage-samples with HTML
reporter

## 3.3.1

![Screenshot 2023-11-15 at 16 55
06](https://github.com/lampepfl/dotty/assets/9353584/909b7eeb-22ca-41ca-bdfb-dc1e3ceb8ebb)

![Screenshot 2023-11-15 at 16 55
11](https://github.com/lampepfl/dotty/assets/9353584/97f086e2-f597-40e8-bee0-7a5d2dac9294)

In the latter picture, they are "3" even though the actual line number
is 4, because

- scoverage deserializer parses `scoverage.coverage`'s line number as it
is
-
https://github.com/scoverage/scalac-scoverage-plugin/blob/dd519767c916293f6d200639641a7f029970e261/serializer/src/main/scala/scoverage/serialize/Serializer.scala#L147
- and StatementWriter uses the `line` number without adjustment (to
1-base)
-
https://github.com/scoverage/scalac-scoverage-plugin/blob/dd519767c916293f6d200639641a7f029970e261/reporter/src/main/scala/scoverage/reporter/StatementWriter.scala#L34-L36

## 3.4.0-RC1-bin-SNAPSHOT
![Screenshot 2023-11-15 at 17 41
06](https://github.com/lampepfl/dotty/assets/9353584/6cbcaebd-3fae-4fcb-ae22-fc4bd844563a)

seems ok 👍

![Screenshot 2023-11-15 at 17 41
01](https://github.com/lampepfl/dotty/assets/9353584/ca16653f-267c-4e43-962c-abdbf277f30f)
  • Loading branch information
nicolasstucki authored Nov 17, 2023
2 parents 772be76 + 86aaea9 commit f3a6ce3
Show file tree
Hide file tree
Showing 39 changed files with 420 additions and 413 deletions.
7 changes: 6 additions & 1 deletion compiler/src/dotty/tools/dotc/coverage/Coverage.scala
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,12 @@ class Coverage:

def addStatement(stmt: Statement): Unit = statementsById(stmt.id) = stmt

/** A statement that can be invoked, and thus counted as "covered" by code coverage tools. */

/**
* A statement that can be invoked, and thus counted as "covered" by code coverage tools.
*
* @param line 1-indexed line number
*/
case class Statement(
location: Location,
id: Int,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,9 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer:
id = id,
start = pos.start,
end = pos.end,
line = pos.line,
// +1 to account for the line number starting at 1
// the internal line number is 0-base https://github.com/lampepfl/dotty/blob/18ada516a85532524a39a962b2ddecb243c65376/compiler/src/dotty/tools/dotc/util/SourceFile.scala#L173-L176
line = pos.line + 1,
desc = sourceFile.content.slice(pos.start, pos.end).mkString,
symbolName = tree.symbol.name.toSimpleName.toString,
treeName = tree.getClass.getSimpleName.nn,
Expand Down
26 changes: 13 additions & 13 deletions tests/coverage/pos/Constructor.scoverage.check
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ covtest.C
<init>
28
36
3
4
<init>
DefDef
false
Expand All @@ -44,7 +44,7 @@ covtest.C
<init>
69
72
5
6
g
Apply
false
Expand All @@ -61,7 +61,7 @@ covtest.C
<init>
80
88
8
9
<init>
DefDef
false
Expand All @@ -78,7 +78,7 @@ covtest.C
<init>
108
128
9
10
+
Apply
false
Expand All @@ -95,7 +95,7 @@ covtest.C
f
133
138
11
12
f
DefDef
false
Expand All @@ -112,7 +112,7 @@ covtest.C
x
153
158
12
13
x
DefDef
false
Expand All @@ -129,7 +129,7 @@ covtest.C
<init>
165
169
13
14
f
Apply
false
Expand All @@ -146,7 +146,7 @@ covtest.C
<init>
167
168
13
14
x
Select
false
Expand All @@ -163,7 +163,7 @@ covtest.C
g
173
178
15
16
g
DefDef
false
Expand All @@ -180,7 +180,7 @@ covtest.O$
g
203
208
18
19
g
DefDef
false
Expand All @@ -197,7 +197,7 @@ covtest.O$
y
223
228
19
20
y
DefDef
false
Expand All @@ -214,7 +214,7 @@ covtest.O$
<init>
235
239
20
21
g
Apply
false
Expand All @@ -231,7 +231,7 @@ covtest.O$
<init>
237
238
20
21
y
Ident
false
Expand Down
12 changes: 6 additions & 6 deletions tests/coverage/pos/ContextFunctions.scoverage.check
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ covtest.OnError
onError
56
67
3
4
onError
DefDef
false
Expand All @@ -44,7 +44,7 @@ covtest.Imperative
readName2
121
134
7
8
readName2
DefDef
false
Expand All @@ -61,7 +61,7 @@ covtest.Imperative
readPerson
252
309
13
14
onError
Apply
false
Expand All @@ -78,7 +78,7 @@ covtest.Imperative
readPerson
252
295
13
14
<init>
Apply
false
Expand All @@ -95,7 +95,7 @@ covtest.Imperative
$anonfun
267
294
13
14
apply
Apply
false
Expand All @@ -112,7 +112,7 @@ covtest.Imperative
readPerson
192
206
11
12
readPerson
DefDef
false
Expand Down
34 changes: 17 additions & 17 deletions tests/coverage/pos/Enum.scoverage.check
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ covtest.Planet
surfaceGravity
338
356
14
15
surfaceGravity
DefDef
false
Expand All @@ -44,7 +44,7 @@ covtest.Planet
surfaceWeight
444
458
15
16
surfaceGravity
Select
false
Expand All @@ -61,7 +61,7 @@ covtest.Planet
surfaceWeight
392
409
15
16
surfaceWeight
DefDef
false
Expand All @@ -78,7 +78,7 @@ covtest.EnumTypes$
test
1043
1077
30
31
println
Apply
false
Expand All @@ -95,7 +95,7 @@ covtest.EnumTypes$
test
1051
1076
30
31
+
Apply
false
Expand All @@ -112,7 +112,7 @@ covtest.EnumTypes$
test
1082
1103
31
32
println
Apply
false
Expand All @@ -129,7 +129,7 @@ covtest.EnumTypes$
test
1090
1102
31
32
s
Apply
false
Expand All @@ -146,7 +146,7 @@ covtest.EnumTypes$
calculateEarthWeightOnPlanets
1195
1222
34
35
surfaceGravity
Select
false
Expand All @@ -163,7 +163,7 @@ covtest.EnumTypes$
calculateEarthWeightOnPlanets
1229
1320
35
36
foreach
Apply
false
Expand All @@ -180,7 +180,7 @@ covtest.EnumTypes$
calculateEarthWeightOnPlanets
1238
1251
35
36
refArrayOps
Apply
false
Expand All @@ -197,7 +197,7 @@ covtest.EnumTypes$
$anonfun
1263
1320
36
37
println
Apply
false
Expand All @@ -214,7 +214,7 @@ covtest.EnumTypes$
$anonfun
1271
1319
36
37
s
Apply
false
Expand All @@ -231,7 +231,7 @@ covtest.EnumTypes$
$anonfun
1296
1317
36
37
surfaceWeight
Apply
false
Expand All @@ -248,7 +248,7 @@ covtest.EnumTypes$
calculateEarthWeightOnPlanets
1109
1142
33
34
calculateEarthWeightOnPlanets
DefDef
false
Expand All @@ -265,7 +265,7 @@ covtest.EnumTypes$
test
1326
1347
38
39
println
Apply
false
Expand All @@ -282,7 +282,7 @@ covtest.EnumTypes$
test
1352
1385
39
40
calculateEarthWeightOnPlanets
Apply
false
Expand All @@ -299,7 +299,7 @@ covtest.EnumTypes$
test
901
909
27
28
test
DefDef
false
Expand Down
4 changes: 2 additions & 2 deletions tests/coverage/pos/Escaping.scoverage.check
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ covtest.\n.\r\n\f
\r\n\f
69
80
3
4
length
Apply
false
Expand All @@ -44,7 +44,7 @@ covtest.\n.\r\n\f
\r\n\f
40
48
3
4
\r\n\f
DefDef
false
Expand Down
Loading

0 comments on commit f3a6ce3

Please sign in to comment.