Skip to content

Use Anys, not wildcards, for converter output types #139

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 1, 2024
Merged

Use Anys, not wildcards, for converter output types #139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions ...ngine/src/main/java/org/scijava/ops/engine/matcher/convert/ConversionMatchingRoutine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,17 +77,11 @@ public OpCandidate findMatch(MatchingConditions conditions, OpMatcher matcher,
.hints()))
{
Conversions.tryConvert(env, info, request).ifPresent(converted -> {
Map<TypeVariable<?>, Type> map = new HashMap<>();
GenericAssignability.inferTypeVariables( //
new Type[] { converted.opType() }, //
new Type[] { request.getType() }, //
map //
);
candidates.add(new OpCandidate( //
env, //
request, //
converted, //
map //
converted.typeVarAssigns() //
));
});
}
Expand Down
23 changes: 16 additions & 7 deletions scijava-ops-engine/src/main/java/org/scijava/ops/engine/matcher/convert/Conversions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import org.scijava.function.Mutable;
import org.scijava.ops.api.*;
import org.scijava.ops.engine.BaseOpHints;
import org.scijava.types.Any;
import org.scijava.types.Nil;
import org.scijava.types.Types;
import org.scijava.types.inference.FunctionalInterfaces;
Expand Down Expand Up @@ -136,7 +137,7 @@ private static ConvertedOpInfo convert(OpEnvironment env, OpInfo info,
return opt.get();
}
// Attempt 2: Computer with identity mutable output
opt = postprocessIdentity(info, request, preConverters, env);
opt = postprocessIdentity(info, request, preConverters, vars, env);
if (opt.isPresent()) {
return opt.get();
}
Expand Down Expand Up @@ -209,7 +210,8 @@ private static Optional<ConvertedOpInfo> postprocessFunction(OpInfo info,
postConverter, //
request.getOutType(), //
null, //
env //
env, //
vars //
));
}

Expand All @@ -231,7 +233,7 @@ private static Optional<ConvertedOpInfo> postprocessFunction(OpInfo info,
*/
private static Optional<ConvertedOpInfo> postprocessIdentity(OpInfo info,
OpRequest request, List<RichOp<Function<?, ?>>> preConverters,
OpEnvironment env)
Map<TypeVariable<?>, Type> vars, OpEnvironment env)
{
// This procedure only applies to Ops with mutable outputs
int ioIndex = mutableIndexOf(request.getType());
Expand All @@ -252,7 +254,8 @@ private static Optional<ConvertedOpInfo> postprocessIdentity(OpInfo info,
null, //
request.getOutType(), //
null, //
env //
env, //
vars //
));
}
return Optional.empty();
Expand Down Expand Up @@ -312,7 +315,8 @@ private static Optional<ConvertedOpInfo> postprocessConvertAndCopy(
postConverter, //
request.getOutType(), //
copyOp, //
env //
env, //
vars //
));
}
catch (OpMatchingException e) {
Expand Down Expand Up @@ -375,7 +379,8 @@ private static Optional<ConvertedOpInfo> postprocessCopy(OpInfo info,
postConverter, //
request.getOutType(), //
copyOp, //
env //
env, //
vars //
));
}
catch (OpMatchingException e) {
Expand Down Expand Up @@ -443,13 +448,17 @@ private static void resolveTypes(Type source, Type dest,
*/
private static Nil<?> wildcardVacuousTypeVars(final Type t) {
Type[] typeParams = Types.typeParamsAgainstClass(t, Types.raw(t));
if (t instanceof TypeVariable<?>) {
TypeVariable<?> tv = (TypeVariable<?>) t;
return Nil.of(new Any(tv.getBounds()));
}
var vars = new HashMap<TypeVariable<?>, Type>();
for (Type typeParam : typeParams) {
if (typeParam instanceof TypeVariable<?>) {
// Get the type variable
TypeVariable<?> from = (TypeVariable<?>) typeParam;
// Create a wildcard type with the type variable bounds
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is no longer correct.

Type to = Types.wildcard(from.getBounds(), null);
Type to = new Any(from.getBounds());
vars.put(from, to);
}
}
Expand Down
11 changes: 9 additions & 2 deletions scijava-ops-engine/src/main/java/org/scijava/ops/engine/matcher/convert/ConvertedOpInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ public class ConvertedOpInfo implements OpInfo {

private final OpInfo info;
private final OpEnvironment env;
private final Map<TypeVariable<?>, Type> typeVarAssigns;
final List<RichOp<Function<?, ?>>> preconverters;
final List<Type> inTypes;
final RichOp<Function<?, ?>> postconverter;
Expand All @@ -116,7 +117,8 @@ public ConvertedOpInfo(OpInfo info,
Arrays.asList(inTypes(info.inputTypes(), preconverters)), //
postconverter, //
outType(info.outputType(), postconverter), copyOp, //
env //
env, //
Collections.emptyMap() //
);
}

Expand All @@ -138,7 +140,8 @@ public ConvertedOpInfo( //
RichOp<Function<?, ?>> postconverter, //
Type reqOutput, //
final RichOp<Computers.Arity1<?, ?>> copyOp, //
OpEnvironment env //
OpEnvironment env, //
Map<TypeVariable<?>, Type> typeVarAssigns //
) {
this.info = info;
this.opType = mapAnys(opType, info);
Expand All @@ -153,6 +156,7 @@ public ConvertedOpInfo( //
BaseOpHints.Conversion.FORBIDDEN, //
"converted" //
);
this.typeVarAssigns = typeVarAssigns;
}

/**
Expand Down Expand Up @@ -943,4 +947,7 @@ private static String fMethodPreprocessing(
return sb.toString();
}

public Map<TypeVariable<?>, Type> typeVarAssigns() {
return this.typeVarAssigns;
}
}
4 changes: 2 additions & 2 deletions ...a-ops-engine/src/main/java/org/scijava/ops/engine/matcher/convert/IdentityCollection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
* @author Gabriel Selzer
* @param <T>
*/
public class IdentityCollection<T, U extends T> implements OpCollection {
public class IdentityCollection<T> implements OpCollection {

/**
* @input t the object to be converted
Expand All @@ -55,7 +55,7 @@ public class IdentityCollection<T, U extends T> implements OpCollection {
@OpHints(hints = { Conversion.FORBIDDEN,
BaseOpHints.DependencyMatching.FORBIDDEN })
@OpField(names = "engine.convert, engine.identity", priority = Priority.FIRST)
public final Function<U, T> identity = (t) -> t;
public final Function<T, T> identity = (t) -> t;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parens are unnecessary; you can just write t -> t.


/**
* @mutable t the object to be "mutated"
Expand Down
41 changes: 41 additions & 0 deletions scijava-ops-engine/src/test/java/org/scijava/ops/engine/matcher/convert/ConversionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import static org.junit.jupiter.api.Assertions.assertEquals;

import java.lang.reflect.Type;
import java.lang.reflect.TypeVariable;
import java.util.function.BiFunction;
import java.util.function.Function;

Expand All @@ -40,6 +41,7 @@
import org.junit.jupiter.api.Test;
import org.scijava.function.Computers;
import org.scijava.function.Container;
import org.scijava.ops.api.OpInfo;
import org.scijava.ops.api.Ops;
import org.scijava.ops.engine.AbstractTestEnvironment;
import org.scijava.ops.engine.conversionLoss.impl.IdentityLossReporter;
Expand Down Expand Up @@ -169,4 +171,43 @@ public void testConvertAnys() {
Assertions.assertInstanceOf(Integer.class, out);
}

/**
* An Op, written as a method, whose type variable has multiple bounds.
* <p>
* Note that, for the purposes of this test, the {@link Number} bound is
* necessary even though it is not needed for the functionality of the test
* Op.
* </p>
*/
@OpMethod(names = "test.boundsConversion", type = BiFunction.class)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once again I got confused by the name of this op. It's weird to me that we have test ops named after the functionality being tested rather than named for what they do like normal ops. In this case, the op test.boundsConversion is a static method named foo (which I find unhelpful in understanding) and it returns the max of two objects. So why not name it "max" instead? Especially since the functionality being tested here, multiple type bounds, is also a sort of comparison, which was the source of my momentary confusion: "What does x > y ? x : y have to do with multiple bounds?" I thought, "It's just one bound..."

public static <T extends Number & Comparable<T>> T foo(T in1, T in2) {
return in1.compareTo(in2) > 0 ? in1 : in2;
}

/**
* Tests that conversion is possible when {@link TypeVariable}s in the
* {@link OpInfo} are bounded by multiple types.
*/
@Test
public void testConvertMultipleBounds() {
// Assert that there's only one possible match for our Op call
var name = "test.boundsConversion";
var infos = ops.infos(name);
Assertions.assertEquals(1, infos.size());
// And its input types are TypeVariables with two upper bounds.
var inType = infos.first().inputTypes().get(0);
Assertions.assertInstanceOf(TypeVariable.class, inType);
var numBounds = ((TypeVariable<?>) inType).getBounds().length;
Assertions.assertEquals(2, numBounds);
// Now, call it such that we need conversion
Integer i1 = 1;
Double i2 = 2.0;
var result = ops.op("test.boundsConversion") //
.arity2().input(i1, i2).apply();
// Assert the result is an Integer
Assertions.assertInstanceOf(Integer.class, result);
Integer intResult = (Integer) result;
Assertions.assertEquals(2, intResult);
}

}