forked from bazelbuild/bazel
-
Notifications
You must be signed in to change notification settings - Fork 0
{{ message }}
Implement coverage reports for Starlark #11
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A `ConditionalExpression`, differing from an `IfStatement`, always has an `else` case.
The generated coverage report includes function, line, and branch coverage information in LCOV format.
If the new flag `--starlark_coverage_report` is set to a non-empty value, the Starlark coverage recorder will be enabled for all `.bzl` files under the output base executed during the Blaze command. At the end of the command, the collected coverage data will be emitted to the file path given as the flag argument in LCOV format.
Author
|
CC @Wyverald I generated reports for some rulesets and found the results to be relatively accurate, but this would definitely benefit from more testing (it doesn't come with test cases yet) and polish. I'm also not sure whether it works for repository rules and/or functions executed during the execution phase, e.g. map_each callbacks. I wanted to open a PR to get early feedback and will happily fix bugs and supply tests if this is generally viewed favorably. |
Author
|
Thanks for the PR, Fabian! I'll leave it to Jon & Sasha to review this. (NB: Jon is OOO until 9 June.) Also cc @adonovan |
Author
|
I fixed some bugs and refactored the branch logic, but will not make further changes. |
Author
Sorry, I certainly didn't intend to skip proper process here. I wrote this code out of a concrete need to better understand the edge cases of rulesets I maintain and to improve my understanding of the Java Starlark implementation. I just thought about sharing it hoping that it could be useful for others. I am completely open to rewriting this from scratch or scrapping it entirely, depending on what the outcome of the design discussion is.
The way the hooks are currently implemented, I won't be able to dismiss this concern with confidence. As you said, benchmarking doesn't necessarily catch all regressions on such a low level. I do think that there is a way to implement the coverage hooks without any overhead in the case where coverage isn't being collected:
The Do you like this approach better?
After a first read of the code, I'm inclined to say this would work for statement coverage, but not for more fine-grained notions of coverage.
I agree that statement coverage would already be quite useful. However, given the general conciseness of Starlark (especially with list/dict comprehensions and conditional expressions), a single statement can encode rather complex logic. With the rulesets I have tried this on, statement coverage has almost always been nearing 100%, but branch coverage revealed some interesting uncovered cases (e.g., handling of workspace vs. external repository, checks for missing/superfluous files). While branch coverage information for Assuming we find a way to ensure that coverage support introduces no overhead at all (as possibly described above), would you still prefer to remove some or all of the branch coverage hooks?. |
Author
|
@adonovan Would you like me to prototype the |
Author
|
Finally taking a look at this, as I promised Fabian back at BazelCon. It looks like there's quite a bit more code to this PR than I originally thought. I'm interested in supporting more tooling for Starlark in the new year, coverage included, but in light of
it's clear that a quick acceptance of this PR is off the table. I'd love to have a more involved discussion centered around the benefit vs maintainability tradeoff in Q1. I'll keep the PR open for now. |
Author
|
@brandjon @tetromino Friendly ping on this. I could trim this down to just line coverage and ensure that there is no runtime overhead if coverage is disabled. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis change introduces comprehensive Starlark code coverage instrumentation and reporting support. It adds a new command-line option to request LCOV-format coverage reports for Starlark files, and integrates coverage collection into the Blaze command execution flow. The Starlark evaluation engine is instrumented to record coverage at the AST node level, including branches and function entries. New infrastructure for coverage recording, AST traversal, and LCOV report generation is implemented. The system is thread-safe and supports filtering by filename. An explicit failure code is added for coverage report dumping errors, and minor related refactoring and bug fixes are included. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BlazeCommandDispatcher
participant Starlark
participant CoverageRecorder
participant FileSystem
User->>BlazeCommandDispatcher: Run command with --starlark_coverage_report=path
BlazeCommandDispatcher->>Starlark: startCoverageCollection(filenameMatcher)
Starlark->>CoverageRecorder: startCoverageCollection(filenameMatcher)
BlazeCommandDispatcher->>...: Execute command (Starlark files evaluated)
...->>CoverageRecorder: recordCoverage(node) (during evaluation)
BlazeCommandDispatcher->>FileSystem: Open coverage report file for writing
alt File open fails
BlazeCommandDispatcher->>User: Print error, exit with STARLARK_COVERAGE_REPORT_DUMP_FAILURE
else File open succeeds
BlazeCommandDispatcher->>Starlark: dumpCoverage(PrintWriter)
Starlark->>CoverageRecorder: dump(PrintWriter)
CoverageRecorder->>FileSystem: Write LCOV report
BlazeCommandDispatcher->>User: Command completes
end
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (9)
src/main/java/net/starlark/java/syntax/Program.java (2)
88-90: Consider also registering programs compiled from expressionsThe current implementation only registers programs created via
compileFilewith the coverage recorder, but not those created viacompileExpr. This could lead to incomplete coverage data when expressions are executed.Consider whether expression coverage should also be tracked by adding similar registration code to the
compileExprmethod.
88-90: Add error handling for the registration callThe
registercall to theCoverageRecorderhas no error handling. If registration fails for any reason, it could potentially cause compilation to fail or leave the coverage recorder in an inconsistent state.Consider adding a try-catch block to handle potential exceptions from the registration process.
src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java (1)
296-303: The command-line option is well-definedThe option follows the established pattern in this file and provides clear documentation of its purpose. It's properly categorized under logging with the appropriate effect tag.
One suggestion would be to enhance the help text with more details about the LCOV format, the types of coverage collected (functions, lines, branches), and any performance implications of enabling coverage.
src/main/java/net/starlark/java/eval/Starlark.java (1)
978-984: Consider adding a method to check if coverage is enabledTo allow conditional behavior based on whether coverage is enabled, consider adding a method to check the coverage state. This would help optimize code paths that only need to execute when coverage is being collected.
+/** + * Returns whether Starlark coverage collection is currently enabled. + * + * @return true if coverage collection is active, false otherwise + */ +public static boolean isCoverageEnabled() { + return CoverageRecorder.getInstance().isActive(); +}src/main/java/net/starl 8000 ark/java/eval/Eval.java (1)
493-494: Hot‑path overhead: guard coverage when disabled
evalis invoked for every expression; unconditionally calling
CoverageRecorder.getInstance().recordCoverage(expr)adds at least one
virtual method dispatch per node even when coverage is turned off.
If the recorder implements a cheap singleton noop this may be fine, but you can eliminate the call entirely:if (CoverageRecorder.isActive()) { CoverageRecorder.getInstance().recordCoverage(expr); }Worth considering if micro‑benchmarks show a noticeable slowdown.
src/main/java/net/starlark/java/syntax/CoverageVisitor.java (1)
85-92: String concatenation cost in deeply‑nested functions
enterFunctionrebuilds the fulla > b > cname by streaming and joining the
entire stack on every function entry.
In files with many nested lambdas this is O(depth²) and allocates a new
String each time.An incremental
StringBuilder(push/pop) or caching the previously built
prefix would avoid repeated traversals and allocations.Not critical, but easy to optimise later.
src/main/java/net/starlark/java/syntax/CoverageRecorder.java (3)
158-167:dumpshould not close thePrintWriterit did not openClosing the writer here prevents callers from writing additional data (e.g. merging multiple recorders) or from handling the resource lifecycle themselves (try‑with‑resources, error handling, etc.).
Leave responsibility to the caller:- registeredPrograms.stream() - .sorted(Comparator.comparing(Program::getFilename)) - .forEachOrdered(program -> { - CoverageNodeVisitor visitor = new CoverageNodeVisitor(program); - visitor.visitAll(program.getResolvedFunction().getBody()); - visitor.dump(out); - }); - out.close(); + registeredPrograms.stream() + .sorted(Comparator.comparing(Program::getFilename)) + .forEachOrdered(program -> { + CoverageNodeVisitor visitor = new CoverageNodeVisitor(program); + visitor.visitAll(program.getResolvedFunction().getBody()); + visitor.dump(out); + }); }
115-136: Avoid creating a newCoverageVisitorfor every top‑level statement
registerinstantiates a fresh anonymousCoverageVisitorfor each top‑level statement, causing:
- Redundant allocations (can be thousands in large files).
- Missed opportunities to share state across statements (e.g., sets/maps).
Refactor to create one visitor per program and call
visitor.visitAll(program.getResolvedFunction().getBody())once, mirroring the approach used later indump.
93-103: Potential memory leak:Nodekeys retain full AST
counts/virtualJumpCountsstore strong references from long‑lived maps to everyNode, which in turn references the entire subtree and the containingProgram.
If the JVM server process lives for many Bazel commands, this can keep all compiled ASTs alive even after dumping coverage.Consider:
- Using a
WeakHashMap<Node, LongAdder>(synchronised wrapper) so entries are cleared once the AST is unreachable, or- Storing a lightweight key (e.g.,
(filename,line,column)).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java(3 hunks)src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java(1 hunks)src/main/java/net/starlark/java/eval/Eval.java(17 hunks)src/main/java/net/starlark/java/eval/Starlark.java(2 hunks)src/main/java/net/starlark/java/syntax/BUILD(1 hunks)src/main/java/net/starlark/java/syntax/CoverageRecorder.java(1 hunks)src/main/java/net/starlark/java/syntax/CoverageVisitor.java(1 hunks)src/main/java/net/starlark/java/syntax/NodePrinter.java(3 hunks)src/main/java/net/starlark/java/syntax/NodeVisitor.java(1 hunks)src/main/java/net/starlark/java/syntax/Parameter.java(1 hunks)src/main/java/net/starlark/java/syntax/Program.java(1 hunks)src/main/protobuf/failure_details.proto(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java (1)
src/main/java/net/starlark/java/eval/Starlark.java (1)
Starlark(55-985)
src/main/java/net/starlark/java/syntax/CoverageRecorder.java (2)
src/main/java/net/starlark/java/syntax/Resolver.java (1)
Function(119-277)src/main/java/net/starlark/java/syntax/Program.java (1)
Program(24-105)
🔇 Additional comments (9)
src/main/java/net/starlark/java/syntax/NodeVisitor.java (1)
177-177:Details
✅ Verification successful
Verify the safety of removing the null check
The null check before visiting the else-case node has been removed, making this visit unconditional. Other similar methods in this file (like visitBlock, visit for defaultValue, Result, etc.) still retain their null checks. This change could potentially cause NullPointerExceptions if
getElseCase()returns null andvisit()doesn't handle null inputs.This change is likely related to the new coverage tracking implementation. Please verify that either:
ConditionalExpression.getElseCase()is guaranteed to never return null, or- The
CoverageVisitorimplementation handles null nodes appropriately.
🏁 Script executed:
#!/bin/bash # Check if ConditionalExpression.getElseCase() can return null grep -A 10 "public .* getElseCase" src/main/java/net/starlark/java/syntax/ConditionalExpression.java # Look for null checks in the CoverageVisitor class grep -A 3 "visit.*ConditionalExpression" src/main/java/net/starlark/java/syntax/CoverageVisitor.javaLength of output: 660
🏁 Script executed:
#!/bin/bash echo "=== visitBranch definition in CoverageVisitor.java ===" grep -R "visitBranch" -n src/main/java/net/starlark/java/syntax/CoverageVisitor.java -A20 echo echo "=== Beginning of NodeVisitor.java (to inspect visit(Node)) ===" sed -n '1,200p' src/main/java/net/starlark/java/syntax/NodeVisitor.javaLength of output: 9410
Null check removal is safe
ConditionalExpression.getElseCase()is set via the constructor and, per the language grammar, always carries a non‑null expression; it can never benull. The unguarded call inNodeVisitor.visit(ConditionalExpression)therefore cannot NPE, andCoverageVisitor.visitBranchalready allows a nullable successor. No further changes are required.src/main/java/net/starlark/java/syntax/BUILD (1)
24-25: LGTM! New coverage implementation files addedThe addition of
CoverageRecorder.javaandCoverageVisitor.javato the build file supports the new Starlark coverage reporting feature. These files will implement the necessary infrastructure for AST traversal with coverage tracking and LCOV-compatible output generation.src/main/protobuf/failure_details.proto (1)
536-536: LGTM! Appropriate error code for coverage report failuresAdding a specific failure code for Starlark coverage report dumping failures is appropriate. The exit code 36 is consistent with other file-related errors in the system like
SERVER_FILE_WRITE_FAILUREand other I/O operation failures.src/main/java/net/starlark/java/syntax/Parameter.java (1)
119-121: LGTM! Defensive null check prevents potential NPEThis change adds proper null handling for Star parameters with null identifiers. Since the constructor accepts a nullable identifier, this defensive programming approach prevents NullPointerExceptions when calculating the end offset of a Star parameter without an identifier.
src/main/java/net/starlark/java/syntax/NodePrinter.java (3)
44-46: Good improvement for handling Comprehension.Clause nodesAdding explicit handling for Comprehension.Clause nodes in the printNode method improves the completeness of this method and ensures all node types can be properly processed.
283-283: Cleaner code through refactoringChanging the inline conditional logic to call the new printClause method improves readability and maintainability.
479-491: Well-structured helper method for printing clausesThe newly added printClause method properly handles both For and If clauses with the correct syntax. The refactoring centralizes this logic in a single place, making it easier to maintain and extend if needed.
src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java (1)
375-381: Ensure coverage collection is reset between commands
Starlark.startCoverageCollectionis invoked every time a command is executed when the flag is set, but there is no corresponding call that resets or guards against a previous invocation still being active.
If two consecutive commands run in the same server process, coverage from the first may pollute the second (or the second call might be a no‑op, depending onstartCoverageCollection’s implementation).Please verify that:
startCoverageCollectionidempotently resets any prior state, or- Coverage is explicitly cleared after
dumpCoverage, or- We guard with a boolean to only start once per server lifetime.
If none of the above holds, users will get confusing, merged LCOV reports.
src/main/java/net/starlark/java/syntax/CoverageRecorder.java (1)
190-199:Details
❌ Incorrect review comment
Same NPE risk when populating
BranchInfo
counts.get(positiveUniqueSuccessor)(andnegativeUniqueSuccessor) can returnnullif the successor hasn’t reachedvisitCodeyet. Guard the lookup just like the previous comment or defer branch materialisation until after the whole subtree has been visited.
I’ll locate how
countsis declared and how it’s populated to confirm whetherget(...)can returnnull.
🏁 Script executed:
#!/bin/bash grep -n "private.*counts" -n src/main/java/net/starlark/java/syntax/CoverageRecorder.java grep -n "counts *= *new" -n src/main/java/net/starlark/java/syntax/CoverageRecorder.java grep -n "counts\\.put" -n src/main/java/net/starlark/java/syntax/CoverageRecorder.java rg -n "counts\\.get" src/main/java/net/starlark/java/syntax/CoverageRecorder.javaLength of output: 948
🏁 Script executed:
#!/bin/bash # Show context around counts.put to see how and when entries are added rg -n -C5 'counts\.put' src/main/java/net/starlark/java/syntax/CoverageRecorder.javaLength of output: 362
🏁 Script executed:
#!/bin/bash sed -n '160,240p' src/main/java/net/starlark/java/syntax/CoverageRecorder.javaLength of output: 3373
I’ll inspect
CoverageVisitorto understand the traversal order ofvisitBranchvs.visitCode.
🏁 Script executed:
#!/bin/bash # Locate the CoverageVisitor class and its methods rg -n "class CoverageVisitor" src/main/java/net/starlark/java/syntax/CoverageVisitor.java rg -n -C5 "visitBranch" src/main/java/net/starlark/java/syntax/CoverageVisitor.java rg -n -C5 "visitCode" src/main/java/net/starlark/java/syntax/CoverageVisitor.javaLength of output: 8760
🏁 Script executed:
#!/bin/bash sed -n '80,160p' src/main/java/net/starlark/java/syntax/CoverageRecorder.javaLength of output: 2725
No NPE risk in
visitBranch– all counts are pre‑populated inregisterThe
countsmap is fully populated for every executableNode(including branch conditions and successors) by the anonymousCoverageVisitorinregister(…)(itsvisitCoderuns on every node in the AST). Likewise,virtualJumpCountsis initialized for any branch with a missingnegativeUniqueSuccessor. By the timedump()runs and theCoverageNodeVisitorcallscounts.get(…)orvirtualJumpCounts.get(…), neither lookup can returnnull. You can safely remove the suggested null guards.Likely an incorrect or invalid review comment.
Comment on lines
+978
to
+980
| public static void startCoverageCollection(Function<String, Boolean> filenameMatcher) { | ||
| CoverageRecorder.startCoverageCollection(filenameMatcher); | ||
| } | 8000
There was a problem hiding this comment.
🛠️ 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); | |
| } |
Comment on lines
+982
to
+984
| public static void dumpCoverage(PrintWriter out) { | ||
| CoverageRecorder.getInstance().dump(out); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation and error handling for dumpCoverage method
This public API metho 67E6 d 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); | |
| } | |
| } |
Comment on lines
+613
to
+624
| 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)); | ||
| } |
There was a problem hiding this comment.
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); | |
| } | |
| } |
Comment on lines
+166
to
+168
| Parameter parameter = rfn.getParameters().get(i); | ||
| CoverageRecorder.getInstance().recordCoverage(parameter.getIdentifier()); | ||
| Expression expr = parameter.getDefaultValue(); |
There was a problem hiding this comment.
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
Comment on lines
+181
to
+185
| protected void visitFunction(String identifier, Node defStatement, Node firstBodyStatement) { | ||
| functions.add(new FunctionInfo(identifier, defStatement.getStartLocation().line(), | ||
| counts.get(firstBodyStatement).sum())); | ||
| } | ||
|
|
There was a problem hiding this comment.
counts.get(firstBodyStatement) can be null → potential NullPointerException
visitFunction is executed before visitCode has visited the first body statement (pre‑order traversal), so the LongAdder may not yet be in counts. Dereferencing sum() without a null‑check will crash during initial registration for every function.
- functions.add(new FunctionInfo(identifier, defStatement.getStartLocation().line(),
- counts.get(firstBodyStatement).sum()));
+ LongAdder adder = counts.get(firstBodyStatement);
+ long count = adder == null ? 0 : adder.sum();
+ functions.add(new FunctionInfo(identifier, defStatement.getStartLocation().line(), count));📝 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
| protected void visitFunction(String identifier, Node defStatement, Node firstBodyStatement) { | |
| functions.add(new FunctionInfo(identifier, defStatement.getStartLocation().line(), | |
| counts.get(firstBodyStatement).sum())); | |
| } | |
| protected void visitFunction(String identifier, Node defStatement, Node firstBodyStatement) { | |
| LongAdder adder = counts.get(firstBodyStatement); | |
| long count = adder == null ? 0 : adder.sum(); | |
| functions.add(new FunctionInfo(identifier, defStatement.getStartLocation().line(), count)); | |
| } |
Comment on lines
+41
to
+56
| static CoverageRecorder getInstance() { | ||
| return CoverageRecorderHolder.INSTANCE; | ||
| } | ||
|
|
||
| /** | ||
| * Collect coverage for all {@link Program}s compiled after the call whose | ||
| * {@link Program#getFilename()} matches {@code filenameMatcher}. | ||
| */ | ||
| sta A3E2 tic void startCoverageCollection(Function<String, Boolean> filenameMatcher) { | ||
| CoverageRecorderHolder.INSTANCE = new LcovCoverageRecorder(filenameMatcher); | ||
| } | ||
|
|
||
| class CoverageRecorderHolder { | ||
|
|
||
| private static CoverageRecorder INSTANCE = new NoopCoverageRecorder(); | ||
|
|
There was a problem hiding this comment.
Make INSTANCE volatile (or use an AtomicReference) to avoid a data‑race
startCoverageCollection can be invoked from a different thread than recordCoverage/getInstance.
Because CoverageRecorderHolder.INSTANCE is a mutable static field without the volatile modifier, the write performed in line 50 may not become visible to threads already calling getInstance(), leading them to observe a partially‑constructed LcovCoverageRecorder or to keep using the NoopCoverageRecorder.
- class CoverageRecorderHolder {
- private static CoverageRecorder INSTANCE = new NoopCoverageRecorder();
+ class CoverageRecorderHolder {
+ // Volatile guarantees safe publication of the recorder instance
+ private static volatile CoverageRecorder INSTANCE = new NoopCoverageRecorder();Alternatively, replace the field with an AtomicReference<CoverageRecorder> and perform a CAS to protect against multiple concurrent initialisations.
📝 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
| static CoverageRecorder getInstance() { | |
| return CoverageRecorderHolder.INSTANCE; | |
| } | |
| /** | |
| * Collect coverage for all {@link Program}s compiled after the call whose | |
| * {@link Program#getFilename()} matches {@code filenameMatcher}. | |
| */ | |
| static void startCoverageCollection(Function<String, Boolean> filenameMatcher) { | |
| CoverageRecorderHolder.INSTANCE = new LcovCoverageRecorder(filenameMatcher); | |
| } | |
| class CoverageRecorderHolder { | |
| private static CoverageRecorder INSTANCE = new NoopCoverageRecorder(); | |
| static CoverageRecorder getInstance() { | |
| return CoverageRecorderHolder.INSTANCE; | |
| } | |
| /** | |
| * Collect coverage for all {@link Program}s compiled after the call whose | |
| * {@link Program#getFilename()} matches {@code filenameMatcher}. | |
| */ | |
| static void startCoverageCollection(Function<String, Boolean> filenameMatcher) { | |
| CoverageRecorderHolder.INSTANCE = new LcovCoverageRecorder(filenameMatcher); | |
| } | |
| class CoverageRecorderHolder { | |
| // Volatile guarantees safe publication of the recorder instance | |
| private static volatile CoverageRecorder INSTANCE = new NoopCoverageRecorder(); |
Collaborator
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
| } | ||
| } | ||
| } | ||
| if (!commonOptions.starlarkCoverageReport.isEmpty()) { |
There was a problem hiding this comment.
Security Issue: Potential path traversal vulnerability in coverage report file path handling. The code directly uses the user-provided file path without validation, which could allow directory traversal attacks (e.g., '../sensitive_file'). Consider validating the path by comparing canonical and absolute paths:
String reportPath = commonOptions.starlarkCoverageReport;
File reportFile = new File(reportPath);
if (reportFile.getCanonicalPath().equals(reportFile.getAbsolutePath())) {
out = new FileOutputStream(reportFile);
} else {
// Handle invalid path
}| String message = "Starlark coverage recorder: " + ex.getMessage(); | ||
| outErr.printErrLn(message); | ||
| return createDetailedCommandResult( | ||
| message, FailureDetails.Command.Code.STARLARK_COVERAGE_REPORT_DUMP_FAILURE); |
There was a problem hiding this comment.
Resource Management Issue: The FileOutputStream isn't guaranteed to be closed if an exception occurs during Starlark.dumpCoverage(). This could lead to resource leaks in this long-running application. Consider using try-with-resources to ensure proper resource cleanup:
try (FileOutputStream out = new FileOutputStream(commonOptions.starlarkCoverageReport);
PrintWriter writer = new PrintWriter(out)) {
Starlark.dumpCoverage(writer);
} catch (IOException ex) {
// Handle exception
}
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If the new flag
--starlark_coverage_reportis set to a non-empty value, the Starlark coverage recorder will be enabled for all.bzlfiles under the output base executed during the Blaze command. At the end of the command, the collected coverage data will be emitted to the file path given as the flag argument in LCOV format, containing function, line, and branch coverage information.Summary by CodeRabbit