Fixes for #637, making theory parameters assignment more consistent across sources

[pimterry]

There's a few inconsistencies in theory implementation, as discussed in #637. This shouldn't make any differences to reasonable normal usage, just to some edge cases.

Essential changes:

• Datapoint matches for all field datapoints and multi-valued method datapoints (i.e. not single-valued method datapoints) are done on runtime type, not the field/method return type (previously this was the case for multi-valued array methods only)
• All datapoint type matches now correctly handle boxing and unboxing for primitive and wrapper types; int values will be considered for theory parameters that are Integer assignable, and Integer values will be considered for int parameters)

(#637 has examples and more details on this)

This conflicts with my other open pull request (#639), but given that we want to make the behaviour in that case more complicated, I think it would be better to fix these inconsistencies first, and then continue that on top. This is also most of the setup work for a fix I have going for #110/#126, as we have to pull types at runtime for iterables there anyway (since the generic type info isn't reliably available).

[dsaff]

Generating an exception can be pretty expensive. Can we use a map reversing the one in canAcceptBoxed, instead?

[dsaff]

One red flag to me: this might make certain types of theory classes much slower. For example, consider a class BigExpensive that is expensive/slow to create:

public class SomeTheories {
@DataPoint public static BigExpensive bigExpensiveOne() { ... }
@DataPoint public static int ZERO = 0;

@test public void foo(int x) { ... }
@test public void fooExpensive(BigExpensive b) { ... }
}

Here, we'll be invoking bigExpensiveOne for foo, which we weren't before.

Perhaps we can optimize by detecting when there's no chance that the runtime type of a method return value could possibly match a parameter's type?

[pimterry]

Updated to fix those two comments. We now make the conversion map symmetric, so we don't need to use reflection (so there's no expensive exception thrown), and we check that types are potentially assignable before calling both single-valued and multi-valued datapoint methods. Multi-valued won't be feasible to work like this for iterables because of type erasure, but we can make it work for arrays, as now, so we might as well do that too.

[kcooney]

I would really prefer that we don't use instance initializers in JUnit. In a constructor like this:

public MyType(...) {
  super(a, b);
  doSomething(c);
}

It is very surprising that there is code magically executed between these two lines.

Could we instead do:

private static final Map<Class<?>, Class<?>> CONVERTABLE_TYPE_MAP = convertableTypeMap();

As a bonus, 1) convertableTypeMap() can return an unmodifiable map and 2) you can remove the @SuppressWarnings("serial")

[pimterry]

Yes, that's nicer, done.

[dsaff]

Perhaps even simpler (and certainly a little more efficient) would be:

addSymmetrically(map, boolean.class, Boolean.class), where

addSymmetrically =
map.put(a, b);
map.put(b, a);

[pimterry]

I don't have any strong feelings on this one way or the other, but I'm happy to change it if you do, so I have.

[dsaff]

Indentation went weird here?

[dsaff]

We're getting close. Can you edit the pull-request summary to summarize the current state of things?

Moving forward, I wonder if we could harmonize everything by having the ability for datapoints to return either a Foo or a Provider, with Provider as a way to get deferred computation.

[pimterry]

Summary updated and whitespace fixed.

Stream of thought on providers (although this is quite out of scope, and we should open an issue/wiki page/something if you want to take this further):

Provider and values is certainly do-able, and committing to that model would nicely clarify the single/multi datapoint method differences.

It does sort-of duplicate the ParameterSuppliers though, from the other direction (specifying a source of generated assignments for all parameters in a class, rather than requesting assignments generation from a source for one parameter). What's the difference between a ParameterSupplier and a Provider here? There's clearly some, but it's not really intuitive to me...

I think if you want to move to that model and carefully break this backward-compatibility, I'd instead separate by annotations to make it clearer: have @DataPoints all be generated once, statically (i.e. put the single-valued datapoints change back in) and then have a @DataPointGenerator (name tbd) annotation for datapoint methods whose values are rebuilt each time. This notably makes for easy migration for people who get bitten by this non-backward compatibility: they just switch to the new annotation, and they don't have to change return types to wrap generation up in a Provider or similar. And splitting behaviour based on the annotated method's return type is going to be a bit messy too (what if you want to actually take Provider parameters to a theory method, to test your providers?)

I think there's maybe a way you can do this even better and pull it closer into the ParameterSupplier model, so you end up with two clear parallel parameter-source hierarchies -- field/method datapoints for static data values and field/method/class ParameterSuppliers for regenerating data values -- but I haven't totally worked out the model for that yet. Ideas very welcome!

[dsaff]

Since CONVERTABLE_TYPES_MAP is symmetric, and canAcceptType already checks one direction, we don't have to also check the other direction here, right?

[dsaff]

Never mind, I should have re-loaded the code into my wetware memory before commenting. No change needed here.

[dsaff]

Okay, we're in good shape on this pull. I'll merge. Can you make a comment in the release notes?

I agree that further discussion would be interesting outside the scope of this pull request, and think your "separate annotation" idea has merit.