[Bug]: spoon.support.reflect.declaration.CtTypeImpl.hasSameParameters(...) lacks an if condition

[CoreRasurae]

Describe the bug

spoon.support.reflect.declaration.CtTypeImpl.hasSameParameters(CtExecutable<?> candidate, CtTypeReference<?>... parameterTypes) lacks an if condition without which a comparison between a non-array type parameter and a candidate executable with an array parameter can take place, leading to a wrong return value.

Source code you are trying to analyze/transform

No response

Source code for your Spoon processing

protected boolean hasSameParameters(CtExecutable<?> candidate, CtTypeReference<?>... parameterTypes) {
		if (candidate.getParameters().size() != parameterTypes.length) {
			return false;
		}
		for (int i = 0; (i < candidate.getParameters().size()) && (i < parameterTypes.length); i++) {
			final CtTypeReference<?> ctParameterType = candidate.getParameters().get(i).getType();
			final CtTypeReference<?> parameterType = parameterTypes[i];
			if (parameterType instanceof CtArrayTypeReference) {
				if (ctParameterType instanceof CtArrayTypeReference) {
					if (!isSameParameter(candidate, ((CtArrayTypeReference) ctParameterType).getComponentType(), ((CtArrayTypeReference) parameterType).getComponentType())) {
						return false;
					} else {
						if (((CtArrayTypeReference) ctParameterType).getDimensionCount() != ((CtArrayTypeReference) parameterType).getDimensionCount()) {
							return false;
						}
					}
				} else {
					return false;
				}
                        <-- Condition is lacking here, if ctParameterType is an Array Type Ref.
			} else if (!isSameParameter(candidate, ctParameterType, parameterType)) {
				return false;
			}
		}
		return true;
	}

Actual output

No response

Expected output

protected boolean hasSameParameters(
            CtExecutable<?> candidate, CtTypeReference<?>... parameterTypes) {
        if (candidate.getParameters().size() != parameterTypes.length) {
            return false;
        }
        for (int i = 0; (i < candidate.getParameters().size()) && (i < parameterTypes.length); 
                i++) {
            final CtTypeReference<?> ctParameterType = 
                    candidate.getParameters().get(i).getType();
            final CtTypeReference<?> parameterType = parameterTypes[i];
            if (parameterType instanceof CtArrayTypeReference) {
                if (ctParameterType instanceof CtArrayTypeReference) {
                    if (!isSameParameter(candidate, 
                            ((CtArrayTypeReference) ctParameterType).getComponentType(), 
                            ((CtArrayTypeReference) parameterType).getComponentType())) {
                        return false;
                    } else {
                        if (((CtArrayTypeReference) ctParameterType).getDimensionCount() 
                                != ((CtArrayTypeReference) parameterType).getDimensionCount()) {
                            return false;
                        }
                    }
                } else {
                    return false;
                }
            //The following else if condition must be appended to fix the logic
            } else if (ctParameterType instanceof CtArrayTypeReference) {
                return false;
            } else if (!isSameParameter(candidate, ctParameterType, parameterType)) {
                return false;
            }
        }
        return true;
    }

Spoon Version

11.0.1

JVM Version

Not a JVM version issue

What operating system are you using?

NixOS Vicuna, but not relevant for this bug

[I-Al-Istannen]

I don't understand the problem. If you pass in a non-array parameter the type erasure will differ (Foo[] vs Foo) and the isSame check will fail. Can you provide a failing example? i.e. some source code using spoon where this produces wrong results?

[CoreRasurae]

You are correct, my unit tests were partially flawed. It is not a bug, however could be seen as an optimization. With proper tests isSameParameter() does distinguish between array and non-array, however this still may be an interesting change in that this will save unneeded typeErasure(...), qualified name generation and String compare operations. How should i proceed in this case?

[I-Al-Istannen]

Is this actually relevant in any program you found? If not, I would likely only make that change if it improves code clarity or does not impact it at all. It is not really worth it to trade complexity for performance without knowing whether it matters.

If I parse your diff correctly, the change is probably small enough that it could be okay, if you add an explanatory comment.

You would need to open a PR for that.