jqwik-team / jqwik

Property-Based Testing on the JUnit Platform
http://jqwik.net
Eclipse Public License 2.0
578 stars 64 forks source link

Domains inject parameterized values incorrectly #499

Closed SimY4 closed 1 year ago

SimY4 commented 1 year ago

Testing Problem

The setup like this:

@Domain(MyDomain.class)
@Domain(DomainContext.Global.class)
class TestTest {
  @Property
  boolean test(@ForAll Predicate<Number> p1, @ForAll Predicate<Number> p2, @ForAll Number n) {
    return p1.or(p2).test(n) == (p1.test(n) || p2.test(n));
  }
}

class MyThing implements Predicate<CharSequence> {
  public boolean test(CharSequence cs) { return true; }
}

class MyDomain extends DomainContextBase {
  @Provide
  Arbitrary<MyThing> myThingArbitrary() {
    return Arbitraries.just(new MyThing());
  }
}

will produce:

  java.lang.ClassCastException:
    class java.lang.Long cannot be cast to class java.lang.CharSequence (java.lang.Long and java.lang.CharSequence are in module java.base of loader 'bootstrap')

but if you remove domain and provide MyThing right in test class - no problem.

jlink commented 1 year ago

@SimY4 Interesting catch. Will look into it.

jlink commented 1 year ago

OK. It's a bug, but sadly one I've tried to fix a couple of times already without seeing a straightforward solution.

See https://github.com/jqwik-team/jqwik/blob/5055eb3f432e30b416f22a05cebaf7c56a0e3a33/engine/src/main/java/net/jqwik/engine/support/types/TypeUsageImpl.java#L410C1-L417C1

Maybe I'll find the time to reinvestigate. The problem is that type matching is sometimes too loose. And it's very hard to fix this without making other cases fail.

In your case the following change would fix it, but you probably know that already:

class MyDomain extends DomainContextBase {
    @Provide
    Arbitrary<Predicate<CharSequence>> myThingArbitrary() {
        return Arbitraries.just(new MyThing());
    }
}
SimY4 commented 1 year ago

@jlink yeah, I figured. Not sure why it works for provided in test though. Is it just by chance? I get that it's probably related to MyThing not having any explicit type arguments. Should it find and compare only matching interfaces between requirements and provided definitions? I.e. I need Predicate I have MyThing which has Predicate but Number is not assignable from CharSeq so no.

jlink commented 1 year ago

@jlink yeah, I figured. Not sure why it works for provided in test though. Is it just by chance?

I assume it's due to search order, which is in parts coincidental :-/

I get that it's probably related to MyThing not having any explicit type arguments. Should it find and compare only matching interfaces between requirements and provided definitions? I.e. I need Predicate I have MyThing which has Predicate but Number is not assignable from CharSeq so no.

I don't really understand what you mean, but if you'd use an explicity ArbitraryProvider implementation in your domain, which is possible by just having an inner class, with an explicit implementation of canProvideFor(TypeUsage targetType) MyThing should not be injected in your test property. Haven't tried, though; just dry-running the code in my head.

vlsi commented 1 year ago

The problem is that type matching is sometimes too loose. And it's very hard to fix this without making other cases fail.

I guess the reason is that you probably want the following to work:

  @Property
  boolean test(@ForAll Predicate<Number> p1, @ForAll Predicate<Number> p2, @ForAll Number n) {
    return p1.or(p2).test(n) == (p1.test(n) || p2.test(n));
  }

  @Provide
  Arbitrary<Predicate<Double>> myThingArbitrary() {
    return ...;
  }

Technically speaking Predicate<Double> is not a subtype of Predicate<Number>, so in theory, jqwik should not wire the two. It should rather fail with error saying "Predicate<Number> is not provided".

The proper Java code would be

  @Property
  boolean test(@ForAll Predicate<? super Number> p1, @ForAll Predicate<? super Number> p2, @ForAll Number n) {
    return p1.or(p2).test(n) == (p1.test(n) || p2.test(n));
  }

  @Provide
  Arbitrary<Predicate<Double>> myThingArbitrary() {
    return ...;
  }

In that case, Predicate<Double> is a subtype of Predicate<? super Number>, so jqwik should infer provide-consume link.

Of course, you might argue that it would break existing tests that relied on somewhat working jqwik's loose type comparison. However, I would argue, well, they have chosen Java language, so they should follow its verbose use-site-type-variance, and they must declare type variance on the use site according to the rules of Java language.

See https://github.com/jspecify/jspecify/issues/72


Unfortunately, Java does not have declaration-site type variance, so the workarounds could be: a) Use Kotlin, see https://github.com/jqwik-team/jqwik/issues/250#issuecomment-1031412461. With Kotlin you declare variance when declaring an interface or class, and it automatically puts ? extends and ? super when you use types in the code b) Hard-code "default variance for well-known classes like Predicate, Function, Consumer, Supplier, Arbitrary". For instance, it is known that java.util.function.Predicate<T> is de-facto a java.util.function.Predicate<in T>, so jqwik's type comparison could silently treat java.util.function.Predicate<Number> as if it was java.util.function.Predicate<? super Number> when comparing the types. c) Ask users to follow Java rules, and spell out @ForAll Predicate<? super Number> when they want a predicate with Number as an input

WDYT?

SimY4 commented 1 year ago

@vlsi But in this case I don't see how variance is relevant since Number and CharSequence has no intersection which should be detectable: Number and CharSequence has no relation.

I get an impression that the error happens because TypeRef can't examine generic parameters. For example this works fine:

class MyThing<T> implements Predicate<T> {
  @Override
  public boolean test(T cs) {
    return true;
  }
}

class MyDomain extends DomainContextBase {
  @Provide
  Arbitrary<MyThing<CharSequence>> myThingArbitrary() {
    return Arbitraries.just(new MyThing());
  }
}

So the solution here is to look for the matching generic interface in type hierarchy and compare their generic parameters. In this example:

Predicate[Number] =?= (MyThing <: Predicate[CharSequence])

Predicate[Number] =?= Predicate[CharSequence]

Number =!= CharSequence

So there should be a pre-step before comparing generic parameters that will look for the right generic interface in arbitrary type hierarchy.

jlink commented 1 year ago

@vlsi @SimY4 You‘re both right. Jqwik doesn’t handle variance perfectly, but the problem here is more straightforward and should be fixable as described by @SimY4 above.

vlsi commented 1 year ago

@jlink , I suggest the following:

if (targetType.getRawType().isAssignableFrom(rawType)) {
    // TODO: this is too loose, e.g. DefaultStringArbitrary can be assigned to Arbitrary<Integer>
    // In order to solve that nested type arguments of this and targetType must be considered
    if (allTypeArgumentsCanBeAssigned(this.getTypeArguments(), targetType.getTypeArguments())) {
        return true;
    } 

I suggest the following: 1) Resolve "target type's" parameters based on the required supertype. I think it can be implemented with https://github.com/harawata/typeparameterresolver/blob/ba7997dff74f454a09d31efc11cee7a02a7e6f8c/src/main/java/net/harawata/reflection/TypeParameterResolver.java#L82-L94

2) Then execute the comparison logic

I believe it would handle all the cases above, however, it would requite harawata/typeparameterresolver dependency

WDYT?

vlsi commented 1 year ago

https://github.com/jqwik-team/jqwik/issues/492 might be relevant as well

vlsi commented 1 year ago

Here's a sample test case for TypeUsageTests

    @Example
    void isAssignableParameterized() throws NoSuchFieldException, NoSuchMethodException {
        class LocalClass {
            public void test(
                Predicate<Double> predicateDouble,
                Predicate<Number> predicateNumber,
                Predicate<? super Number> predicateSuperNumber,
                Predicate<? extends Number> predicateExtendsNumber
            ) {
            }
        }
        Method method = LocalClass.class.getMethod("test", Predicate.class, Predicate.class, Predicate.class, Predicate.class);
        List<MethodParameter> parameters = JqwikReflectionSupport.getMethodParameters(method, LocalClass.class);
        TypeUsage predicateDouble = TypeUsageImpl.forParameter(parameters.get(0));
        TypeUsage predicateNumber = TypeUsageImpl.forParameter(parameters.get(1));
        TypeUsage predicateSuperNumber = TypeUsageImpl.forParameter(parameters.get(2));
        TypeUsage predicateExtendsNumber = TypeUsageImpl.forParameter(parameters.get(3));
        assertThat(predicateDouble.canBeAssignedTo(predicateNumber)).isFalse();
        assertThat(predicateNumber.canBeAssignedTo(predicateDouble)).isFalse();
        assertThat(predicateDouble.canBeAssignedTo(predicateSuperNumber)).isTrue();
        assertThat(predicateNumber.canBeAssignedTo(predicateSuperNumber)).isTrue();
        assertThat(predicateNumber.canBeAssignedTo(predicateExtendsNumber)).isFalse();
        assertThat(predicateDouble.canBeAssignedTo(predicateExtendsNumber)).isFalse();
    }
jlink commented 1 year ago

Here's a sample test case for TypeUsageTests

IMO (and the opinion of the compiler) the assertions are wrong:

assertThat(predicateDouble.canBeAssignedTo(predicateSuperNumber)).isTrue();
assertThat(predicateNumber.canBeAssignedTo(predicateSuperNumber)).isTrue();
assertThat(predicateNumber.canBeAssignedTo(predicateExtendsNumber)).isFalse();
assertThat(predicateDouble.canBeAssignedTo(predicateExtendsNumber)).isFalse();

should be

assertThat(predicateDouble.canBeAssignedTo(predicateSuperNumber)).isFalse();
assertThat(predicateNumber.canBeAssignedTo(predicateSuperNumber)).isTrue();
assertThat(predicateNumber.canBeAssignedTo(predicateExtendsNumber)).isTrue();
assertThat(predicateDouble.canBeAssignedTo(predicateExtendsNumber)).isTrue();
vlsi commented 1 year ago

there should be predicateSuperNumber.canBeAssignedTo(predicateDouble).isTrue as well

SimY4 commented 1 year ago

Just one other thought and another reason to find and compare matching generic interfaces is:

class MyThing<T> implements Predicate<CharSequence> { ... }

In here MyThing is assignable to Predicate but the generic param has nothing to do with the Predicate type argument.

vlsi commented 1 year ago

@SimY4 , please read "1." in https://github.com/jqwik-team/jqwik/issues/499#issuecomment-1625947469

It seems you missed it

jlink commented 1 year ago

predicateSuperNumber.canBeAssignedTo(predicateDouble).isTrue

Does not look like that was the case:

    void test(
        Predicate<Double> predicateDouble,
        Predicate<Number> predicateNumber,
        Predicate<? super Number> predicateSuperNumber,
        Predicate<? extends Number> predicateExtendsNumber
    ) {
        predicateSuperNumber = predicateNumber;
        // predicateDouble = predicateSuperNumber; // does not compile
        predicateExtendsNumber = predicateNumber;
        predicateExtendsNumber = predicateDouble;
}
jlink commented 1 year ago

I'm working on it on branch issue-449

Variance and type parameter matching seems fine so far, but I introduced a problem with recursive types on the way :-/

SimY4 commented 1 year ago

@jlink for the purposes of PBT framework do you need to solve this for such generic case? I feel like you can't possibly expect to summon instances of not fully resolved types. And if they are resolved then T should be concrete. Essentially you can give up exploring the hierarchy branch when you get the parameterised type. You can't compare T and U even if they have common supertype. Wildcards are ok though.

jlink commented 1 year ago

It's not about solving all cases, but the following property - and others like it - worked before:

@Property
<T extends Comparable<T>> boolean constrainedTypeVariable(@ForAll T aValue) {
    return aValue != null;
}

Now it fails with a SOF. That's not acceptable.

jlink commented 1 year ago

Provided a fix and released it as "1.7.5-SNAPSHOT".

@SimY4 Maybe you can try if your use case now works.

SimY4 commented 1 year ago

@jlink I ran my test suite using snapshot release - all seem to be working fine here.

jlink commented 1 year ago

Since a change to the behaviour of canBeAssignedTo(..) is relevant for a lot of hooks and domain implementations, I'll move the current version to 1.8.0-SNAPSHOT.

vlsi commented 1 year ago

Here's one more example which fails with bf4577bc356bcdb583a9adc7c953254e06b1878f

    @Example
    void canBeAssignedToParametereized() throws NoSuchFieldException, NoSuchMethodException {
        abstract class StrFunction<T extends Number> implements Function<CharSequence, T> {
        };
        class LocalClass {
            public void test(
                Function<? extends CharSequence, Integer> functionExtendsCsInteger,
                Function<? extends CharSequence, Number> functionExtendsCsNumber,
                StrFunction<Number> customNumber,
                StrFunction<Integer> customInteger
            ) {
                // Compilation fails if uncomment
                // functionExtendsCsInteger = customNumber;
                functionExtendsCsNumber = customNumber;
                functionExtendsCsInteger = customInteger;
                // functionExtendsCsNumber = customInteger;
            }
        }
        Method method = LocalClass.class.getMethod("test", Function.class, Function.class, StrFunction.class, StrFunction.class);
        List<MethodParameter> parameters = JqwikReflectionSupport.getMethodParameters(method, LocalClass.class);
        TypeUsage functionExtendsCsInteger = TypeUsageImpl.forParameter(parameters.get(0));
        TypeUsage functionExtendsCsNumber = TypeUsageImpl.forParameter(parameters.get(1));
        TypeUsage customNumber = TypeUsageImpl.forParameter(parameters.get(2));
        TypeUsage customInteger = TypeUsageImpl.forParameter(parameters.get(3));

        assertThat(customNumber.canBeAssignedTo(functionExtendsCsInteger)).isFalse();
        // FAILS in bf4577bc356bcdb583a9adc7c953254e06b1878f
        assertThat(customNumber.canBeAssignedTo(functionExtendsCsNumber)).isTrue();
        // FAILS in bf4577bc356bcdb583a9adc7c953254e06b1878f
        assertThat(customInteger.canBeAssignedTo(functionExtendsCsInteger)).isTrue();
        assertThat(customInteger.canBeAssignedTo(functionExtendsCsNumber)).isFalse();
    }
jlink commented 1 year ago

Here's one more example which fails with bf4577b

I'll see if that can easily be remedied. I assume not. The fix is an improvement but not a cure :-/

jlink commented 1 year ago

Working on a solution in branch https://github.com/jqwik-team/jqwik/tree/issue499

jlink commented 1 year ago

pull request to solve most/all of the variance and assignment issues

Also available as 1.8.0-SNAPSHOT