Skip to content
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.

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

generated Generic Functions are not pure #126

Closed
Danny02 opened this issue Oct 20, 2020 · 11 comments
Closed

generated Generic Functions are not pure #126

Danny02 opened this issue Oct 20, 2020 · 11 comments
Labels

Comments

@Danny02
Copy link
Contributor

Danny02 commented Oct 20, 2020

Testing Problem

While switching from 1.2.7 to 1.3.6 I encountered the following regression.

a property for a generic value of type T and and a second value of type Function<T, String> generates functions which are not pure. Some of the functions called with the first value return different values on each call.

@jlink
Copy link
Collaborator

jlink commented Oct 20, 2020

@Danny02 Shouldn't happen. Can you give an example?

@Danny02
Copy link
Contributor Author

Danny02 commented Oct 21, 2020

@jlink I created a minimal failing example for you (it happens with a custom arbitrary provider)
https://github.com/Danny02/jqwik-errors/blob/master/src/test/java/de/PureFunctionSpec.java

@jlink
Copy link
Collaborator

jlink commented Oct 21, 2020

Copying your example:

import io.vavr.control.Validation;
import net.jqwik.api.ForAll;
import net.jqwik.api.Property;

import java.util.function.Function;

public class PureFunctionSpec {
    @Property
    <T> boolean isPure(@ForAll T t, @ForAll Function<T, Validation<T, String>> f) {
           return f.apply(t).equals(f.apply(t));
    }
}

How are instances of io.vavr.control.Validation being generated?

@Danny02
Copy link
Contributor Author

Danny02 commented Oct 21, 2020

see https://github.com/Danny02/jqwik-errors/blob/master/src/test/java/de/providers/ValidationArbProvider.java

public class ValidationArbProvider implements ArbitraryProvider {

@Override
public boolean canProvideFor(TypeUsage typeUsage) {
    return typeUsage.canBeAssignedTo(TypeUsage.forType(Validation.class));
}

@Override
public Set<Arbitrary<?>> provideFor(TypeUsage typeUsage, SubtypeProvider subtypeProvider) {
    var failType = typeUsage.getTypeArgument(0);
    var successType = typeUsage.getTypeArgument(1);

    var failArbs = subtypeProvider.apply(failType);
    var successArbs = subtypeProvider.apply(successType);

    return failArbs.stream().flatMap(f -> successArbs.stream().map(s -> create(f, s))).collect(Collectors.toSet());
}

Arbitrary<?> create(Arbitrary<?> fail, Arbitrary<?> success) {
    return combine(fail, success.optional()).as((f, s) -> Option.ofOptional(s).toValidation(f));
}
}

@Danny02
Copy link
Contributor Author

Danny02 commented Oct 21, 2020

the more minimal example

@Property
<S> void isPure(@ForAll String t, @ForAll Function<String, S> f) {
    assertEquals(f.apply(t), f.apply(t));
}

fails too. without any custom generators.

Both example are working if the generic types are replaced by concrete.

@jlink
Copy link
Collaborator

jlink commented Oct 22, 2020

As I see it the behaviour is correct (and seems to have been the same in version 1.2.7 and before). Let me explain:

Since S can represent any class it can also represent classes that do not provide their own equals implementation, e.g. Object. In these cases the generated function will provide an instance of Object which is instantiated in the exact same way. But new Object().equals(new Object()) is false :-(.

Returning the same instance in repeated calls seems wrong to me because most functions (e.g. factories) do not work that way. This has also been discussed here: #73

So what can you do? I suggest you restrict the type variable in some way to make sure only types with reasonable equality behaviour are considered, e.g.:

@Property(afterFailure = AfterFailureMode.RANDOM_SEED)
<S extends Comparable<S>> void isPure(@ForAll String t, @ForAll Function<String, S> f) {
	assertEquals(f.apply(t), f.apply(t));
}

Alternatively you could create a domain that only provides types that you consider useful for purity testing and use this domain in those tests.

A possible new feature would be to have is some kind to filter out types of type variables, e.g. through an annotation like @FilterTypes(WithEqualityOnly.class, filterSubtypes=true). Such a feature could be implemented with some effort but the annotation would have to be at the usage of the type variable, not the declaration (the reason is also discussed in #73). If such a feature is interesting for you, please create a new issue for it.

I'm closing this issue. Feel free to reopen it if my reasoning is flawed, which is the case often enough.

@jlink jlink closed this as completed Oct 22, 2020
@Danny02
Copy link
Contributor Author

Danny02 commented Oct 22, 2020

Hi thx for the explaination of this behavior, but I think there is something different going on in my initial problem (I encountered in my project).

@Property
<E> void buggy(@ForAll String t, @ForAll Function<String, Validation<String, E>> f) {
    assertEquals(f.apply(t).isValid(), f.apply(t).isValid());
}

@Property
<E> void works(@ForAll String t, @ForAll Function<String, Validation<E, String>> f) {
    assertEquals(f.apply(t).isValid(), f.apply(t).isValid());
}

In this example I do not use equals()on any generated value. Validation is an ADT (Failure or Success), the generated function seems to generate completly different values on each call.

Do you see anything wrong with my ArbitraryProvider implementation?

@jlink jlink reopened this Oct 22, 2020
@jlink
Copy link
Collaborator

jlink commented Oct 22, 2020

I can replicate your problem. It seems unrelated to the behaviour described above. I'll have a closer look at it.

@jlink jlink added the bug label Oct 22, 2020
@jlink
Copy link
Collaborator

jlink commented Oct 24, 2020

It's a really tricky bug. Thanks for catching it!

It (probably) has to do with Java's Optional class. I'll research that later, but I have a work-around for you that I think will improve the understandability of your ValidationArbProvider class. I suggest you change the create method in such a way:

    Arbitrary<Validation<?, ?>> create(Arbitrary<?> fail, Arbitrary<?> success) {
        return Arbitraries.oneOf(
                success.map(Validation::valid),
                fail.map(Validation::invalid)
        );
    }

It makes clear what the original optional() call hides: You are choosing between a valid and an invalid validation.
If you want to shift the probabilities towards more valid instances, you can do it like that:

    Arbitrary<Validation<?, ?>> create(Arbitrary<?> fail, Arbitrary<?> success) {
        return Arbitraries.frequencyOf(
                Tuple.of(4, success.map(Validation::valid)),
                Tuple.of(1, fail.map(Validation::invalid))
        );
    }

which will give you a 20% probability for invalids. optional() will create empty values with a probability of 5% only.

@Danny02
Copy link
Contributor Author

Danny02 commented Oct 25, 2020

yes this is a mutch more clear implementation^^

jlink added a commit that referenced this issue Oct 26, 2020
@jlink
Copy link
Collaborator

jlink commented Oct 26, 2020

I also fixed the original problem. Available in 1.3.7-SNAPSHOT

@jlink jlink closed this as completed Oct 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants