Inconsistent behavior for new array of Number with numeric initializers

[BladeWise]

Numeric types are not handled consistently when trying to initialize arrays.

In general, trying to initialize a Number[] fails with both primitive and reference numeric types, but with different errors.

Below a couple of test to reproduce the issue:

    @Test
    public void newNumberArrayWithPrimitiveNumeric() {
        final Number[] array = new Number[] { 3, (Double) 0.9, (Number) 1.12f };

        final Expression newArray =
            Expression.newArrayInit(
                Types.Number,
                Expression.constant(3, PrimitiveTypes.Integer),
                Expression.constant(0.9, Types.Double),
                Expression.constant(1.12f, Types.Number)
            );
        final LambdaExpression<Supplier<Number[]>> lambda = Expression.lambda(Type.of(Supplier.class).makeGenericType(Types.Number.makeArrayType()), newArray);
        final Supplier<Number[]> delegate = lambda.compile();
        assertEquals(array[2], delegate.get()[2]);
    }

    @Test
    public void newNumberArrayWithoutPrimitiveNumeric() {
        final Number[] array = new Number[] { (Integer) 3, (Double) 0.9, (Number) 1.12f };

        final Expression newArray =
            Expression.newArrayInit(
                Types.Number,
                Expression.constant((Integer)3, Types.Integer),
                Expression.constant(0.9, Types.Double),
                Expression.constant(1.12f, Types.Number)
            );
        final LambdaExpression<Supplier<Number[]>> lambda = Expression.lambda(Type.of(Supplier.class).makeGenericType(Types.Number.makeArrayType()), newArray);
        final Supplier<Number[]> delegate = lambda.compile();
        assertEquals(array[2], delegate.get()[2]);
    }

Attached the full test class.

Notice that both initializations (with or without primitive types) are legal in Java, but in Procyon

• for primitive types, the Expression throws an exception (i.e. "An expression of type 'int' cannot be used to initialize an array of type 'java.lang.Number'", even if it is a superclass of the boxed value)
• for reference types, the Expression accepts the values, but an assertion in the CodeGenerator fails (actually, complaining about emitting a constant for the values provided as initializer).

The tests show that using an Object[] results in the same behavior, while assignment between Number/Object and numeric primitives/references works fine.

[BladeWise]

Upon further investigation, I was able to make the code work applying the following changes to the code base:

• Comment out this assert
  

  procyon/Procyon.Expressions/src/main/java/com/strobel/expressions/BoundConstants.java

  Lines 65 to 67 in fec0d7a

  	void emitConstant(final LambdaCompiler lc, final Object value, final Type<?> type) {
  	assert !CodeGenerator.canEmitConstant(value, type)
  	: "!CodeGenerator.canEmitConstant(value, type)";

  
  which does not take into account a conversion from reference numeric (e.g. Double) to the Number superclass, in other words the calling code
  

  procyon/Procyon.Expressions/src/main/java/com/strobel/expressions/LambdaCompiler.java

  Lines 1997 to 2026 in fec0d7a

  	private void emitConstant(final Object value, final Type<?> type) {
  	// Try to emit the constant directly into IL
  	if (CodeGenerator.canEmitConstant(value, type)) {
  	final boolean isBoxed = TypeUtils.isAutoUnboxed(type);
  	final boolean isClosureAvailable = canEmitBoundConstants();
  	if (!isBoxed || !isClosureAvailable) {
  	generator.emitConstant(value, type);
  	//
  	// NOTE: For backward compatibility, allow boxed values to be emitted as constants when
  	// a closure is not available. Previously, we allowed, e.g., a Long to be stored
  	// as a long constant, but we didn't emit any boxing upon loading it. This caused
  	// problems when the constant was being used as an Object, e.g., as a method
  	// invocation target.
  	//
  	// To prevent code that previously worked from breaking, allow the unboxed value
  	// to be emitted for a boxed ConstantExpression when no alternative is available,
  	// but emit the boxing operation immediately afterward to avoid the problem above.
  	//
  	if (isBoxed && value != null) {
  	generator.emitBox(type);
  	}
  	return;
  	}
  	}
  	_boundConstants.emitConstant(this, value, type);
  	}

  
  performs a check which is not equivalent to the assertion (i.e. for Double to Number conversion, both isBoxed and isClosureAvailable are true, so generator.emitConstant is not called.

• Modify the conversion check in
  

  procyon/Procyon.Reflection/src/main/java/com/strobel/reflection/emit/CodeGenerator.java

  Lines 1799 to 1805 in fec0d7a

  	if (sourceType.isInterface() || // interface cast
  	targetType.isInterface() ||
  	sourceType == Types.Object || // boxing cast
  	targetType == Types.Object) {
  	emitCastToType(sourceType, targetType);
  	}

  
  in

        if (sourceType.isInterface() ||   // interface cast
            targetType.isInterface() ||
            sourceType == Types.Object ||   // boxing cast
            targetType == Types.Object ||
            (!targetType.isPrimitive() && !sourceType.isPrimitive() && TypeUtils.isNumeric(sourceType) && targetType.isAssignableFrom(sourceType))) { // super cast (e.g. Double -> Number)

            emitCastToType(sourceType, targetType);
        }

to ensure that a cast is emitted for a numeric type to a superclass of the boxed reference.

• Relax the validation in newArrayInit
  

  procyon/Procyon.Expressions/src/main/java/com/strobel/expressions/Expression.java

  Lines 434 to 436 in fec0d7a

  	if (!TypeUtils.areReferenceAssignable(elementType, item.getType())) {
  	throw Error.expressionTypeCannotInitializeArrayType(item.getType(), elementType);
  	}

  
  to allow primitives with a matching boxed type

            if (!TypeUtils.areReferenceAssignable(elementType, TypeUtils.getBoxedTypeOrSelf(item.getType()))) {
                throw Error.expressionTypeCannotInitializeArrayType(item.getType(), elementType);
            }

With these modifications, the above tests complete (and no regression appears in existing ones, as far as I could see).

[mstrobel]

Will look into this in greater detail later, but this did catch my eye:

I don't think constant(1.12f, Types.Number) should be considered valid, and it's not really what the equivalent Java code does; it really ought to be convert(constant(1.12f), Types.Number), where the conversion is either implicit or explicit.

By the way, you've been submitting a lot of bug reports related to procyon-expressions (which is great!). May I ask how you've been using it? I get very little info about who's using the various Procyon libraries apart from the number of downloads on my decompiler.

[BladeWise]

To be honest, I think that an implicit conversion between primitive and Number is a bit of a stretch too, but it was quite handy!
On the other side, reference types to Number seem legit.

In my case, I am using Procyon to implement some sort of configurable processing framework.
Basically, I use a Java/C#-like scripting language to declare a processing pipeline, parse the configuration in an expression tree, and then let Procyon generate code at runtime.

I could have used a full-fledged scripting engine (or maybe Groovy), but this way I can implement some handy features to simplify/shorten the configuration or use language features not available in Java (my main background is C#, so conditional access operator, coalesce operator and extension methods are something I miss dearly).

[mstrobel]

Cool project! You may want to go ahead and check out my v0.6-staging branch, as it makes some changes to TypeBuilder and LambdaCompiler to let them work on JDKs newer than Java 8. Notably:

1. The name passed to a TypeBuilder is stripped of any package name, and only the class name is used.
2. The package is now provided via a MethodHandles.lookup(). If you don't provide one, it will search the base class and then the caller class for a lookup in a static field named PACKAGE_ACCESS or packageAccess. If it doesn't find one, it falls back to using a package called generated.
3. Lambdas basically follow the same rules, except to specify a lookup directly, you need to push an ExpressionContext.

These are, unfortunately, necessary, as the method I used to define classes dynamically has been deprecated and removed. As far as I can tell, you can no longer declare new classes in an arbitrary package (unless you resort to custom class loaders).

[BladeWise]

I had a look at that, and it seems really good!
To be honest, I had already implemented a solution to port Procyon to JDK 11 using method handles (is a quick & dirty variation of what you do, and requires to provide an explicit lookup resolver), and was waiting for 0.6 release to migrate to your solution.

I have a fork here on GitHub, were I ported the project to Grade 7 (and was about to update JUnit to version 5), but I could not check signing. If you are interested, I can make a WIP PR to let you review and check it.