Cannot compile valid Groovy code due to final variable not seen as having been initialized

[renatoathaydes]

Issue description

Valid Groovy class cannot be compiled (worked fine before upgrade) because a final variable is seen as possibly not initialized in the cleanup block.

How to reproduce

Attempt to compile this spec:

import spock.lang.Specification

class Broken3 extends Specification {
    def "test"() {
        final value = 'hello'

        expect:
        value.size() > 3

        cleanup:
        clean value
    }

    def clean(v) {}
}

Run:

./gradlew compileTestGroovy

Error:

> Task :compileTestGroovy FAILED
startup failed:
/Users/renato/programming/projects/spock-tests/src/test/groovy/Broken3.groovy: 11: The variable [value] may be uninitialized
. At [11:15]  @ line 11, column 15.
           clean value
                 ^

Expected outcome

Should compile ok.

Additional Environment information

From ./gradlew --version:

------------------------------------------------------------
Gradle 5.2.1
------------------------------------------------------------

Build time:   2019-02-08 19:00:10 UTC
Revision:     f02764e074c32ee8851a4e1877dd1fea8ffb7183

Kotlin DSL:   1.1.3
Kotlin:       1.3.20
Groovy:       2.5.4
Ant:          Apache Ant(TM) version 1.9.13 compiled on July 10 2018
JVM:          1.8.0_181 (Azul Systems, Inc. 25.181-b02)
OS:           Mac OS X 10.14.6 x86_64

Also attempted with Oracle's JVM and the result is the same:

JVM:          1.8.0_171 (Oracle Corporation 25.171-b11)

Groovy/Spock version

dependencies {
    compile 'org.spockframework:spock-core:1.3-groovy-2.5'
}

[renatoathaydes]

Also tried with JVM version 12, and the result is the same.

[leonard84]

Replace final by def and everything works fine. The issue here is that cleanup wraps everthing in a try-finally block. This does not work for finally as the initialization happens inside the try but the declaration must be outside the try so that it can be accessed in the catch.

try {
            java.lang.Object value 
            java.lang.Throwable $spock_feature_throwable 
            try {
                value = 'hello'
                try {
                    org.spockframework.runtime.SpockRuntime.verifyCondition($spock_errorCollector, $spock_valueRecorder.reset(), 'value.size() > 3', 8, 9, null, $spock_valueRecorder.record($spock_valueRecorder.startRecordingValue(5), $spock_valueRecorder.record($spock_valueRecorder.startRecordingValue(3), $spock_valueRecorder.record($spock_valueRecorder.startRecordingValue(0), value).$spock_valueRecorder.record($spock_valueRecorder.startRecordingValue(1), 'size')()) > $spock_valueRecorder.record($spock_valueRecorder.startRecordingValue(4), 3)))
                } 
                catch (java.lang.Throwable throwable) {
                    org.spockframework.runtime.SpockRuntime.conditionFailedWithException($spock_errorCollector, $spock_valueRecorder, 'value.size() > 3', 8, 9, null, throwable)} 
                finally { 
                } 
            } 
            catch (java.lang.Throwable $spock_tmp_throwable) {
                $spock_feature_throwable = $spock_tmp_throwable 
                throw $spock_tmp_throwable 
            } 
            finally { 
                try {
                    this.clean(value)
                } 
                catch (java.lang.Throwable $spock_tmp_throwable) {
                    if ( $spock_feature_throwable != null) {
                        $spock_feature_throwable.addSuppressed($spock_tmp_throwable)
                    } else {
                        throw $spock_tmp_throwable 
                    }
                } 
                finally { 
                } 
            } 
            this.getSpecificationContext().getMockController().leaveScope()
        } 
        finally { 
            $spock_errorCollector.validateCollectedErrors()
        } 

[renatoathaydes]

Why do you consider that this is not a problem? Breaking Groovy semantics should not be acceptable in my opinion, specially given that older versions of Spock did not break such semantics (and this seemed to break a large number of our tests because we use a lot of constants in our tests).

[leonard84]

Spock's logic did not change, it was groovy-2.5 that introduced this problem. See https://groovy-lang.org/releasenotes/groovy-2.5.html

Users combining final and @CompileStatic or final and Spock may see errors from the final variable analyzer. Work is underway to resolve those error messages. You may need to temporarily remove the final modifier in the meantime.

Any news on that front @paulk-asert ?

I don't see a way other than just silently removing the final for the cleanup/initializing mechanism to work like it does. If we move the variable out of the try-block, then you end up with weird re-ordering issues, e.g. if your final variable relies on the value of a non-final variable. If we move it fully in the try-block, then you could not access it in cleanup.

[brianeray]

Not to revive this issue but just to confirm, does this use case fall into the same category?

import spock.lang.*

class FinalVariableAnalysisSpec extends Specification {
  def "final String variable split across Spock blocks"() {
    when: "we define a finalized variable in this block"
    final foo = "bar"

    //Oddly enough if this block is commented and the downstream and: block 
    //becomes a then: block, then the FVA problem goes away. And I 
    //think this has to be a thrown(E) or notThrown(E) flavor of assertion. A 
    //simple assertion a la 1 == 1 does not trigger the FVA problem. Nor does  
    //another "wrapper" variant like verifyAll { 1 == 1 }.
    then: "no exception"
    notThrown(IllegalArgumentException)
    
    and: "we should be able to validate it in a later block"
    foo == "bar"
  }
}

//[...]FinalVariableAnalysisSpec.groovy: 20: The variable [foo] may be uninitialized
//. At [20:5]  @ line 20, column 5.
//       foo == "bar"
//
//With:
//  groovy:2.5.9:indy
//  spock-core:1.3-groovy-2.5

[brianeray]

Please ignore my previous comment. This wound up being less of an impact to our Specs than initially anticipated. Paul King is providing some feedback over on GROOVY-8346.