unruly / java-8-matchers

Hamcrest Matchers for Java 8 features
MIT License
23 stars 5 forks source link

Support building matchers with lambdas #11

Closed runeflobakk closed 7 years ago

runeflobakk commented 7 years ago

This is a suggestion to solve #9

The implementation does not use TypeTools, but builds on the code provided by @benjiman in #9 and reflects on the lambda/method references using SerializedLambda. I needed to make some modifications to the original code, as lambdas on the form x -> x.y() are not treated the same as method references like x::y. In fact the latter provides the name of the referred method, which this implementation uses for describing the lambda, if available.

I would very much appreciate some reviewing of the treatment of the SerializedLambda, to possibly identify corner-cases I may have not been able to handle, if any.

CliveEvans commented 7 years ago

@bruceeddy Do you have time to do this, or do you want me to pick it up?

bruceeddy commented 7 years ago

Thanks @CliveEvans I'm picking this up now.

CliveEvans commented 7 years ago

On my development machine I have a failing test. This is because LambdaUtils.actualMethodOf is returning the wrong getBytes method. I suspect this is a jvm thing, I'll resolve locally and amend the PR.

runeflobakk commented 7 years ago

@CliveEvans is it LambdaMethodFinderTest.getResultDescription_resolvesMethodNameAndTypeForMethodReferences which fails? What is the test failure message? Which JVM do you use?

CliveEvans commented 7 years ago

@runeflobakk Failed tests: LambdaMethodFinderTest.getResultDescription_resolvesMethodNameAndTypeForMethodReferences:24 Expected: is "getBytes (a byte[])" but: was "getBytes (a void)"

java version "1.8.0_72" Java(TM) SE Runtime Environment (build 1.8.0_72-b15) Java HotSpot(TM) 64-Bit Server VM (build 25.72-b15, mixed mode)

The issue is the assumption that the first method with the same name is going to have the correct signature. This will depend on the jvm, although it appears to be based on the order they're declared in the class.

I spent quite a bit of time yesterday trying to make sure I was getting the correct method back from LambdaUtils.getActualMethod, but I think that was the wrong way of going about it. Since I was pretty much being forced to parse the signature of the lambda anyway, and we just want the return type, I suspect we should just do that instead. I've been looking at sun.reflect.generics.parser.SignatureParser, but I'm not happy with what it gives us back. OTOH, if I'm only trying to get the return type, a small helper that gives us a string representation for a sun.reflect.generics.tree.ReturnType wouldn't be too painful.

I'm not going to have time to look at this today, if you want to take a look at doing it in LambdaUtils, here's some failing and passing tests that should all pass. At least one of the tests should fail, regardless of your jvm:

@Test
public void shouldCorrectlyIdentifyStringGetBytes() throws Exception {
    DescribableFunction<String, byte[]> getBytes = String::getBytes;

    Method method = LambdaUtils.actualMethodOf(getBytes.asSerializedLambda());

    Method actual = String.class.getDeclaredMethod("getBytes", null);
    assertThat(method, equalTo(actual));
}

@Test
public void shouldCorrectlyIdentifyStaticMethodsWithVerySimilarSignatures() throws Exception {
    DescribableFunction<String, String> stringFunction = LambdaUtilsTest::someMethodWithSameNameAndSameParameterCount;

    Method method = LambdaUtils.actualMethodOf(stringFunction.asSerializedLambda());

    Method actual = LambdaUtilsTest.class.getDeclaredMethod("someMethodWithSameNameAndSameParameterCount", String.class);
    assertThat(method, equalTo(actual));
}

@Test
public void shouldCorrectlyIdentifyPrimitiveMethod() throws Exception {
    DescribableFunction<Integer, Integer> intFunction = LambdaUtilsTest::someMethodWithSameNameAndSameParameterCount;

    Method method = LambdaUtils.actualMethodOf(intFunction.asSerializedLambda());

    Method actual = LambdaUtilsTest.class.getDeclaredMethod("someMethodWithSameNameAndSameParameterCount", int.class);
    assertThat(method, equalTo(actual));
}

static int someMethodWithSameNameAndSameParameterCount(int ignored) {
    return 0;
}

static String someMethodWithSameNameAndSameParameterCount(String ignored) {
    return null;
}
runeflobakk commented 7 years ago

Ok, just to give an update. I have started looking at the tests you provided @CliveEvans (thanks!). Well, LambdaUtils.actualMethodOf simply does not take the parameter types into account when resolving the method, and that's just an embarrassing oversight on my part.

I have started to try to solve it, will keep you posted.

runeflobakk commented 7 years ago

Wow, that turned out to be really easy. I managed to delete the whole LambdaUtils class with its somewhat flaky resolving, and replace it with MethodType.fromMethodDescriptorString. Never used this class of the reflection API before; it essentially offers a bridge back to the reflection realm from the serialized form of the lambda.

CliveEvans commented 7 years ago

Well, a quick once over and it looks a lot nicer. Will check it properly tomorrow.

runeflobakk commented 7 years ago

@CliveEvans What do you think about moving the new DescriptionUtils class to an "internal" package as described in this comment? https://github.com/unruly/java-8-matchers/pull/11/files#r89789496 I didn't want to "just do it" since it sort of introduces a new architectural concept for the structuring of the library code.

CliveEvans commented 7 years ago

I agree with your reasoning and don't really have a better suggestion, to be honest.

runeflobakk commented 7 years ago

Hi! Sorry for nagging, but are there any progress on the review of this? :)