feat(#4983): enable WPA caching in MjLint#4986
Conversation
📝 WalkthroughWalkthroughThe PR introduces injectable file-filtering predicates to the Cache class and refactors MjLint's linting workflow to cache defect results in XMIR format. Cache hashing logic converts from static to instance methods, enabling directory walks to apply custom filters. MjLint now collects and persists linting defects via XMIR caching, recovering defect data across runs through a new XMIR parsing method. Changes
Sequence Diagram(s)sequenceDiagram
participant MjLint
participant Cache
participant XMIR
participant Defect as Defect List
MjLint->>MjLint: lintAll() invoked
alt Cache enabled
MjLint->>Cache: Apply with filter predicate
Cache->>Cache: sha(dir) with filtered files
Cache->>XMIR: Write XMIR with defects
MjLint->>XMIR: read(Path)
XMIR->>Defect: Parse /defects/error
Defect->>MjLint: Return List<Defect>
else Cache disabled
MjLint->>MjLint: wpa(pkg)
MjLint->>Defect: Collect List<Defect>
Defect->>MjLint: Return results
end
MjLint->>MjLint: Update counts from Defect
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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. Comment |
🚀 Performance AnalysisAll benchmarks are within the acceptable range. No critical degradation detected (threshold is 100%). Please refer to the detailed report for more information. Click to see the detailed report
|
There was a problem hiding this comment.
Pull request overview
This PR aims to enable and validate Whole Program Analysis (WPA) caching for MjLint by turning on the previously disabled cache-related test and adding cache support in the WPA linting path.
Changes:
- Removed the TODO puzzle and enabled the WPA cache test in
MjLintTest. - Added WPA caching logic to
MjLint.lintAll()and introduced parsing of cached WPA defects. - Extended
Cacheto support hashing directories with a configurable file filter.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
eo-maven-plugin/src/test/java/org/eolang/maven/MjLintTest.java |
Enables the WPA cache test and updates expectations for cached WPA output. |
eo-maven-plugin/src/main/java/org/eolang/maven/MjLint.java |
Implements WPA caching in lintAll() and adds cached defect parsing. |
eo-maven-plugin/src/main/java/org/eolang/maven/Cache.java |
Adds a Predicate<Path> filter to directory hashing to support selective cache invalidation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private static List<Defect> read(final Path path) { | ||
| return new Xnav(path).path("/defects/error").map( | ||
| node -> new Defect.Default( | ||
| node.attribute("check").text().orElseThrow(), | ||
| Severity.parsed(node.attribute("severity").text().orElseThrow()), | ||
| "", | ||
| 0, | ||
| "" | ||
| ) | ||
| ).collect(Collectors.toList()); |
There was a problem hiding this comment.
read() uses orElseThrow() for required attributes, which will throw a generic NoSuchElementException if the cache XMIR is corrupted/partial. Elsewhere in this class (see existing()), missing required attributes throw an exception with a clear message.
It would be more diagnosable to throw with an explicit message here too (e.g., "Cached WPA defect must contain 'check'/'severity' attribute").
| @@ -61,7 +82,7 @@ final class Cache { | |||
| */ | |||
| public void apply(final Path source, final Path target, final Path tail) { | |||
| try { | |||
| final String sha = Cache.sha(source); | |||
| final String sha = this.sha(source); | |||
| final Path hash = this.hash(tail); | |||
| final Path cache = this.base.resolve(tail); | |||
| if ( | |||
| @@ -99,10 +120,10 @@ private Path hash(final Path tail) { | |||
| * @param any File or directory path | |||
| * @return Base64-encoded SHA-256 hash of the file or directory contents | |||
| */ | |||
| private static String sha(final Path any) { | |||
| private String sha(final Path any) { | |||
| final String result; | |||
| if (Files.isDirectory(any)) { | |||
| result = Cache.dirSha(any); | |||
| result = this.dirSha(any); | |||
| } else if (Files.isRegularFile(any)) { | |||
| result = Cache.fileSha(any); | |||
| } else { | |||
| @@ -118,11 +139,12 @@ private static String sha(final Path any) { | |||
| * @param dir Directory path. | |||
| * @return Base64-encoded SHA-256 hash of the directory contents. | |||
| */ | |||
| private static String dirSha(final Path dir) { | |||
| private String dirSha(final Path dir) { | |||
| try { | |||
| final MessageDigest digest = MessageDigest.getInstance("SHA-256"); | |||
| try (Stream<Path> stream = Files.walk(dir)) { | |||
| stream.filter(Files::isRegularFile) | |||
| .filter(this.filter::test) | |||
| .sorted(Comparator.comparing(Path::toString)) | |||
There was a problem hiding this comment.
A new Cache feature was added (directory hashing filter via Predicate<Path>), but there’s no unit test coverage to confirm that excluded files don’t affect the computed hash and that included files do. Since this is now relied on by WPA caching, a focused test would help prevent regressions (e.g., create a directory with two files, exclude one via filter, modify it, and assert the cache does not recompile).
| final Path wpa = Path.of("wpa.xmir"); | ||
| final Path target = this.targetDir.toPath().resolve(MjLint.DIR).resolve(wpa); | ||
| new Cache( | ||
| this.cache.toPath().resolve(MjLint.CACHE), | ||
| root -> { | ||
| Logger.info(this, "Linting a package"); | ||
| final Directives all = new Directives().add("defects"); | ||
| for (final Defect defect : this.wpa(pkg)) { | ||
| MjLint.embedded(all, defect); | ||
| } | ||
| all.up(); | ||
| return new Xembler(all).xmlQuietly(); | ||
| }, | ||
| p -> p.getFileName().toString().endsWith(".xmir") | ||
| && !p.getFileName().equals(wpa) | ||
| ).apply(this.sourcesDir.toPath(), target, wpa); |
There was a problem hiding this comment.
In the WPA cached branch, the cache key is computed from this.sourcesDir but the directory hash is filtered to only include .xmir files (excluding wpa.xmir). sourcesDir defaults to src/main/eo, so the hash will likely be computed from an empty file set (or at least ignore the real WPA inputs), causing stale cache reuse across source changes and potential cross-project cache collisions.
Consider hashing the actual WPA inputs (e.g., the XMIR directory under targetDir, or the EO sources with a .eo filter), and/or using a CachePath that includes plugin version + a project/package-specific component so wpa.xmir doesn’t live at a global fixed location.
| final List<Defect> defects; | ||
| if (this.cacheEnabled) { | ||
| final Path wpa = Path.of("wpa.xmir"); | ||
| final Path target = this.targetDir.toPath().resolve(MjLint.DIR).resolve(wpa); | ||
| new Cache( | ||
| this.cache.toPath().resolve(MjLint.CACHE), | ||
| root -> { | ||
| Logger.info(this, "Linting a package"); | ||
| final Directives all = new Directives().add("defects"); | ||
| for (final Defect defect : this.wpa(pkg)) { | ||
| MjLint.embedded(all, defect); | ||
| } | ||
| all.up(); | ||
| return new Xembler(all).xmlQuietly(); | ||
| }, | ||
| p -> p.getFileName().toString().endsWith(".xmir") | ||
| && !p.getFileName().equals(wpa) | ||
| ).apply(this.sourcesDir.toPath(), target, wpa); | ||
| defects = MjLint.read(target); | ||
| } else { | ||
| Logger.info( | ||
| this, | ||
| "Linting a package without cache, this might be slow, consider enabling cache" | ||
| ); | ||
| defects = this.wpa(pkg); | ||
| } | ||
| for (final Defect defect : defects) { | ||
| counts.compute(defect.severity(), (sev, before) -> before + 1); | ||
| } |
There was a problem hiding this comment.
With cache enabled, WPA defects are only logged during a cache miss (inside the compilation lambda via this.wpa(pkg)). On a cache hit you read cached defects and update counts, but nothing is logged, which makes output depend on cache state (unlike lintOne, which logs every run even when cached).
If consistent logging is desired, log defects after loading them from cache as well. That likely requires caching/restoring the full defect details (object/line/text) instead of dropping them in read().
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@eo-maven-plugin/src/main/java/org/eolang/maven/MjLint.java`:
- Around line 445-460: The read method currently discards defect details (only
using check and severity); update it to extract and preserve the defect's
optional attributes and content from the Xnav node: read
node.attribute("line").text().map(Integer::parseInt).orElse(0) for the line
number, node.text().orElse("") for the defect message, and if present
node.attribute("file").text().orElse("") for the file/path, then pass these
values into the Defect.Default constructor (while keeping Severity.parsed(...)
for severity) so cached XMIR defects retain full fidelity; use safe parsing with
defaults to avoid exceptions in read/Xnav handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0da3e241-e505-4d30-9923-ceb07b400a56
📒 Files selected for processing (3)
eo-maven-plugin/src/main/java/org/eolang/maven/Cache.javaeo-maven-plugin/src/main/java/org/eolang/maven/MjLint.javaeo-maven-plugin/src/test/java/org/eolang/maven/MjLintTest.java
| /** | ||
| * Read defects from XMIR. | ||
| * @param path Path to XMIR | ||
| * @return Collection of defects | ||
| */ | ||
| private static List<Defect> read(final Path path) { | ||
| return new Xnav(path).path("/defects/error").map( | ||
| node -> new Defect.Default( | ||
| node.attribute("check").text().orElseThrow(), | ||
| Severity.parsed(node.attribute("severity").text().orElseThrow()), | ||
| "", | ||
| 0, | ||
| "" | ||
| ) | ||
| ).collect(Collectors.toList()); | ||
| } |
There was a problem hiding this comment.
Defect data is discarded when reading from cache.
The read method only extracts check and severity, but ignores line attribute and text content that are actually persisted by embedded(). While this works for the current counting-only usage, it loses defect fidelity.
Consider extracting available data to preserve full defect information:
Proposed fix to preserve defect data
private static List<Defect> read(final Path path) {
return new Xnav(path).path("/defects/error").map(
node -> new Defect.Default(
node.attribute("check").text().orElseThrow(),
Severity.parsed(node.attribute("severity").text().orElseThrow()),
"",
- 0,
- ""
+ Integer.parseInt(node.attribute("line").text().orElse("0")),
+ node.text().orElse("")
)
).collect(Collectors.toList());
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@eo-maven-plugin/src/main/java/org/eolang/maven/MjLint.java` around lines 445
- 460, The read method currently discards defect details (only using check and
severity); update it to extract and preserve the defect's optional attributes
and content from the Xnav node: read
node.attribute("line").text().map(Integer::parseInt).orElse(0) for the line
number, node.text().orElse("") for the defect message, and if present
node.attribute("file").text().orElse("") for the file/path, then pass these
values into the Defect.Default constructor (while keeping Severity.parsed(...)
for severity) so cached XMIR defects retain full fidelity; use safe parsing with
defaults to avoid exceptions in read/Xnav handling.
|
@yegor256 could you review this one, please? |
|
@volodya-lombrozo Great contribution! 🎉 You've earned +12 points (+16 base, -4 for 122 lines of code exceeding our 100-line guideline). Keep the quality contributions flowing - your running score is now +272! Don't forget to check your Zerocracy account for updates. |
This PR enables the WPA cache in
MjLintTest.java.Fixes #4983
Summary by CodeRabbit