groovy
  1. groovy
  2. GROOVY-3161

Wrong initialization order for static fields in enums

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.5.6, 1.5.7, 1.6-beta-2
    • Fix Version/s: 1.6-rc-1, 1.5.8, 1.7-beta-1
    • Component/s: None
    • Labels:
      None
    • Environment:
      Gentoo Linux, SUN JVM 1.6.0.10
    • Testcase included:
      yes
    • Number of attachments :
      4

      Description

      In an enum declaration, i'm using a static list that holds references to the enum values. The list is not initialized correctly, all values are set to null instead of the expected enum.

      1. 3161_v15x.diff
        4 kB
        Roshan Dawrani
      2. 3161_v16x.diff
        4 kB
        Roshan Dawrani
      3. 3161_v17x.diff
        4 kB
        Roshan Dawrani
      4. EnumTest.groovy
        0.3 kB
        Jochen Hinrichsen

        Activity

        Hide
        Roshan Dawrani added a comment -

        Hi,
        Attaching the patch for 1.5.8 as a partial fix. In 1.6, the same fix is resulting in
        some errors and I am looking into them. If I am able to resolve them, I'll update the patches.

        The issue is related to Verifier pushing the static enum fields defined by user to the
        beginning of the static init block of the enum class (before the enum values are initialized).

        When EnumVisitor visits an enum class node, it creates a <clinit>(static init) block
        in which it defines the enum values. But when Verifier visits this class node later, if
        it sees some static fields, it adds their AST statements to the beginning of the <clinit> block.

        enum GroovyColors {
            // AST statements for following values are first added to <clinit> by EnumVisitor
            red, blue, green
        
            /* Later, ExpressionStatement for this static Field gets moved by Verifier
            before the red/blue/green in <clinit> */
            static def ALL_COLORS = [red, blue, green]
        }
        
        // since <clinit> has declaration of ALL_COLORS before red/blue/green are being 
        set, instead of having the correct values, it has un-initialized null values.
        println GroovyColors.ALL_COLORS
        

        Here is the test case with which I have verified this partial fix:

        class Groovy3161Bug extends GroovyTestCase {
            def void testStaticEnumFieldWithEnumValues() {
                def allColors = GroovyColors3161.ALL_COLORS
                assert allColors.size == 3
                assert allColors[0] == GroovyColors3161.red
                assert allColors[1] == GroovyColors3161.blue
                assert allColors[2] == GroovyColors3161.green
            }
        }
        enum GroovyColors3161 {
            red, blue, green
            static def ALL_COLORS = [red, blue, green]
        }
        
        Show
        Roshan Dawrani added a comment - Hi, Attaching the patch for 1.5.8 as a partial fix. In 1.6, the same fix is resulting in some errors and I am looking into them. If I am able to resolve them, I'll update the patches. The issue is related to Verifier pushing the static enum fields defined by user to the beginning of the static init block of the enum class (before the enum values are initialized). When EnumVisitor visits an enum class node, it creates a <clinit>(static init) block in which it defines the enum values. But when Verifier visits this class node later, if it sees some static fields, it adds their AST statements to the beginning of the <clinit> block. enum GroovyColors { // AST statements for following values are first added to <clinit> by EnumVisitor red, blue, green /* Later, ExpressionStatement for this static Field gets moved by Verifier before the red/blue/green in <clinit> */ static def ALL_COLORS = [red, blue, green] } // since <clinit> has declaration of ALL_COLORS before red/blue/green are being set, instead of having the correct values, it has un-initialized null values. println GroovyColors.ALL_COLORS Here is the test case with which I have verified this partial fix: class Groovy3161Bug extends GroovyTestCase { def void testStaticEnumFieldWithEnumValues() { def allColors = GroovyColors3161.ALL_COLORS assert allColors.size == 3 assert allColors[0] == GroovyColors3161.red assert allColors[1] == GroovyColors3161.blue assert allColors[2] == GroovyColors3161.green } } enum GroovyColors3161 { red, blue, green static def ALL_COLORS = [red, blue, green] }
        Hide
        Roshan Dawrani added a comment -

        Attaching the fix for versions 1.6-RC-1 / 1.7-beta-1 and a revised patch for 1.5.8.

        Briefly, the fix applied is only this: If there are any explicitly declared static fields
        inside an enum, their initialization inside the <clinit> block should not come
        before the initialization of the enum values. Otherwise if such a static field uses enum
        values, it does not see the initialized enum value.

        The changes have been made to org.codehaus.groovy.classgen.Verifier's addInitialization() and addFieldInitialization() methods to handle initializaton of static fields inside an enum a little differently.

        Hope it helps.

        rgds,
        Roshan

        Show
        Roshan Dawrani added a comment - Attaching the fix for versions 1.6-RC-1 / 1.7-beta-1 and a revised patch for 1.5.8. Briefly, the fix applied is only this: If there are any explicitly declared static fields inside an enum, their initialization inside the <clinit> block should not come before the initialization of the enum values. Otherwise if such a static field uses enum values, it does not see the initialized enum value. The changes have been made to org.codehaus.groovy.classgen.Verifier's addInitialization() and addFieldInitialization() methods to handle initializaton of static fields inside an enum a little differently. Hope it helps. rgds, Roshan
        Hide
        Roshan Dawrani added a comment -

        Hi,
        If the attached patches can be reviewed, we can move forward on this issue one way or other.

        Thanks,
        Roshan

        Show
        Roshan Dawrani added a comment - Hi, If the attached patches can be reviewed, we can move forward on this issue one way or other. Thanks, Roshan
        Hide
        blackdrag blackdrag added a comment -

        the patch is ok for me

        Show
        blackdrag blackdrag added a comment - the patch is ok for me
        Hide
        Roshan Dawrani added a comment -

        Fixed by ensuring that enum values are initialized before any static fields explicitly declared in the enum.

        Show
        Roshan Dawrani added a comment - Fixed by ensuring that enum values are initialized before any static fields explicitly declared in the enum.

          People

          • Assignee:
            Roshan Dawrani
            Reporter:
            Jochen Hinrichsen
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: