Uploaded image for project: 'Groovy'
  1. Groovy
  2. GROOVY-506

Invoker.asArray, does not function w/ Constructors with (String[]) params

Details

    • Bug
    • Status: Closed
    • Major
    • Resolution: Fixed
    • 1.0-beta-6
    • 1.0-JSR-2
    • None
    • None

    Description

      Invoker.asArray() does not do the right thing when converting input args into compatible args for Constructor.newInstance().

      Specifically, when calling a constructor with one argument of type String[], asArray() does not wrap the input arguments in a new Object[], which results in passing the input String[] to the constructor, which then throws an IllegalArumentException, as it expected something like: new Object[]

      { stringArray }

      My guess is that this problem occurs for all constructors which take a single argument, whoes value passes an instanceof Object[] test.

      I tested that hacking up Invoker.asArray with:

      if (arguments instanceof String[]) {
      return new Object[]

      { arguments }

      ;
      }

      did the right thing.

      --jason

      Attachments

        1. AsmClassGenerator.patch
          0.7 kB
          Jason Dillon
        2. ConstructorArgumentsTest.groovy
          0.7 kB
          Jason Dillon

        Activity

          jason@planet57.com Jason Dillon added a comment -

          Array transformation should really be done later, when there is a Method or Constructor instance to use to generate the correct Object[] for invocation. Invoker does not have enough information to create the proper array.

          jason@planet57.com Jason Dillon added a comment - Array transformation should really be done later, when there is a Method or Constructor instance to use to generate the correct Object[] for invocation. Invoker does not have enough information to create the proper array.
          jason@planet57.com Jason Dillon added a comment -

          Looking into this a little more, I am starting to believe that whatever is getting the argumenst initially may be broken.

          Basically, instead of passing Object[Object[]] up the invoke chain, it only passes up Object[].

          jason@planet57.com Jason Dillon added a comment - Looking into this a little more, I am starting to believe that whatever is getting the argumenst initially may be broken. Basically, instead of passing Object[Object[]] up the invoke chain, it only passes up Object[].
          jason@planet57.com Jason Dillon added a comment -

          I think I have found the problem. AsmClassGenerator.visitConstructorCallExpression() will attempt to unroll a TupleExpression expression of size = 1

          <snip>
          public void visitConstructorCallExpression(ConstructorCallExpression call) {
          this.leftHandExpression = false;

          Expression arguments = call.getArguments();
          if (arguments instanceof TupleExpression) {
          TupleExpression tupleExpression = (TupleExpression) arguments;
          int size = tupleExpression.getExpressions().size();
          if (size == 0)

          { arguments = null; }

          else if (size == 1)

          { arguments = (Expression) tupleExpression.getExpressions().get(0); }

          }
          // ...
          }
          </snip>

          The problem is with:

          <snip>
          else if (size == 1) {
          arguments = (Expression) tupleExpression.getExpressions().get(0);
          }
          </snip>

          Unrolling the TupleExpression, when size = 1 will result in loss of detail when attempting to invoke constructors.

          When invoking constructors and methods, the arguments should all be Object[], which is a collection of the arguments actually given. Attempting to unroll this list is harmful, and will only result in errors when trying to invoke the target constructor/method down the invocation chain.

          Removing this unroll also fixes GROOVY-507.

          jason@planet57.com Jason Dillon added a comment - I think I have found the problem. AsmClassGenerator.visitConstructorCallExpression() will attempt to unroll a TupleExpression expression of size = 1 <snip> public void visitConstructorCallExpression(ConstructorCallExpression call) { this.leftHandExpression = false; Expression arguments = call.getArguments(); if (arguments instanceof TupleExpression) { TupleExpression tupleExpression = (TupleExpression) arguments; int size = tupleExpression.getExpressions().size(); if (size == 0) { arguments = null; } else if (size == 1) { arguments = (Expression) tupleExpression.getExpressions().get(0); } } // ... } </snip> The problem is with: <snip> else if (size == 1) { arguments = (Expression) tupleExpression.getExpressions().get(0); } </snip> Unrolling the TupleExpression, when size = 1 will result in loss of detail when attempting to invoke constructors. When invoking constructors and methods, the arguments should all be Object[], which is a collection of the arguments actually given. Attempting to unroll this list is harmful, and will only result in errors when trying to invoke the target constructor/method down the invocation chain. Removing this unroll also fixes GROOVY-507 .
          jason@planet57.com Jason Dillon added a comment -

          Patch to remove argument unrolling

          jason@planet57.com Jason Dillon added a comment - Patch to remove argument unrolling
          jason@planet57.com Jason Dillon added a comment -

          Test case for fixes to GROOVY-506 & GROOVY-507

          jason@planet57.com Jason Dillon added a comment - Test case for fixes to GROOVY-506 & GROOVY-507
          bingran Bing Ran added a comment -

          Yes, I have identified the problem too and have the same fix in my local copy. Will include the fix soon.

          bingran Bing Ran added a comment - Yes, I have identified the problem too and have the same fix in my local copy. Will include the fix soon.

          seems to be fixed

          blackdrag Jochen Theodorou added a comment - seems to be fixed
          phkim Kim, Pilho added a comment -

          Lets look at:
          String[] args = new String[]

          { "a", "b", "c" }

          ;

          This is used often in Java language.
          But the above code can not be used in Groovy language,
          because the braces "

          {" and "}

          " mean a block (for example, a closure).
          Now we should use the brackets "[" and "]" to create an array.

          Since groovy-1.0-har-01, we can use one of the followings:
          String[] args = [ "a", "b", "c" ]
          String[] args = [ "a", "b", "c" ] as String[]
          def args = [ "a", "b", "c" ] as String[]

          Here is my changed example:

          class ConstructorArgumentsTest extends GroovyTestCase {
          void testStringArray()

          { String[] args = [ "a", "b", "c" ] obj = new ConstructorArgumentsTestoid(args) }

          void testStringArray2()

          { String[] args = [ "a", "b", "c" ] obj = new ConstructorArgumentsTestoid(args, args) }

          }

          class ConstructorArgumentsTestoid {
          ConstructorArgumentsTestoid(String[] args)

          { assert args.length == 3 assert args[2] == "c" }

          ConstructorArgumentsTestoid(String[] a, String[] b)

          { assert a == b assert a.length == 3 assert a[2] == "c" assert b.length == 3 assert b[2] == "c" }

          }

          I will close this issue after a few of days if none reply to my comment.

          phkim Kim, Pilho added a comment - Lets look at: String[] args = new String[] { "a", "b", "c" } ; This is used often in Java language. But the above code can not be used in Groovy language, because the braces " {" and "} " mean a block (for example, a closure). Now we should use the brackets " [" and "] " to create an array. Since groovy-1.0-har-01, we can use one of the followings: String[] args = [ "a", "b", "c" ] String[] args = [ "a", "b", "c" ] as String[] def args = [ "a", "b", "c" ] as String[] Here is my changed example: class ConstructorArgumentsTest extends GroovyTestCase { void testStringArray() { String[] args = [ "a", "b", "c" ] obj = new ConstructorArgumentsTestoid(args) } void testStringArray2() { String[] args = [ "a", "b", "c" ] obj = new ConstructorArgumentsTestoid(args, args) } } class ConstructorArgumentsTestoid { ConstructorArgumentsTestoid(String[] args) { assert args.length == 3 assert args[2] == "c" } ConstructorArgumentsTestoid(String[] a, String[] b) { assert a == b assert a.length == 3 assert a[2] == "c" assert b.length == 3 assert b[2] == "c" } } I will close this issue after a few of days if none reply to my comment.

          People

            blackdrag Jochen Theodorou
            jason@planet57.com Jason Dillon
            Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: