diff --git a/specs/S7-validation.md b/specs/S7-validation.md index 437f45c..2a2ae48 100644 --- a/specs/S7-validation.md +++ b/specs/S7-validation.md @@ -47,9 +47,9 @@ Set JAVA_HOME to JDK 25 and run from the repository root. | Command/check | Result | | --- | --- | -| `mvn -Dformat.mode=check clean verify` | Passed: 38 backend tests, 17 checker tests, standards checks and the exact backend coverage gate. | +| `mvn -Dformat.mode=check clean verify` | Passed: 38 backend tests, 19 checker tests, standards checks and the exact backend coverage gate. | | `mvn -pl agiledancers-be package -DskipTests` | Passed; Quarkus packaged app successfully produced and subsequently exercised by Playwright. | -| `mvn -f tooling/standards/pom.xml test exec:java` | 17 checker tests passed; no unwaived blocking violations. | +| `mvn -f tooling/standards/pom.xml test exec:java` | 19 checker tests passed; no unwaived blocking violations. | | `java tooling/coverage/Coverage.java agiledancers-be/target/site/jacoco/jacoco.xml` | Clean backend-only line gate passed: 791 covered / 818 total = 96.69926650366749%. | | `PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH=/opt/google/chrome/chrome npm run e2e` in frontend | Both browser tests passed against the packaged instrumented backend. | | Pinned CLI `dump --address 127.0.0.1 --destfile agiledancers-be/target/playwright.exec` | Successfully collected execution data from the agent's TCP server. | @@ -111,3 +111,15 @@ The maintainer approved both proposals in the S7 implementation conversation: There are no parameter waivers for the three service methods, blanket suppressions or weakened thresholds. The completed specification resides in `specs/in_review`. + +## Review fixes + +The review fixes enforce finality for assign-once pattern bindings, including +`instanceof`, switch and record-deconstruction bindings, while allowing reassigned +bindings and inferred record-pattern types. Trivial-accessor detection now uses +AST statements and resolved field/parameter references, so comments do not change +public-method counts. Accessors with calls, extra statements or compound +assignments still count. Two regression tests cover these cases. + +After these fixes, JDK 25 `mvn -B -ntp -Dformat.mode=check verify` passed with +38 backend tests, 19 checker tests and backend line coverage of 791/818 (96.70%). diff --git a/tooling/standards/src/main/java/Standards.java b/tooling/standards/src/main/java/Standards.java index 98c4e2c..4e0259d 100644 --- a/tooling/standards/src/main/java/Standards.java +++ b/tooling/standards/src/main/java/Standards.java @@ -356,9 +356,9 @@ public class Standards { static void variable(ASTVariableId id) { if (id.isEnumConstant() && !id.getName().matches("[A-Z][A-Z0-9]*(_[A-Z0-9]+)*")) block(id, "Enum constants require UPPER_SNAKE_CASE"); - if (id.isRecordComponent() || id.isEnumConstant() || id.isPatternBinding() || id.isUnnamed()) + if (id.isRecordComponent() || id.isEnumConstant() || id.isUnnamed()) return; - if (id.isTypeInferred() && !id.isLambdaParameter()) { + if (id.isTypeInferred() && !id.isLambdaParameter() && !id.isPatternBinding()) { final ASTExpression initializer = id.getInitializer(); if (!(initializer instanceof ASTConstructorCall) && !(initializer instanceof ASTCastExpression) @@ -447,12 +447,8 @@ public class Standards { } static boolean trivial(ASTExecutableDeclaration method) { - if (method.getBody() == null) return false; - final String body = method - .getTextDocument() - .sliceOriginalText(method.getBody().getTextRegion()) - .toString() - .replaceAll("\\s+", ""); + if (method.getBody() == null || method.getBody().getNumChildren() != 1) return false; + final JavaNode statement = method.getBody().getChild(0); final String name = method.getName(); String property = name; if (name.startsWith("get") || name.startsWith("set")) property = name.substring(3); @@ -463,12 +459,26 @@ public class Standards { if (method.getEnclosingType().getSymbol().getDeclaredFields().stream() .noneMatch(f -> f.getSimpleName().equals(fieldName))) return false; if (method.getArity() == 0) - return body.equals("{return" + property + ";}") - || body.equals("{returnthis." + property + ";}"); + return statement instanceof ASTReturnStatement returned + && accessorField(returned.getExpr(), method, property); if (method.getArity() != 1 || !name.startsWith("set")) return false; - final String parameter = method.getFormalParameters().get(0).getVarId().getName(); - return body.equals("{this." + property + "=" + parameter + ";}") - || !property.equals(parameter) && body.equals("{" + property + "=" + parameter + ";}"); + return statement instanceof ASTExpressionStatement expression + && expression.getExpr() instanceof ASTAssignmentExpression assignment + && !assignment.isCompound() + && accessorField(assignment.getLeftOperand(), method, property) + && assignment.getRightOperand() instanceof ASTVariableAccess value + && method.getFormalParameters().get(0).getVarId().getSymbol() + .equals(value.getReferencedSym()); + } + + static boolean accessorField( + ASTExpression expression, ASTExecutableDeclaration method, String property) { + if (!(expression instanceof ASTAssignableExpr.ASTNamedReferenceExpr reference)) return false; + if (expression instanceof ASTFieldAccess field + && !(field.getQualifier() instanceof ASTThisExpression)) return false; + return method.getEnclosingType().getSymbol().getDeclaredFields().stream() + .anyMatch(field -> field.getSimpleName().equals(property) + && field.equals(reference.getReferencedSym())); } record Dependency(JTypeMirror type, Set qualifiers) {} diff --git a/tooling/standards/src/test/java/StandardsTest.java b/tooling/standards/src/test/java/StandardsTest.java index 09f83ac..2c6e77e 100644 --- a/tooling/standards/src/test/java/StandardsTest.java +++ b/tooling/standards/src/test/java/StandardsTest.java @@ -145,6 +145,35 @@ class StandardsTest { assertFalse(findings.stream().anyMatch(s -> s.contains("requires final: count"))); } + @Test + void patternBindingsRequireFinalOnlyWhenAssignedOnce() throws Exception { + var findings = analyze(""" + record Pair(Object key, Object value) {} + class Fixture { + public void run(final Object input) { + if (input instanceof String unchanged) { System.out.println(unchanged); } + if (input instanceof final String protectedValue) { System.out.println(protectedValue); } + if (input instanceof String reassigned) { reassigned = reassigned.trim(); System.out.println(reassigned); } + switch (input) { + case String switched -> System.out.println(switched); + default -> {} + } + if (input instanceof Pair(var key, var value)) { + System.out.println(key); + System.out.println(value); + } + } + } + """); + for (String binding : List.of("unchanged", "switched", "key", "value")) + assertTrue(findings.stream().anyMatch(s -> s.contains("requires final: " + binding)), + findings.toString()); + for (String binding : List.of("protectedValue", "reassigned")) + assertFalse(findings.stream().anyMatch(s -> s.contains("requires final: " + binding)), + findings.toString()); + assertFalse(findings.stream().anyMatch(s -> s.contains("var requires")), findings.toString()); + } + @Test void inheritanceCountsOnlyProjectSuperclasses() throws Exception { var findings = analyze( @@ -202,6 +231,30 @@ class StandardsTest { assertTrue(contains(findings, "METRIC", "publicMethods=3.0"), findings.toString()); } + @Test + void commentedAccessorsRemainTrivialWhileAdditionalWorkStillCounts() throws Exception { + var findings = analyze(""" + class Fixture { + private String name; + private String trimmed; + private String logged; + private String calculated; + private String appended; + public String getName() { /* Intent comment. */ return name; } + public void setName(final String value) { this./* Field comment. */name = value; } + public String name() { // Read the stored value. + return this.name; + } + public void setName(final String value, final boolean validate) { this.name = value; } + public void setTrimmed(final String value) { this.trimmed = value.trim(); } + public String getLogged() { System.out.println(logged); return logged; } + public String getCalculated() { return calculated.trim(); } + public void setAppended(final String value) { this.appended += value; } + } + """); + assertTrue(contains(findings, "METRIC", "publicMethods=5.0"), findings.toString()); + } + @Test void classLengthBoundaryAndNestedTypeLines() throws Exception { assertTrue(contains(