[
https://issues.apache.org/jira/browse/GROOVY-12463?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18128879#comment-18128879
]
ASF GitHub Bot commented on GROOVY-12463:
-----------------------------------------
Copilot commented on code in PR #2988:
URL: https://github.com/apache/groovy/pull/2988#discussion_r4317886360
##########
subprojects/groovy-contracts/src/main/java/org/apache/groovy/contracts/generation/AssertStatementCreationUtility.java:
##########
@@ -95,29 +95,22 @@ public static AssertStatement getAssertionStatement(final
BooleanExpression bool
* @return a {@link ReturnStatement} or <tt>null</tt>
*/
public static List<ReturnStatement> getReturnStatements(MethodNode method)
{
-
+ // GROOVY-12129: delegate implicit-return conversion to the compiler's
own ReturnAdder.
+ // The value-producing trailing statement may be a plain expression
statement, but also
+ // an if/else whose branches are expressions, a switch, or a try/catch
(e.g. after
+ // method-level @Decreases wrapping); converting only the first case
bound `result` to
+ // a synthesized default — and the synthesized `return <default>`
changed what the
+ // woven method returned. ReturnAdder performs exactly the conversion
the compiler
+ // would apply later anyway (including a default-value return for an
empty body,
+ // preserving GROOVY-12082 semantics), so weaving at its returns
cannot change what
+ // the method computes. Void methods are left untouched by ReturnAdder.
+ // GROOVY-12463: the conversion must run even when the method already
contains explicit
+ // returns: an early `return` does not make the trailing value
explicit, and skipping the
+ // conversion left that path unchecked. ReturnAdder leaves existing
returns unchanged.
+ new ReturnAdder().visitMethod(method);
Review Comment:
`getReturnStatements` now always mutates the passed `MethodNode` by running
`ReturnAdder`. If this utility is invoked more than once for the same
`MethodNode` during weaving/visiting, it can cause repeated AST transformation
work and makes call-order more fragile. Consider guarding with node metadata
(e.g., set a flag on the `MethodNode` after applying `ReturnAdder`) so the
conversion is applied at most once per method.
##########
subprojects/groovy-contracts/src/main/java/org/apache/groovy/contracts/generation/AssertStatementCreationUtility.java:
##########
@@ -95,29 +95,22 @@ public static AssertStatement getAssertionStatement(final
BooleanExpression bool
* @return a {@link ReturnStatement} or <tt>null</tt>
*/
public static List<ReturnStatement> getReturnStatements(MethodNode method)
{
Review Comment:
The Javadoc return description is incorrect: the method returns a
`List<ReturnStatement>` (likely empty), not a single `ReturnStatement` or
`null`. Please update `@return` to reflect the actual return type/semantics
(e.g., “list of return statements (possibly empty)”).
> groovy-contracts: postcondition and class invariant not checked on a trailing
> implicit return when the method also has an explicit early return
> -----------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12463
> URL: https://issues.apache.org/jira/browse/GROOVY-12463
> Project: Groovy
> Issue Type: Bug
> Components: groovy-contracts
> Affects Versions: 6.0.0
> Reporter: Paul King
> Priority: Major
>
> h2. Problem
> When a non-void method has an explicit {{return}} on one path and produces
> its value through an implicit return on another, the implicit path skips the
> contract checks. Neither the {{@Ensures}} postcondition nor the class
> {{@Invariant}} is checked there, and the method returns an unchecked value.
> {code:groovy}
> import groovy.contracts.*
> class H {
> @Ensures({ result > 0 })
> int implicitAfterEarly(boolean b) { if (b) return 1; -1 }
> @Ensures({ result > 0 })
> int explicitBoth(boolean b) { if (b) return 1; return -1 }
> @Ensures({ result > 0 })
> int ifElseImplicit(boolean b) { if (b) { 1 } else { -1 } }
> }
> def h = new H()
> h.implicitAfterEarly(false) // returns -1, no PostconditionViolation
> h.explicitBoth(false) // PostconditionViolation
> h.ifElseImplicit(false) // PostconditionViolation
> {code}
> The same happens with {{@CompileStatic}} and with class invariants:
> {code:groovy}
> @Invariant({ count >= 0 })
> class K {
> int count = 0
> int dec(boolean b) { if (b) return count; count -= 5; count }
> }
> new K().dec(false) // returns -5, no ClassInvariantViolation
> {code}
> This is easy to hit with guard clauses, a common style: return early for bad
> input, then let the last expression be the result. Found while writing a
> contract for a date parser of that shape, where a 31 February result was not
> caught.
> h2. Cause
> {{AssertStatementCreationUtility.getReturnStatements}} collects the method's
> explicit {{ReturnStatement}} nodes. It runs {{ReturnAdder}}, which turns
> trailing expressions and value-producing if/else, switch and try statements
> into returns, only when that list is empty. GROOVY-12129 introduced the
> {{ReturnAdder}} delegation, but its tests do not cover a method that mixes
> the two. With any explicit return present, the trailing expression stays an
> expression statement, no assertion is woven before it, and the compiler adds
> the return later, unchecked.
> All three callers are affected:
> * {{PostconditionGenerator}}: {{@Ensures}}.
> * {{ClassInvariantGenerator}}: invariant checks on non-void methods.
> * {{AnnotationClosureVisitor.recordNonNullReturnViolations}}: the {{return
> null}} paths handed to NullChecker.
> h2. Fix
> Always run {{ReturnAdder}} before collecting the returns. {{ReturnAdder}}
> performs the conversion the compiler applies later anyway and leaves existing
> return statements unchanged. Applying it unconditionally does not change what
> the method computes; it only makes every exit path visible to the weaving.
> h2. Tests
> Added to {{ImplicitReturnPostconditionTests}} and
> {{SimpleClassInvariantTests}}:
> * trailing implicit return after an early return is checked (violation
> detected);
> * the returned values are unchanged by weaving (vacuous postcondition);
> * trailing implicit if/else after an early return is checked;
> * the same under {{@CompileStatic}}, with an {{Optional}} result;
> * the class invariant is checked on the trailing implicit path.
> Four of the five fail before the fix. The values test passes either way,
> because the bug skipped the check without changing the value. With the fix,
> the full groovy-contracts suite (364 tests) and groovy-typecheckers suite
> (356 tests) pass.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
