Skip to content

Implement coverage reports for Starlark #11

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Implement coverage reports for Starlark #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
edc799e
Fix NPE in Parameter.Star#getLocation() for unnamed vararg
fmeum May 31, 2022
4d1fe9b
Fix NodePrinter crash on Comprehension.Clause
fmeum May 29, 2022
fc580d7
Remove redundant null check in NodeVisitor
fmeum May 30, 2022
90607e1
Add an LCOV coverage report generator for Starlark
fmeum May 29, 2022
b603de0
Add `--starlark_coverage_report`
fmeum May 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.OutputStream;
import java.io.PrintWriter;
import java.time.Duration;
import java.util.ArrayList;
import java.util.List;
Expand Down Expand Up @@ -371,6 +372,12 @@ private BlazeCommandResult execExclusively(
message, FailureDetails.Command.Code.STARLARK_CPU_PROFILING_INITIALIZATION_FAILURE);
}
}
if (!commonOptions.starlarkCoverageReport.isEmpty()) {
// Record coverage for all `.bzl` files, excluding the built-ins, which don't have paths that
// could be resolved when a human-readable coverage report is generated.
Starlark.startCoverageCollection(
path -> !path.startsWith("/virtual_builtins_bzl") && path.endsWith(".bzl"));
}

BlazeCommandResult result =
createDetailedCommandResult(
Expand Down Expand Up @@ -603,6 +610,18 @@ private BlazeCommandResult execExclusively(
}
}
}
if (!commonOptions.starlarkCoverageReport.isEmpty()) {
FileOutputStream out;
try {
out = new FileOutputStream(commonOptions.starlarkCoverageReport);
} catch (IOException ex) {
String message = "Starlark coverage recorder: " + ex.getMessage();
outErr.printErrLn(message);
return createDetailedCommandResult(
message, FailureDetails.Command.Code.STARLARK_COVERAGE_REPORT_DUMP_FAILURE);
}
Starlark.dumpCoverage(new PrintWriter(out));
}
Comment on lines +613 to +624
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Leak‑prone I/O: use try‑with‑resources and flush PrintWriter

FileOutputStream and the wrapping PrintWriter are opened but never closed.
On long‑running servers this leaks file descriptors, and on short‑lived invocations you may miss buffered bytes because PrintWriter is not flushed.

-        FileOutputStream out;
-        try {
-          out = new FileOutputStream(commonOptions.starlarkCoverageReport);
-        } catch (IOException ex) {
-          ...
-        }
-        Starlark.dumpCoverage(new PrintWriter(out));
+        try (FileOutputStream fos =
+                new FileOutputStream(commonOptions.starlarkCoverageReport);
+             PrintWriter pw = new PrintWriter(fos)) {
+          Starlark.dumpCoverage(pw);
+        } catch (IOException ex) {
+          ...
+        }

This guarantees flushing and closes both streams even if dumpCoverage throws.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!commonOptions.starlarkCoverageReport.isEmpty()) {
FileOutputStream out;
try {
out = new FileOutputStream(commonOptions.starlarkCoverageReport);
} catch (IOException ex) {
String message = "Starlark coverage recorder: " + ex.getMessage();
outErr.printErrLn(message);
return createDetailedCommandResult(
message, FailureDetails.Command.Code.STARLARK_COVERAGE_REPORT_DUMP_FAILURE);
}
Starlark.dumpCoverage(new PrintWriter(out));
}
if (!commonOptions.starlarkCoverageReport.isEmpty()) {
try (FileOutputStream fos =
new FileOutputStream(commonOptions.starlarkCoverageReport);
PrintWriter pw = new PrintWriter(fos)) {
Starlark.dumpCoverage(pw);
} catch (IOException ex) {
String message = "Starlark coverage recorder: " + ex.getMessage();
outErr.printErrLn(message);
return createDetailedCommandResult(
message, FailureDetails.Command.Code.STARLARK_COVERAGE_REPORT_DUMP_FAILURE);
}
}


needToCallAfterCommand = false;
return runtime.afterCommand(env, result);
Expand Down
9 changes: 9 additions & 0 deletions src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,15 @@ public String getTypeDescription() {
help = "Writes into the specified file a pprof profile of CPU usage by all Starlark threads.")
public String starlarkCpuProfile;

@Option(
name = "starlark_coverage_report",
defaultValue = "",
documentationCategory = OptionDocumentationCategory.LOGGING,
effectTags = {OptionEffectTag.BAZEL_MONITORING},
help = "Writes into the specified file an LCOV coverage report for all Starlark files "
+ "executed for the requested Bazel command.")
public String starlarkCoverageReport;

@Option(
name = "record_full_profiler_data",
defaultValue = "false",
Expand Down
40 changes: 36 additions & 4 deletions src/main/java/net/starlark/java/eval/Eval.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import net.starlark.java.syntax.CallExpression;
import net.starlark.java.syntax.Comprehension;
import net.starlark.java.syntax.ConditionalExpression;
import net.starlark.java.syntax.CoverageRecorder;
import net.starlark.java.syntax.DefStatement;
import net.starlark.java.syntax.DictExpression;
import net.starlark.java.syntax.DotExpression;
Expand All @@ -45,6 +46,7 @@
import net.starlark.java.syntax.ListExpression;
import net.starlark.java.syntax.LoadStatement;
import net.starlark.java.syntax.Location;
import net.starlark.java.syntax.Parameter;
import net.starlark.java.syntax.Resolver;
import net.starlark.java.syntax.ReturnStatement;
import net.starlark.java.syntax.SliceExpression;
Expand Down Expand Up @@ -131,6 +133,7 @@ private static TokenKind execFor(StarlarkThread.Frame fr, ForStatement node)
continue;
case BREAK:
// Finish loop, execute next statement after loop.
CoverageRecorder.getInstance().recordVirtualJump(node);
return TokenKind.PASS;
case RETURN:
// Finish loop, return from function.
Expand All @@ -145,6 +148,7 @@ private static TokenKind execFor(StarlarkThread.Frame fr, ForStatement node)
} finally {
EvalUtils.removeIterator(seq);
}
CoverageRecorder.getInstance().recordVirtualJump(node);
return TokenKind.PASS;
}

Expand All @@ -159,7 +163,9 @@ private static StarlarkFunction newFunction(StarlarkThread.Frame fr, Resolver.Fu
int nparams =
rfn.getParameters().size() - (rfn.hasKwargs() ? 1 : 0) - (rfn.hasVarargs() ? 1 : 0);
for (int i = 0; i < nparams; i++) {
Expression expr = rfn.getParameters().get(i).getDefaultValue();
Parameter parameter = rfn.getParameters().get(i);
CoverageRecorder.getInstance().recordCoverage(parameter.getIdentifier());
Expression expr = parameter.getDefaultValue();
Comment on lines +166 to +168
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Null‑safety for anonymous * / ** parameters

Parameter.getIdentifier() may legally return null for the lone "*" that separates positional from keyword‑only parameters.
Calling CoverageRecorder.recordCoverage(null) will throw a NullPointerException, crashing evaluation of such functions.

-      CoverageRecorder.getInstance().recordCoverage(parameter.getIdentifier());
+      Identifier pid = parameter.getIdentifier();
+      if (pid != null) {
+        CoverageRecorder.getInstance().recordCoverage(pid);
+      }

Apply the same guard inside the later loop that visits varargs/kwargs.

Also applies to: 178-181

if (expr == null && defaults == null) {
continue; // skip prefix of required parameters
}
Expand All @@ -169,6 +175,10 @@ private static StarlarkFunction newFunction(StarlarkThread.Frame fr, Resolver.Fu
defaults[i - (nparams - defaults.length)] =
expr == null ? StarlarkFunction.MANDATORY : eval(fr, expr);
}
// Visit kwargs and varargs for coverage.
for (int i = nparams; i < rfn.getParameters().size(); i++) {
CoverageRecorder.getInstance().recordCoverage(rfn.getParameters().get(i).getIdentifier());
}
if (defaults == null) {
defaults = EMPTY;
}
Expand Down Expand Up @@ -206,6 +216,7 @@ private static TokenKind execIf(StarlarkThread.Frame fr, IfStatement node)
} else if (node.getElseBlock() != null) {
return execStatements(fr, node.getElseBlock(), /*indented=*/ true);
}
CoverageRecorder.getInstance().recordVirtualJump(node);
return TokenKind.PASS;
}

Expand Down Expand Up @@ -262,6 +273,7 @@ private static TokenKind exec(StarlarkThread.Frame fr, Statement st)
if (++fr.thread.steps >= fr.thread.stepLimit) {
throw new EvalException("Starlark computation cancelled: too many steps");
}
CoverageRecorder.getInstance().recordCoverage(st);

switch (st.kind()) {
case ASSIGNMENT:
Expand Down Expand Up @@ -330,6 +342,7 @@ private static void assign(StarlarkThread.Frame fr, Expression lhs, Object value

private static void assignIdentifier(StarlarkThread.Frame fr, Identifier id, Object value)
throws EvalException {
CoverageRecorder.getInstance().recordCoverage(id);
Resolver.Binding bind = id.getBinding();
switch (bind.getScope()) {
case LOCAL:
Expand Down Expand Up @@ -477,6 +490,7 @@ private static Object eval(StarlarkThread.Frame fr, Expression expr)
if (++fr.thread.steps >= fr.thread.stepLimit) {
throw new EvalException("Starlark computation cancelled: too many steps");
}
CoverageRecorder.getInstance().recordCoverage(expr);

// The switch cases have been split into separate functions
// to reduce the stack usage during recursion, which is
Expand Down Expand Up @@ -533,9 +547,17 @@ private static Object evalBinaryOperator(StarlarkThread.Frame fr, BinaryOperator
// AND and OR require short-circuit evaluation.
switch (binop.getOperator()) {
case AND:
return Starlark.truth(x) ? eval(fr, binop.getY()) : x;
if (Starlark.truth(x)) {
return eval(fr, binop.getY());
}
CoverageRecorder.getInstance().recordVirtualJump(binop);
return x;
case OR:
return Starlark.truth(x) ? x : eval(fr, binop.getY());
if (Starlark.truth(x)) {
CoverageRecorder.getInstance().recordVirtualJump(binop);
return x;
}
return eval(fr, binop.getY());
default:
Object y = eval(fr, binop.getY());
try {
Expand Down Expand Up @@ -577,7 +599,9 @@ private static Object evalDict(StarlarkThread.Frame fr, DictExpression dictexpr)
private static Object evalDot(StarlarkThread.Frame fr, DotExpression dot)
throws EvalException, InterruptedException {
Object object = eval(fr, dot.getObject());
String name = dot.getField().getName();
Identifier field = dot.getField();
CoverageRecorder.getInstance().recordCoverage(field);
String name = field.getName();
try {
return Starlark.getattr(
fr.thread.mutability(), fr.thread.getSemantics(), object, name, /*defaultValue=*/ null);
Expand Down Expand Up @@ -627,6 +651,7 @@ private static Object evalCall(StarlarkThread.Frame fr, CallExpression call)
Object[] positional = npos == 0 ? EMPTY : new Object[npos];
for (i = 0; i < npos; i++) {
Argument arg = arguments.get(i);
CoverageRecorder.getInstance().recordCoverage(arg);
Object value = eval(fr, arg.getValue());
positional[i] = value;
}
Expand All @@ -635,13 +660,15 @@ private static Object evalCall(StarlarkThread.Frame fr, CallExpression call)
Object[] named = n == npos ? EMPTY : new Object[2 * (n - npos)];
for (int j = 0; i < n; i++) {
Argument.Keyword arg = (Argument.Keyword) arguments.get(i);
CoverageRecorder.getInstance().recordCoverage(arg);
Object value = eval(fr, arg.getValue());
named[j++] = arg.getName();
named[j++] = value;
}

// f(*args) -- varargs
if (star != null) {
CoverageRecorder.getInstance().recordCoverage(star);
Object value = eval(fr, star.getValue());
if (!(value instanceof StarlarkIterable)) {
fr.setErrorLocation(star.getStartLocation());
Expand All @@ -656,6 +683,7 @@ private static Object evalCall(StarlarkThread.Frame fr, CallExpression call)

// f(**kwargs)
if (starstar != null) {
CoverageRecorder.getInstance().recordCoverage(starstar);
Object value = eval(fr, starstar.getValue());
if (!(value instanceof Dict)) {
fr.setErrorLocation(starstar.getStartLocation());
Expand Down Expand Up @@ -791,6 +819,7 @@ void execClauses(int index) throws EvalException, InterruptedException {
assign(fr, forClause.getVars(), elem);
execClauses(index + 1);
}
CoverageRecorder.getInstance().recordVirtualJump(clause);
} catch (EvalException ex) {
fr.setErrorLocation(forClause.getStartLocation());
throw ex;
Expand All @@ -802,12 +831,15 @@ void execClauses(int index) throws EvalException, InterruptedException {
Comprehension.If ifClause = (Comprehension.If) clause;
if (Starlark.truth(eval(fr, ifClause.getCondition()))) {
execClauses(index + 1);
} else {
CoverageRecorder.getInstance().recordVirtualJump(clause);
}
}
return;
}

// base case: evaluate body and add to result.
CoverageRecorder.getInstance().recordCoverage(comp.getBody());
if (dict != null) {
DictExpression.Entry body = (DictExpression.Entry) comp.getBody();
Object k = eval(fr, body.getKey());
Expand Down
12 changes: 12 additions & 0 deletions src/main/java/net/starlark/java/eval/Starlark.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,19 +23,23 @@
import com.google.errorprone.annotations.FormatMethod;
import java.io.IOException;
import java.io.OutputStream;
import java.io.PrintWriter;
import java.lang.reflect.Method;
import java.math.BigInteger;
import java.time.Duration;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.TreeSet;
import java.util.function.Function;
import java.util.regex.Pattern;
import javax.annotation.Nullable;
import javax.annotation.concurrent.Immutable;
import net.starlark.java.annot.StarlarkAnnotations;
import net.starlark.java.annot.StarlarkBuiltin;
import net.starlark.java.annot.StarlarkMethod;
import net.starlark.java.spelling.SpellChecker;
import net.starlark.java.syntax.CoverageRecorder;
import net.starlark.java.syntax.Expression;
import net.starlark.java.syntax.FileOptions;
import net.starlark.java.syntax.ParserInput;
Expand Down Expand Up @@ -970,4 +974,12 @@ public static boolean startCpuProfile(OutputStream out, Duration period) {
public static void stopCpuProfile() throws IOException {
CpuProfiler.stop();
}

public static void startCoverageCollection(Function<String, Boolean> filenameMatcher) {
CoverageRecorder.startCoverageCollection(filenameMatcher);
}
Comment on lines +978 to +980
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add documentation for startCoverageCollection method

This public API method lacks documentation explaining its purpose, when to use it, and how it integrates with the broader coverage system. Consider adding a Javadoc comment to clarify:

  • The purpose of the method
  • What the filenameMatcher parameter does exactly (what format it expects, examples)
  • When this should be called in relation to Starlark execution
  • Thread-safety guarantees
  • Any performance implications
+/**
+ * Starts collecting coverage data for Starlark code execution.
+ * 
+ * <p>Coverage will be collected for all Starlark files that match the provided filename matcher.
+ * This method should be called before any Starlark code execution for which coverage data is
+ * desired.
+ *
+ * <p>This method is thread-safe and can be called concurrently with Starlark execution.
+ *
+ * @param filenameMatcher a function that returns true for filenames that should have coverage
+ *     collected, and false for files to exclude from coverage
+ */
 public static void startCoverageCollection(Function<String, Boolean> filenameMatcher) {
   CoverageRecorder.startCoverageCollection(filenameMatcher);
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public static void startCoverageCollection(Function<String, Boolean> filenameMatcher) {
CoverageRecorder.startCoverageCollection(filenameMatcher);
}
/**
* Starts collecting coverage data for Starlark code execution.
*
* <p>Coverage will be collected for all Starlark files that match the provided filename matcher.
* This method should be called before any Starlark code execution for which coverage data is
* desired.
*
* <p>This method is thread-safe and can be called concurrently with Starlark execution.
*
* @param filenameMatcher a function that returns true for filenames that should have coverage
* collected, and false for files to exclude from coverage
*/
public static void startCoverageCollection(Function<String, Boolean> filenameMatcher) {
CoverageRecorder.startCoverageCollection(filenameMatcher);
}


public static void dumpCoverage(PrintWriter out) {
CoverageRecorder.getInstance().dump(out);
}
Comment on lines +982 to +984
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add documentation and error handling for dumpCoverage method

This public API method also lacks documentation and doesn't provide any error handling mechanism. If an error occurs while writing the coverage data, it would silently fail.

Consider enhancing this method with proper documentation and error handling:

+/**
+ * Writes the collected Starlark coverage data to the specified writer in LCOV format.
+ * 
+ * <p>This method should be called after Starlark code execution has completed to generate
+ * the coverage report. It will dump coverage for all files that matched the filter provided
+ * to {@link #startCoverageCollection}.
+ *
+ * <p>The coverage report includes function, line, and branch coverage information.
+ *
+ * @param out the writer to which the LCOV report will be written
+ * @throws RuntimeException if an error occurs while writing the coverage data
+ */
 public static void dumpCoverage(PrintWriter out) {
-  CoverageRecorder.getInstance().dump(out);
+  try {
+    CoverageRecorder.getInstance().dump(out);
+  } catch (Exception e) {
+    throw new RuntimeException("Failed to dump Starlark coverage data", e);
+  }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public static void dumpCoverage(PrintWriter out) {
CoverageRecorder.getInstance().dump(out);
}
/**
* Writes the collected Starlark coverage data to the specified writer in LCOV format.
*
* <p>This method should be called after Starlark code execution has completed to generate
* the coverage report. It will dump coverage for all files that matched the filter provided
* to {@link #startCoverageCollection}.
*
* <p>The coverage report includes function, line, and branch coverage information.
*
* @param out the writer to which the LCOV report will be written
* @throws RuntimeException if an error occurs while writing the coverage data
*/
public static void dumpCoverage(PrintWriter out) {
try {
CoverageRecorder.getInstance().dump(out);
} catch (Exception e) {
throw new RuntimeException("Failed to dump Starlark coverage data", e);
}
}

}
2 changes: 2 additions & 0 deletions src/main/java/net/starlark/java/syntax/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ java_library(
"Comment.java",
"Comprehension.java",
"ConditionalExpression.java",
"CoverageRecorder.java",
"CoverageVisitor.java",
"DefStatement.java",
"DictExpression.java",
"DotExpression.java",
Expand Down
Loading