XBEAN-355 - XbeanAsmParameterNameLoader incorrectly maps parameter names when methods contain long or double parameters

xbean-reflect/src/main/java/org/apache/xbean/recipe/XbeanAsmParameterNameLoader.java

@@ -224,9 +224,11 @@ private static class AllParameterNamesDiscoveringVisitor extends ClassVisitor {
         private final String methodName;
         private final Map<String,Method> methodMap = new HashMap<String,Method>();
         private final Map<String,Constructor> constructorMap = new HashMap<String,Constructor>();
+        private final Class<?> clazz;
 
         public AllParameterNamesDiscoveringVisitor(Class type, String methodName) {
             super(ASM_VERSION);
+            this.clazz = type;
             this.methodName = methodName;
 
             List<Method> methods = new ArrayList<Method>(Arrays.asList(type.getMethods()));
@@ -240,6 +242,7 @@ public AllParameterNamesDiscoveringVisitor(Class type, String methodName) {
 
         public AllParameterNamesDiscoveringVisitor(Class type) {
             super(ASM_VERSION);
+            this.clazz = type;
             this.methodName = "<init>";
 
             List<Constructor> constructors = new ArrayList<Constructor>(Arrays.asList(type.getConstructors()));
@@ -265,6 +268,7 @@ public Map<String,Exception> getExceptions() {
             return exceptions;
         }
 
+        @Override
         public MethodVisitor visitMethod(int access, String name, String desc, String signature, String[] exceptions) {
             if (!name.equals(this.methodName)) {
                 return null;
@@ -273,51 +277,86 @@ public MethodVisitor visitMethod(int access, String name, String desc, String si
             try {
                 final List<String> parameterNames;
                 final boolean isStaticMethod;
-
+                final Type[] paramTypes;
                 final int paramLen;
+
+                // Determine if this is a non-static inner class constructor
+                boolean isNonStaticInner = methodName.equals("<init>") &&
+                        clazz.getEnclosingClass() != null &&
+                        !Modifier.isStatic(clazz.getModifiers());
+
                 if (methodName.equals("<init>")) {
                     Constructor constructor = constructorMap.get(desc);
                     if (constructor == null) {
                         return null;
                     }
 
                     paramLen = constructor.getParameterTypes().length;
-                    parameterNames = new ArrayList<String>(paramLen);
-                    parameterNames.addAll(Collections.<String>nCopies(paramLen, null));
+                    parameterNames = new ArrayList<>(paramLen);
+                    parameterNames.addAll(Collections.nCopies(paramLen, null));
                     constructorParameters.put(constructor, parameterNames);
                     isStaticMethod = false;
+
+                    // Get ASM Type array for slot computation
+                    paramTypes = new Type[paramLen];
+                    Class<?>[] types = constructor.getParameterTypes();
+                    for (int i = 0; i < paramLen; i++) {
+                        paramTypes[i] = Type.getType(types[i]);
+                    }
+
                 } else {
                     Method method = methodMap.get(desc);
                     if (method == null) {
                         return null;
                     }
 
                     paramLen = method.getParameterTypes().length;
-                    parameterNames = new ArrayList<String>(paramLen);
-                    parameterNames.addAll(Collections.<String>nCopies(paramLen, null));
+                    parameterNames = new ArrayList<>(paramLen);
+                    parameterNames.addAll(Collections.nCopies(paramLen, null));
                     methodParameters.put(method, parameterNames);
                     isStaticMethod = Modifier.isStatic(method.getModifiers());
+
+                    // Get ASM Type array for slot computation
+                    Class<?>[] types = method.getParameterTypes();
+                    paramTypes = new Type[types.length];
+                    for (int i = 0; i < types.length; i++) {
+                        paramTypes[i] = Type.getType(types[i]);
+                    }
+                }
+
+                // Determine the starting parameter index (skip synthetic outer-class param if inner class)
+                final int paramOffset = (methodName.equals("<init>") && isNonStaticInner) ? 1 : 0;
+
+                // Build slot -> parameter index map
+                final Map<Integer, Integer> slotToParamIndex = new HashMap<>();
+                int slot = isStaticMethod ? 0 : 1; // slot 0 reserved for "this" in non-static
+                for (int i = 0; i < paramLen; i++) {
+                    if (i < paramOffset) {
+                        // skip synthetic outer-class parameter
+                        slot += paramTypes[i].getSize();
+                        continue;
+                    }
+                    slotToParamIndex.put(slot, i);
+                    slot += paramTypes[i].getSize(); // 1 for normal, 2 for long/double
                 }
 
+                // Return visitor that assigns names correctly
                 return new MethodVisitor(ASM_VERSION) {
-                    // assume static method until we get a first parameter name
+                    @Override
                     public void visitLocalVariable(String name, String description, String signature, Label start, Label end, int index) {
-                        if (isStaticMethod) {
-                            if (paramLen > index) {
-                                parameterNames.set(index, name);
-                            }
-                        } else if (index > 0) {
-                            // for non-static the 0th arg is "this" so we need to offset by -1
-                            if (paramLen >= index) {
-                                parameterNames.set(index - 1, name);
-                            }
+                        final Integer paramIndex = slotToParamIndex.get(index);
+                        if (paramIndex != null) {
+                            parameterNames.set(paramIndex, name);
                         }
                     }
                 };
+
             } catch (Exception e) {
                 this.exceptions.put(signature, e);
             }
+
             return null;
         }
+
     }
 }

xbean-reflect/src/test/java/org/apache/xbean/recipe/MultiArgCtor.java

@@ -0,0 +1,25 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing, software
+ *  distributed under the License is distributed on an "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *  See the License for the specific language governing permissions and
+ *  limitations under the License.
+ */
+package org.apache.xbean.recipe;
+
+public class MultiArgCtor {
+
+    @SuppressWarnings({"UnusedDeclaration"})
+    public MultiArgCtor(String svmType, String kernelType, int degree, double gamma, double coef0, double nu,
+                     double cost, double eps, double p, double cacheSize, boolean shrinking,
+                     boolean probability, boolean crossValidation, int nFold, String modelName, boolean debug) {}
+}

xbean-reflect/src/test/java/org/apache/xbean/recipe/ParameterNameLoaderTest.java

@@ -25,6 +25,7 @@
 import java.util.List;
 import java.util.Map;
 
+import junit.framework.Test;
 import junit.framework.TestCase;
 
 /**
@@ -107,6 +108,39 @@ public void testEmptyMethod() throws Exception {
         assertParameterNames(Collections.<String>emptyList(), method);
     }
 
+    public void testXBEAN355() throws Exception{
+      Constructor constructor =  TestClass.class.getConstructor(String.class, String.class, int.class, double.class, double.class, double.class,
+                double.class, double.class, double.class, double.class, boolean.class, boolean.class, boolean.class, int.class,
+                String.class, boolean.class);
+        assertParameterNames(Arrays.asList("svmType", "kernelType", "degree", "gamma", "coef0", "nu",
+                "cost", "eps", "p", "cacheSize", "shrinking", "probability", "crossValidation", "nFold", "modelName", "debug"), constructor);
+    }
+
+    public void testXBEAN355_2() throws Exception{
+        Constructor constructor =  MultiArgCtor.class.getConstructor(String.class, String.class, int.class, double.class, double.class, double.class,
+                double.class, double.class, double.class, double.class, boolean.class, boolean.class, boolean.class, int.class,
+                String.class, boolean.class);
+        assertParameterNames(Arrays.asList("svmType", "kernelType", "degree", "gamma", "coef0", "nu",
+                "cost", "eps", "p", "cacheSize", "shrinking", "probability", "crossValidation", "nFold", "modelName", "debug"), constructor);
+    }
+
+    public void testXBEAN355_3() throws Exception{
+        Constructor constructor =  NonStaticInnerTestClass.class.getConstructor(ParameterNameLoaderTest.class, String.class, String.class, int.class, double.class, double.class, double.class,
+                double.class, double.class, double.class, double.class, boolean.class, boolean.class, boolean.class, int.class,
+                String.class, boolean.class);
+        assertParameterNames(Arrays.asList(null, "svmType", "kernelType", "degree", "gamma", "coef0", "nu",
+                "cost", "eps", "p", "cacheSize", "shrinking", "probability", "crossValidation", "nFold", "modelName", "debug"), constructor);
+    }
+
+    @SuppressWarnings({"UnusedDeclaration"})
+    private abstract class NonStaticInnerTestClass extends ParentTestClass {
+
+        public NonStaticInnerTestClass(String svmType, String kernelType, int degree, double gamma, double coef0, double nu,
+                         double cost, double eps, double p, double cacheSize, boolean shrinking,
+                         boolean probability, boolean crossValidation, int nFold, String modelName, boolean debug) {}
+
+    }
+
     @SuppressWarnings({"UnusedDeclaration"})
     private static class ParentTestClass {
         public void inheritedMethod(Map nothing) {}
@@ -119,6 +153,9 @@ public TestClass(int foo) {}
         public TestClass(Object bar) {}
         public TestClass(Object[] objectArray) {}
         private TestClass(Double scotch) {}
+        public TestClass(String svmType, String kernelType, int degree, double gamma, double coef0, double nu,
+                         double cost, double eps, double p, double cacheSize, boolean shrinking,
+                         boolean probability, boolean crossValidation, int nFold, String modelName, boolean debug) {}
 
         public static void factoryMethod(int a, Object b, Long c) {}
         public static void factoryMethod(int beer) {}