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

Bug: Matching of generic target types no longer works for provider methods in interface default methods #599

Closed
mihxil opened this issue Oct 14, 2024 · 14 comments
Labels

Comments

@mihxil
Copy link

mihxil commented Oct 14, 2024

Testing Problem

In jqwik 1.7 it was possible to have a structure like this:

public interface SimpleTest<T> {

   
    @Property
    default void simpleTest(@ForAll("datapoints") T datapoint) {

    }


    @Provide("datapoints")
    Arbitrary<T> datapoints();
}

Then have an extension for this, in which it is more natural to not refer to the provider any more as 'datapoints'.

interface ATest<X extends A> extends SimpleTest<X> {

    @Provide("as")
    Arbitrary<X> as();

    @Override
    @Provide("datapoints")
    default Arbitrary<X> datapoints() {
        return as();
    }


    @Property
    default void aProperty(@ForAll("as") X a) {

    }
}

And that have an actual test class

public class BTest implements ATest<B> {

    @Provide("as")
    public Arbitrary<B> as() {
        return Combinators.combine(
            Arbitraries.integers().between(1, 100),
                Arbitraries.strings().alpha().ofLength(5))
            .as(B::new);
    }
}

This test then runned fine. It runs both 'simpleTest' as 'aPropertyTest' for all b's.

Since 'datapoints' was just implemented in ATest with a default implementation, in all extensions it was the same as 'as'.

In versions of jqwik > 1.7 it doesn't work any more and sais:

 Cannot find an Arbitrary [datapoints] for Parameter of type [@net.jqwik.api.ForAll(value="datapoints", supplier=net.jqwik.api.ArbitrarySupplier$NONE.class) B]

Suggested Solution

I think the original behaviour was sensible, and this regression was probably not intentional.

@jlink
Copy link
Collaborator

jlink commented Oct 15, 2024

In general this should still be possible. What has changed is that the constraints applied to type variables are more strict now since they (try to) follow the Java compiler's rules for type substitutabiliy.

@mihxil I'll check your concrete example to find out what's happening here.

@jlink
Copy link
Collaborator

jlink commented Oct 15, 2024

@mihxil Can you add a simple implementations for A and B (and probably X) so that your example compiles? I could do it myself, but I might get their relation to type X wrong, which could change the picture. My guess is that there's an unexpected effect from these type relations.

@jlink
Copy link
Collaborator

jlink commented Oct 15, 2024

I figured out one possible problem: The variable datapoint in simpleTest is of generic type T which has no constraints, i.e. effectively it could be anything, but the compiler won't allow any concrete type to be assigned to datapoint.

Workaround: If you change the declaration to

@Property
default void simpleTest(@ForAll("datapoints") Object datapoint) {
}

it works for me.

Since T is unconstrained you can only use methods of Object anyway.

@mihxil
Copy link
Author

mihxil commented Oct 19, 2024

My case was of course a bit more complicated then this. I'm not convinced yet that it will do :-) But thanks for looking into it. I'll come back to you.

@jlink
Copy link
Collaborator

jlink commented Oct 19, 2024

OK. I'll leave the issue open until you can come up with more information.

@mihxil
Copy link
Author

mihxil commented Oct 28, 2024

I looked a bit further into it. And it is clear that the issue has only to do but with the title already sais. Using 'default' Provides breaks things. Every property depending on a 'default' implementation needs to accept a more generic type then needed.

The most simple thing I could come up

public interface A  {

}
public interface ATheory<T extends A>  {


    @Property
    default void toStringNoErrors(@ForAll("datapointsOrNull") Object object) {// Must be Object!
        Assertions.assertThatNoException().isThrownBy(() -> {
            System.out.println("" + object);
        });
   }

    @Provide
    Arbitrary<T> datapoints();


     @Provide
    default Arbitrary<@Nullable T> datapointsOrNull() {
        return datapoints()
            .injectNull(0.1);
    }

}
public class AImplTest implements ATheory<AImpl> {
    @Override
    public Arbitrary<AImpl> datapoints() {
        return
            Arbitraries.integers().between(1, 100).map((i) -> new AImpl());
    }
}
public interface B extends A {

    int getJ();

    default int plus(B o) {
        return o == null ? getJ() : o.getJ() + getJ();

    }
}

And here it gets ugly:

public interface BTheory<T extends B> extends ATheory<T> {


    @Property
    default void jNotNegative(@ForAll("datapoints") T b) {
        assertThat(b.getJ()).isNotNegative();
    }

    @Property
    default void plusTest(@ForAll("datapoints") T b1, @ForAll("datapointsOrNull") Object o2) {
        T b2 = (T) o2; // ugly

        assertThatNoException().isThrownBy(() -> {
            b1.plus(b2);
        });


    }
}

with

public class BImpl extends AImpl implements B {

    private final int j;

    public BImpl(int j) {

        this.j = j;
    }

    @Override
    public int getJ() {
        return j;
    }
}


public class BImplTest implements BTheory<B> {

    @Override
    @Provide("datapoints")
    public  Arbitrary<B> datapoints() {
        return
            Arbitraries.integers().between(1, 100).map(BImpl::new);
    }
}

 

So, I used default method to have also a provider that has nulls sometimes. But if I make something up for that, in this case just test if the 'plus' methods at least doesn't throw, and also accept an occasional null, it gets ugly. It was not needed in 1.7.2, I could just have then

    default void plusTest(@ForAll("datapoints") T b1, @ForAll("datapointsOrNull") T o2) {

No need for casting.

Yes, I can work around it by casting, and in such a simple example it is not so bad or course, but in actual classes the whole thing may get littered with casts.

It may be that the whole issue is that I don't understand generics well enough, it's always a bit hard. It may also be that what I'm trying here is a bit silly anyway for some reason.

But I really don't see why it should be like this. The 'datapointsOrNull' method is returning the correct type. And it did work earlier :-)

@jlink jlink added the bug label Nov 1, 2024
@jlink
Copy link
Collaborator

jlink commented Nov 1, 2024

My personal check for the question is it a generics problem or a bug: Can a variable of type Arbitrary<TargetType> be assigned the return value of the provider method. So in your case: Does the following code compile

interface BTheory<T extends B> extends ATheory<T> ...
    default void plusTestNotUgly(@ForAll("datapoints") T b1, @ForAll("datapointsOrNull") T b2) {
        Arbitrary<T> assigned = datapointsOrNull(); // Does this compile?
    }

The answer is yes, which makes this a bug as far as I currently understand it. The fact that it works for non-default methods but not default methods suggests the same.

@mihxil Thank you for catching and reporting this. I hope you can live with the workaround for the time being. I put the bug as first thing for 1.9.2 but cannot make any promises about when I'm going to work on it.

@jlink jlink changed the title Not possible any more to use default methods for @Providers. Bug: Matching of generic target types no longer works for provider methods in interface default methods Nov 1, 2024
@jlink
Copy link
Collaborator

jlink commented Nov 9, 2024

I've dived a bit deeper into the generic type matching logic. Short story: It's very complicated :-/
That means that I cannot fix it easily.

One thing you could do - besides the cast - is to override the provider method just calling super:

interface BTheory<T extends B> extends ATheory<T> ...
	@Property
	default void plusTestNotUgly(@ForAll("datapoints") T b1, @ForAll("datapointsOrNull") T b2) {
		assertThatNoException().isThrownBy(() -> {
			b1.plus(b2);
		});
	}

	@Override
	default Arbitrary<@Nullable T> datapointsOrNull() {
		return ATheory.super.datapointsOrNull();
	}

It's more code but less ugly. YMMV.

@jlink
Copy link
Collaborator

jlink commented Nov 9, 2024

Another option is to completely release the type constraints for provider methods and only require them to return some kind of arbitrary. This would lead to generation time errors if providers create non fitting instances - instead of property resolution errors.

@jlink
Copy link
Collaborator

jlink commented Nov 9, 2024

Another option is to completely release the type constraints for provider methods and only require them to return some kind of arbitrary. This would lead to generation time errors if providers create non fitting instances - instead of property resolution errors.

The more I think about that it seems to be the most pragmatic choice. And we had that already in earlier versions...

@jlink
Copy link
Collaborator

jlink commented Nov 28, 2024

Another option is to completely release the type constraints for provider methods and only require them to return some kind of arbitrary. This would lead to generation time errors if providers create non fitting instances - instead of property resolution errors.

The more I think about that it seems to be the most pragmatic choice. And we had that already in earlier versions...

I'm going forward with this idea.

jlink added a commit that referenced this issue Nov 28, 2024
@jlink
Copy link
Collaborator

jlink commented Nov 28, 2024

Fixed in 48cda86.

@mihxil Can be tested in version 1.9.2-SNAPSHOT. Please reopen the issue if the fix does not work for you.

@jlink jlink closed this as completed Nov 28, 2024
@jlink jlink removed the in progress label Nov 28, 2024
@jlink
Copy link
Collaborator

jlink commented Dec 3, 2024

Released in 1.9.2

@mihxil
Copy link
Author

mihxil commented Dec 16, 2024

thanks, sorry for not reacting sooner. I'll let you know if needed.

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