Skip to content

Commit d2084ac

Browse files
gselzergselzer
committed
Improve Identity Conversions
As a byproduct, ambiguous type variables are now mapped to the narrowest possible mapping, not the broadest, and are widened, not narrowed.
1 parent 87cf0ff commit d2084ac

File tree

6 files changed

+63
-44
lines changed

6 files changed

+63
-44
lines changed

‎scijava-ops-engine/src/main/java/org/scijava/ops/engine/matcher/convert/Conversions.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -450,14 +450,15 @@ private static Nil<?> wildcardVacuousTypeVars(final Type t) {
450450
Type[] typeParams = Types.typeParamsAgainstClass(t, Types.raw(t));
451451
if (t instanceof TypeVariable<?>) {
452452
TypeVariable<?> tv = (TypeVariable<?>) t;
453+
// Create an Any with the type variable bounds
453454
return Nil.of(new Any(tv.getBounds()));
454455
}
455456
var vars = new HashMap<TypeVariable<?>, Type>();
456457
for (Type typeParam : typeParams) {
457458
if (typeParam instanceof TypeVariable<?>) {
458459
// Get the type variable
459460
TypeVariable<?> from = (TypeVariable<?>) typeParam;
460-
// Create a wildcard type with the type variable bounds
461+
// Create an Any with the type variable bounds
461462
Type to = new Any(from.getBounds());
462463
vars.put(from, to);
463464
}

‎scijava-ops-engine/src/main/java/org/scijava/ops/engine/matcher/convert/IdentityCollection.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -40,12 +40,12 @@
4040
import java.util.function.Function;
4141

4242
/**
43-
* An {@link OpCollection} containing {@code identity} Ops.
43+
* An {@link OpCollection} containing {@code engine.identity} Ops.
4444
*
4545
* @author Gabriel Selzer
4646
* @param <T>
4747
*/
48-
public class IdentityCollection<T> implements OpCollection {
48+
public class IdentityCollection<T, U extends T> implements OpCollection {
4949

5050
/**
5151
* @input t the object to be converted
@@ -55,13 +55,13 @@ public class IdentityCollection<T> implements OpCollection {
5555
@OpHints(hints = { Conversion.FORBIDDEN,
5656
BaseOpHints.DependencyMatching.FORBIDDEN })
5757
@OpField(names = "engine.convert, engine.identity", priority = Priority.FIRST)
58-
public final Function<T, T> identity = (t) -> t;
58+
public final Function<U, T> identity = t -> t;
5959

6060
/**
6161
* @mutable t the object to be "mutated"
6262
*/
6363
@OpHints(hints = { Conversion.FORBIDDEN })
6464
@OpField(names = "engine.identity", priority = Priority.FIRST)
65-
public final Inplaces.Arity1<T> inplace = (t) -> {};
65+
public final Inplaces.Arity1<T> inplace = t -> {};
6666

6767
}

‎scijava-ops-engine/src/test/java/org/scijava/ops/engine/matcher/convert/ConversionTest.java‎

Lines changed: 43 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -29,13 +29,6 @@
2929

3030
package org.scijava.ops.engine.matcher.convert;
3131

32-
import static org.junit.jupiter.api.Assertions.assertEquals;
33-
34-
import java.lang.reflect.Type;
35-
import java.lang.reflect.TypeVariable;
36-
import java.util.function.BiFunction;
37-
import java.util.function.Function;
38-
3932
import org.junit.jupiter.api.Assertions;
4033
import org.junit.jupiter.api.BeforeAll;
4134
import org.junit.jupiter.api.Test;
@@ -53,6 +46,13 @@
5346
import org.scijava.ops.spi.OpField;
5447
import org.scijava.ops.spi.OpMethod;
5548

49+
import java.lang.reflect.Type;
50+
import java.lang.reflect.TypeVariable;
51+
import java.util.function.BiFunction;
52+
import java.util.function.Function;
53+
54+
import static org.junit.jupiter.api.Assertions.assertEquals;
55+
5656
/**
5757
* Basic Op conversion test
5858
*
@@ -145,18 +145,18 @@ public void testConvertedOpInfo() {
145145
Type[] expected = { Long[].class, Byte[].class };
146146
Assertions.assertArrayEquals(expected, info.inputTypes().toArray());
147147
// Assert the output type is expected
148-
Assertions.assertEquals(Byte[].class, info.outputType());
148+
assertEquals(Byte[].class, info.outputType());
149149

150150
// Assert the struct types are correct
151151
var members = info.struct().members();
152-
Assertions.assertEquals(Long[].class, members.get(0).getType());
153-
Assertions.assertEquals(Byte[].class, members.get(1).getType());
152+
assertEquals(Long[].class, members.get(0).getType());
153+
assertEquals(Byte[].class, members.get(1).getType());
154154

155155
// Assert the implementation name reflects the new parameter types
156156
var expImplName = "org.scijava.ops.engine.matcher.convert.ConversionTest" +
157157
".foo(java.lang.Integer[],java.lang.Double[])|converted_Long_Arr_Byte_Arr";
158158
var actImplName = info.implementationName();
159-
Assertions.assertEquals(expImplName, actImplName);
159+
assertEquals(expImplName, actImplName);
160160
}
161161

162162
@OpMethod(names = "test.anyConversion", type = Function.class)
@@ -179,7 +179,7 @@ public void testConvertAnys() {
179179
* Op.
180180
* </p>
181181
*/
182-
@OpMethod(names = "test.boundsConversion", type = BiFunction.class)
182+
@OpMethod(names = "test.maxNumberAndComparable", type = BiFunction.class)
183183
public static <T extends Number & Comparable<T>> T foo(T in1, T in2) {
184184
return in1.compareTo(in2) > 0 ? in1 : in2;
185185
}
@@ -191,23 +191,49 @@ public static <T extends Number & Comparable<T>> T foo(T in1, T in2) {
191191
@Test
192192
public void testConvertMultipleBounds() {
193193
// Assert that there's only one possible match for our Op call
194-
var name = "test.boundsConversion";
194+
var name = "test.maxNumberAndComparable";
195195
var infos = ops.infos(name);
196-
Assertions.assertEquals(1, infos.size());
196+
assertEquals(1, infos.size());
197197
// And its input types are TypeVariables with two upper bounds.
198198
var inType = infos.first().inputTypes().get(0);
199199
Assertions.assertInstanceOf(TypeVariable.class, inType);
200200
var numBounds = ((TypeVariable<?>) inType).getBounds().length;
201-
Assertions.assertEquals(2, numBounds);
201+
assertEquals(2, numBounds);
202202
// Now, call it such that we need conversion
203203
Integer i1 = 1;
204204
Double i2 = 2.0;
205-
var result = ops.op("test.boundsConversion") //
205+
var result = ops.op(name) //
206206
.arity2().input(i1, i2).apply();
207207
// Assert the result is an Integer
208208
Assertions.assertInstanceOf(Integer.class, result);
209209
Integer intResult = (Integer) result;
210-
Assertions.assertEquals(2, intResult);
210+
assertEquals(2, intResult);
211+
}
212+
213+
/**
214+
* An Op, written as a method, whose type variable has multiple bounds.
215+
* <p>
216+
* Note that, for the purposes of this test, the {@link Number} bound is
217+
* necessary even though it is not needed for the functionality of the test
218+
* Op.
219+
* </p>
220+
*/
221+
@OpMethod(names = "test.maxNumberIntegerNumber", type = BiFunction.class)
222+
public static Number max(Number in1, Integer in2) {
223+
return in2.compareTo(in1.intValue()) > 0 ? in1 : in2;
224+
}
225+
226+
/**
227+
* Tests that subtypes are "identity-converted"
228+
*/
229+
@Test
230+
public void testIdentityConversion() {
231+
// Conversion is required for the second input.
232+
// The first input, then, should be identity-converted
233+
ops.binary("test.maxNumberIntegerNumber") //
234+
.inType(Double.class, Double.class) //
235+
.outType(Number.class) //
236+
.function();
211237
}
212238

213239
}

‎scijava-types/src/main/java/org/scijava/types/inference/GenericAssignability.java‎

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -644,17 +644,11 @@ else if (!Types.isRecursiveBound(type, bound)) {
644644
if (bound instanceof TypeVariable) {
645645
// This bound might be seen if you write something like
646646
// <O extends Number, I extends O> and are trying to infer I from a
647-
// Double
648-
// The O would be seen here. Right now, we want the O to be assigned
649-
// to the broadest
650-
// possible type for later assignment
647+
// Double, the O would be seen here. It is important that the O
648+
// be malleable, as it is not yet fixed, and could be changed to
649+
// another type later
651650
TypeVariable<?> tv = (TypeVariable<?>) bound;
652-
if (tv.getBounds().length == 1) {
653-
var b = tv.getBounds()[0];
654-
var map = GenericAssignability.inferTypeVariables(b, inferFrom);
655-
var mapped = Types.mapVarToTypes(b, map);
656-
inferTypeVariables(tv, mapped, typeMappings, true);
657-
}
651+
inferTypeVariables(tv, inferFrom, typeMappings, true);
658652
}
659653
else {
660654
// Else go into recursion as we encountered a new var.

‎scijava-types/src/main/java/org/scijava/types/inference/TypeMapping.java‎

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -75,28 +75,22 @@ public TypeMapping(TypeVariable<?> typeVar, Type mappedType,
7575
* {@code typeVar} and {@code mappedType} <em>given</em> the existing
7676
* malleability of {@code mappedType} and the malleability imposed by
7777
* {@code newType}. If {@code newType} cannot be accommodated, a
78-
* {@link TypeInferenceException} will be thrown. Note that it is not a
79-
* guarantee that either the existing {@code mappedType} or {@code newType}
80-
* will become the new {@link #mappedType} after the method ends;
81-
* {@link #mappedType} could be a supertype of these two {@link Type}s.
78+
* {@link TypeInferenceException} will be thrown.
8279
*
8380
* @param otherType - the type that will be refined into {@link #mappedType}
8481
* @param newTypeMalleability - the malleability of {@code otherType},
8582
* determined by the context from which {@code otherType} came.
8683
*/
8784
public void refine(Type otherType, boolean newTypeMalleability) {
88-
malleable &= newTypeMalleability;
8985
if (Any.is(mappedType)) {
9086
mappedType = otherType;
9187
return;
9288
}
9389
if (Any.is(otherType)) {
9490
return;
9591
}
96-
if (mappedType.equals(otherType)) {
97-
return;
98-
}
9992
if (otherType instanceof WildcardType) {
93+
malleable &= newTypeMalleability;
10094
WildcardType wType = (WildcardType) otherType;
10195
if (wType.getLowerBounds().length == 0 && //
10296
wType.getUpperBounds().length == 1 && //
@@ -105,11 +99,15 @@ public void refine(Type otherType, boolean newTypeMalleability) {
10599
return;
106100
}
107101
}
108-
if (malleable && Types.isAssignable(otherType, mappedType)) {
102+
if (malleable && Types.isAssignable(mappedType, otherType)) {
103+
malleable &= newTypeMalleability;
109104
mappedType = otherType;
110105
return;
111106
}
112-
if (Objects.equal(mappedType, otherType)) return;
107+
if (Objects.equal(mappedType, otherType)) {
108+
malleable &= newTypeMalleability;
109+
return;
110+
}
113111
throw new TypeInferenceException(typeVar +
114112
" cannot simultaneously be mapped to " + otherType + " and " +
115113
mappedType);

‎scijava-types/src/test/java/org/scijava/types/inference/InferTypeVariablesTest.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -309,12 +309,12 @@ public <T extends Number> void testInferSuperWildcard()
309309
Map<TypeVariable<?>, TypeMapping> typeAssigns = new HashMap<>();
310310
GenericAssignability.inferTypeVariables(type, inferFrom, typeAssigns);
311311

312-
// We expect I= Double, O = Number
312+
// We expect I= Double, O = Double
313313
Map<TypeVariable<?>, TypeMapping> expected = new HashMap<>();
314314
TypeVariable<?> typeVarI = (TypeVariable<?>) new Nil<I>() {}.getType();
315315
expected.put(typeVarI, new TypeMapping(typeVarI, Double.class, false));
316316
TypeVariable<?> typeVarO = (TypeVariable<?>) new Nil<O>() {}.getType();
317-
expected.put(typeVarO, new TypeMapping(typeVarO, Number.class, true));
317+
expected.put(typeVarO, new TypeMapping(typeVarO, Double.class, true));
318318

319319
Assertions.assertEquals(expected, typeAssigns);
320320
}

0 commit comments

Comments
 (0)