Single super constructor argument is casted to array if super constructor has vararg parameter

The ClassCastException part of this issue has been fixed under GROOVY-4015.

What is remaining now is that super("String") call to Foo(String...s) is becoming equivalent to "String" as String[] and reaching the vararg String[] of Foo#<init>() as [S, t, r, i, n, g].

Why is it that this()/super() calls are handled specially and don't go through the callsites as normal method calls and constructor invocations do?

Because of special handling done there for this()/super() calls, the ParameterTypes#correctArguments() logic, which takes care of varargs in case of usual method calls, will need to be replicated there, is it?

They cannot go through normal callsites, because an invokespecial is needed for that. And that invokespecial needs to be comming from the subclass. What the groovy compiler does is generating a table of super(..) calls if super is used, one for each constructure of the super class. Then the arguments are used to select the right contructor and the final call is then done using normal java bytecode with invokespecial. So not only do you need to coerce the arguments, you also need to but them on the stack according to what java needs.

About your comment before... do you tell em that foo("String") reaches foo(String[]) with an String[], that consists of the strings "S","T","R","I","N","G"? Because that certainly is wrong. It should be exactly an String[] of length one, containing "String" as sole element.

Yes Jochen, that is what I meant - that "String" is reaching foo(String...s) as an array of six 1-character strings

because to get rid of ClassCastException, the "asType" coercion is happening now and it is effectively ("String" as String[]) that gets passed.

it is reaching that as such array now, right? Because that is something we absolutely need ot fix.

I think the asType coercion is maybe not the right thing. I am sorry that I gave my ok now. Because as I realize now, asType coercion is used for the as operator while the maximum we want to have would be a groovy cast. Neither a cast nor asType are there to handle vargs. Usually the way of doing things here is the following:

pack all arguments in an array, use that to decide what constructor to call, return a new coerced array, that is then unpacked und used for the method call. What happens currently is that we probably only change the arguments of the array. But since we need to handle vargs, we need the ability to return a new array.

So some method in ScriptBytecodeAdapter is needed that does the coercion. Along with that you should probably undo the a change for 4015, because this extended logic here will do what is needed in a much better way and does not falsify the cast with strang asType method calls. In the end the coercion logic for MetaMethods will be dulpicated this way yes. Before Alex this kind of code was in MetaClasselper as static method, but this is no longer available and doing so now is maybe not the best idea too.

It's not a problem - I will undo the 4015 fix. Good that 4015 and 4016 are reported at the same time. Better to catch this issue sooner than later.

So, just to recapture what you have suggested:

1) Undo the 4015 fix and from "doConvertAndCast" go back to "doCast" logic.

2) Introduce a new doCoerceAndCast method in SBA that does MetaMethod like handling of varargs.

3) Switch from "doCast" to this new "doCoerceAndCast" in ASM#visitSpecialConstructorCall()

Atm we have selectConstructorAndTransformArguments in ScriptBytecodeAdapter. Looking further into the code, the returned int indicates also if the method was vargs (MetaClassImpl). I think in that case the array is supposed to store the new array as first argument and unrolling of the internal array has to happen. This part exists in the design, it is just not implemented yet.. Using the original or a new array to store the coerced arguments is also no big deal.

So I think no new method is needed, just filling the gaps is.

I will need to see how the returned int tells that a var args method was found, new array, unrolling, etc. Most of the last comment just bounced off me

I will go through SBA#selectConstructorAndTransformArguments() and ASM#visitSpecialConstructorCall() again a bit later to try to understand your last comment better.

If no new method is required, I won't add one - I promise

The important method is in MetaClassImpl:

    public int selectConstructorAndTransformArguments(int numberOfConstructors, Object[] arguments) {
        //TODO: that is just a quick prototype, not the real thing!
        if (numberOfConstructors != constructors.size()) {
            throw new IncompatibleClassChangeError("the number of constructors during runtime and compile time for " +
                    this.theClass.getName() + " do not match. Expected " + numberOfConstructors + " but got " + constructors.size());
        }

        if (arguments == null) arguments = EMPTY_ARGUMENTS;
        Class[] argClasses = MetaClassHelper.convertToTypeArray(arguments);
        MetaClassHelper.unwrap(arguments);
        CachedConstructor constructor = (CachedConstructor) chooseMethod("<init>", constructors, argClasses);
        if (constructor == null) {
            constructor = (CachedConstructor) chooseMethod("<init>", constructors, argClasses);
        }
        if (constructor == null) {
            throw new GroovyRuntimeException(
                    "Could not find matching constructor for: "
                            + theClass.getName()
                            + "(" + InvokerHelper.toTypeString(arguments) + ")");
        }
        List l = new ArrayList(constructors.toList());
        Comparator comp = new Comparator() {
            public int compare(Object arg0, Object arg1) {
                CachedConstructor c0 = (CachedConstructor) arg0;
                CachedConstructor c1 = (CachedConstructor) arg1;
                String descriptor0 = BytecodeHelper.getMethodDescriptor(Void.TYPE, c0.getNativeParameterTypes());
                String descriptor1 = BytecodeHelper.getMethodDescriptor(Void.TYPE, c1.getNativeParameterTypes());
                return descriptor0.compareTo(descriptor1);
            }
        };
        Collections.sort(l, comp);
        int found = -1;
        for (int i = 0; i < l.size(); i++) {
            if (l.get(i) != constructor) continue;
            found = i;
            break;
        }
        // NOTE: must be changed to "1 |" if constructor was vargs
        return 0 | (found << 8);
    }

If you take the last line, then you see that found is shifted by 8 and if you look into the bytecode then you will see that there is a backshift. So if the first bit is set, then varargs transformation had been applied. So in bytecode you need to first do x&1!=0 to see if the vargs transform had been used (xbeing the return value of the method). If it is 0 then the current logic applies, if not, then we can for example (not yet implemented!) store a new array in element 0, which contain all arguments transformed for the vargs. I would suggest that in general, but for backwarts compatibility it is better to use these two ways. Also it is not very complicated, since the unroling logic can be reused, just the used array is different.

I will spend more time on that, but that is one difficult method to understand.

• What purpose do 2 consecutive calls to chooseMethod("<init>", constructors, argClasses) serve?

• If there were multiple constructors defined, how does the index of the constructor (that is then used in "0 | (found << 8)") indicate whether the constructor uses varargs or not.
• Or, is "// NOTE: must be changed to "1 |" if constructor was vargs" really saying that varargs handling in the constructor selection is un-implemented in MC#selectConstructorAndTransformArguments() as well, and when implemented, the return value should be changed to "1 | (found << 8)"? If that is the case, then varargs handling will have to be implemented both in MC#selectConstructorAndTransformArguments() as well as in ASM#visitSpecialConstructorCall()

Oh.... that is avery good question... I have absolutely not idea. Doesn't look right to me.The return value does not yet indicate if a varargs transform happened. This needs implementation. And yes, in that case 1 | (found << 8) should be done... of course the argument array needs to bechanged before.

ASM#visitSpecialConstructorCall() needs to provide code to recognize that bit being set and unroll a different array then. But that varargs handling is different from what is to be done in MC#selectConstructorAndTransformArguments(). Because this code needs to preapre the array only.

Jochen, I am a bit skeptical about making changes without clearly understanding exactly what is happening in MC#selectConstructorAndTransformArguments() and why. I don't want to break more than I make there. There are too many small-small bits I don't understand in that method - like the sorting based on type descriptors, role of index of constructor chosen, etc. If I don't clearly get the current picture of it, I will give this issue a pass most probably.

In that case, do you think it will be better to undo 4015 and go back to ClassCastExceptions? Or, can that fix be kept so that at least we are half-way there in supporting this()/super() with non-matching argument/parameter types? It's only in corner cases like var arg usage here that behavior may be buggy until 4016 gets solved properly.

If you think it will be better to go pre-4015 state, I can just undo that for now.

I think it is probably best to undo 4015 and make it a duplicate of this one

Sounds good. I will do that.

Jochen, I have undone the fix of 4015 and marked that as duplicate of this one.

When this issue is fixed, it must be ensured that GROOVY-4015 scenario is handled as well. Probable the testcase can be reused later from MethodSelectionTest.groovy's history.

Sorry about a little mess that I have created here, but I had no idea about 4016 when I was working on 4015

this issues seems to be fixed since 1.8.0