[
https://issues.apache.org/jira/browse/GROOVY-12463?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18128585#comment-18128585
]
ASF GitHub Bot commented on GROOVY-12463:
-----------------------------------------
paulk-asert opened a new pull request, #2988:
URL: https://github.com/apache/groovy/pull/2988
… checked on a trailing implicit return when the method also has an explicit
early return
> 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)
