Groovy
  1. Groovy
  2. GROOVY-3311

NPE on Groovy 1.6-RC2: Cannot invoke method times() on null object

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.6-beta-2, 1.6-rc-1, 1.6-rc-2
    • Fix Version/s: 1.6-rc-3, 1.6.1, 1.7-beta-1
    • Component/s: None
    • Labels:
      None

      Description

      The following code breaks Groovy 1.6 with NPE, while passes fine on Groovy 1.5.7:

      [code]
      --------- test.gr --------------
      public class Day extends Date {

      static sdf = new java.text.SimpleDateFormat("dd.MM.yyyy")

      public static Day get(_date)

      { return new Day(sdf.parse(_date)) }

      def hours = []

      public Day(Date _date) {
      super(_date.time)
      def time = getTime()
      24.times

      { hour -> hours << new Date(time + hour*1000*60*60) }

      }

      public String toString()

      { sdf.format(this) }

      static def period = (1..3).toList().collect { get "1$

      {it}

      .11.2007" }
      }

      println new Day(new Date())
      --------------------------------
      [/code]

      >> groovy-1.6-RC2/bin/groovy test.gr
      [code]
      Caught: java.lang.ExceptionInInitializerError
      java.lang.ExceptionInInitializerError
      at java.lang.Class.forName0(Native Method)
      at java.lang.Class.forName(Class.java:169)
      . . .
      Caused by: java.lang.NullPointerException: Cannot invoke method times() on null object
      at org.codehaus.groovy.runtime.NullObject.invokeMethod(NullObject.java:77)
      at org.codehaus.groovy.runtime.InvokerHelper.invokePogoMethod(InvokerHelper.java:743)
      at org.codehaus.groovy.runtime.InvokerHelper.invokeMethod(InvokerHelper.java:720)
      at org.codehaus.groovy.runtime.callsite.NullCallSite.call(NullCallSite.java:17)
      at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:43)
      at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:116)
      at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:124)
      at Day.<init>(test.gr:14)
      at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
      . . .
      [/code]

        Activity

        Hide
        Jochen Theodorou added a comment -

        the code can be reduced to

        public class Day  {
          static period = new Day()
          static period2 = 24
          public Day() {
           println 24
          }
        }
        println Day.period2

        If you execute the above, then you will get the output that 24 is first null and then the normal 24. I assume that since we use predefined constants in static fields, that the constant is not initialized when period gets its value. But something prevents period2 from getting null too. Either that number is not cached in a field or some special ordering happens.

        Show
        Jochen Theodorou added a comment - the code can be reduced to public class Day { static period = new Day() static period2 = 24 public Day() { println 24 } } println Day.period2 If you execute the above, then you will get the output that 24 is first null and then the normal 24. I assume that since we use predefined constants in static fields, that the constant is not initialized when period gets its value. But something prevents period2 from getting null too. Either that number is not cached in a field or some special ordering happens.
        Hide
        Roshan Dawrani added a comment -

        Looks like an ordering related thing, which even Java suffers from (or may be it is as designed). The code in the groovy example above (Day class) becomes equivalent of this Java code because of synthetic constant fields introduced by Groovy compiler for numbers/characters.

        public class Main {
            public static void main(String[] args){
            	Test t2 = Test.t; // trigger the static code's execution in Test class
            }
        }
        class Test {
        	public static Test t = new Test();
        	static int f = 1;
        	Test() {
        		System.out.println(f); //prints 0 and not 1
        	}
        }
        
        Show
        Roshan Dawrani added a comment - Looks like an ordering related thing, which even Java suffers from (or may be it is as designed). The code in the groovy example above (Day class) becomes equivalent of this Java code because of synthetic constant fields introduced by Groovy compiler for numbers/characters. public class Main { public static void main( String [] args){ Test t2 = Test.t; // trigger the static code's execution in Test class } } class Test { public static Test t = new Test(); static int f = 1; Test() { System .out.println(f); //prints 0 and not 1 } }
        Hide
        Roshan Dawrani added a comment -

        The de-compiled code for the Day class in the example provided looks like below, which is similar to Java example above:

        public class Day implements GroovyObject {
          private static Object period;
          public Day()
          {
            ....
            println($const$0);
            ....
          }
        
          static
          {
             Day.period = new Day();
             $const$0 = new Integer(24); // constant is getting set only now but used in the constructor call invoked above
             ...
          }
        ...
        }
        
        Show
        Roshan Dawrani added a comment - The de-compiled code for the Day class in the example provided looks like below, which is similar to Java example above: public class Day implements GroovyObject { private static Object period; public Day() { .... println($ const $0); .... } static { Day.period = new Day(); $ const $0 = new Integer (24); // constant is getting set only now but used in the constructor call invoked above ... } ... }
        Hide
        Jochen Theodorou added a comment -

        normally the static intializer is run before the constructor can be invoked, But in this case here the static initializer runs the constructor. The compiler has to ensure that all constants are intialized by that time. so changing the order should solve the problem

        Show
        Jochen Theodorou added a comment - normally the static intializer is run before the constructor can be invoked, But in this case here the static initializer runs the constructor. The compiler has to ensure that all constants are intialized by that time. so changing the order should solve the problem
        Hide
        Roshan Dawrani added a comment -

        Yes, changing the order should solve the problem.

        Out of curiosity, what is the use of creating synthetic fields for such numbers/characters used in groovy classes? Do they help much somewhere?

        Show
        Roshan Dawrani added a comment - Yes, changing the order should solve the problem. Out of curiosity, what is the use of creating synthetic fields for such numbers/characters used in groovy classes? Do they help much somewhere?
        Hide
        Jochen Theodorou added a comment -

        think of code like

        1000.times {
          i = 24
        }
        

        before that change we created 1000 Integer instances with the value 24. Now we reuse 1 instance and do a simple assignment here. So it can improve performance very much

        Show
        Jochen Theodorou added a comment - think of code like 1000.times { i = 24 } before that change we created 1000 Integer instances with the value 24. Now we reuse 1 instance and do a simple assignment here. So it can improve performance very much
        Hide
        Roshan Dawrani added a comment -

        I have fixed on 1.6.1 and 1.7-beta-1.

        IMO, it should be fixed on 1.6 too. It would seem silly if (24 != null) didn't evaluate to true in the latest groovy in the usage below, which is not very uncommon.

        class Groovy3311Bug extends GroovyTestCase  {
            static period = new Groovy3311Bug()
            def Groovy3311Bug() {
                assert (24 != null)
            }
        
            def void testStaticInit() {
                println Groovy3311Bug.class / trigger the static init
            }
        }
        
        Show
        Roshan Dawrani added a comment - I have fixed on 1.6.1 and 1.7-beta-1. IMO, it should be fixed on 1.6 too. It would seem silly if (24 != null) didn't evaluate to true in the latest groovy in the usage below, which is not very uncommon. class Groovy3311Bug extends GroovyTestCase { static period = new Groovy3311Bug() def Groovy3311Bug() { assert (24 != null ) } def void testStaticInit() { println Groovy3311Bug.class / trigger the static init } }
        Hide
        Guillaume Delcroix added a comment -

        Yes, you're right, I agree, this is the kind of bug that deserves being committed to our new 1.6.0 branch.

        Show
        Guillaume Delcroix added a comment - Yes, you're right, I agree, this is the kind of bug that deserves being committed to our new 1.6.0 branch.
        Hide
        Roshan Dawrani added a comment -

        I am not familiar with merging from one branch to another branch. Can I do the usual check-in to 1.6.0 branch?

        Show
        Roshan Dawrani added a comment - I am not familiar with merging from one branch to another branch. Can I do the usual check-in to 1.6.0 branch?
        Hide
        Guillaume Delcroix added a comment -

        I must confess I usually rarely use the svn merge command, and instead create patches I reapply to other branches – not the best practice I guess, but one I'm used to do anyway.
        I think you need a commit after an svn merge anyway, so you should be able to try it safely locally, if I'm not mistaken.
        You may also copy'n paste back the areas where you made changes, as a workaround.

        Show
        Guillaume Delcroix added a comment - I must confess I usually rarely use the svn merge command, and instead create patches I reapply to other branches – not the best practice I guess, but one I'm used to do anyway. I think you need a commit after an svn merge anyway, so you should be able to try it safely locally, if I'm not mistaken. You may also copy'n paste back the areas where you made changes, as a workaround.
        Hide
        Roshan Dawrani added a comment -

        I know about the delta code that I applied and I can safely apply it on 1.6.0.

        Shall I go ahead and make this change on 1.6.0 then?

        Show
        Roshan Dawrani added a comment - I know about the delta code that I applied and I can safely apply it on 1.6.0. Shall I go ahead and make this change on 1.6.0 then?
        Hide
        Guillaume Delcroix added a comment -

        As suggested by Jochen on the list, we should discuss any commit on the mailing-list first.
        So bring that again on the list, and Jochen will confirm if it's okay.
        But I think we should bring it in 1.6.0 as well.

        Show
        Guillaume Delcroix added a comment - As suggested by Jochen on the list, we should discuss any commit on the mailing-list first. So bring that again on the list, and Jochen will confirm if it's okay. But I think we should bring it in 1.6.0 as well.
        Hide
        Roshan Dawrani added a comment -

        Fixed the issue on 1.6.0 branch too.

        Show
        Roshan Dawrani added a comment - Fixed the issue on 1.6.0 branch too.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development