Junit5: Change to assertThrows in #1394 makes some existing call sites ambiguous

[cushon]

I initially reported this as a comment in #1394 after that issue was closed. I wanted to start a new bug in case the comment got missed.

We started using the new assertThrows in our code base, and found that there is a small fraction of cases where this change is incompatible with existing uses of the method.

If the argument is a lambda the compiler will perform type inference and select one of the two overloads. However for method references to some generic methods that throw checked exceptions, type inference isn't powerful enough to disambiguate the overloads:

// error: both method <T#1>assertThrows(Class<? extends T#1>,Runnable) and
// method <T#2>assertThrows(Class<? extends T#2>,Callable<?>) in Z match
assertThrows(ExecutionException.class, future::get);

This isn't a common problem: it affects about 1% of files in our codebase that use assertThrows. The vast majority of those are specifically using future::get, and we may have an unusual number of expected exception tests on Futures. There's also a simple work-around of using () -> future.get() instead of future::get.

This seems OK from my perspective. What do you think? Is this a concern, or is it an acceptable breaking change?

[sormuras]

Thanks for creating this issue, I did miss your comment in the closed bug.

Like you said, it's a breaking change: code that compiles with 5.2.0 and older version breaks with 5.3.0.

1% of a large enough codebase results in a high absolute number, right? What are we talking here? 100, 1.000 or even more assertThrows that need to be rewritten?

I see two options here:

1. keep the new overload as-is and document the breaking change. Maybe, javac and friends will be smarter and select the best match?
2. Invent a new method name for the overload ... making javac happy, and our Assertions API bit larger.

[cushon]

1% of a large enough codebase results in a high absolute number, right?

Right, although of course it depends how much code you have, and I'm also not sure the corpus I looked at is representative: we happen to have a lot of tests that are specifically testing future::get.
It affected about 700 files for us. I'd be interested to know how common this problem is in other codebases.

Invent a new method name for the overload ... making javac happy, and our Assertions API bit larger.

The disadvantage of renaming the method is that you have to know about (and remember to use) the other method to get the improved failure message. It's nice that with the current solution existing code gets the improved behaviour for free when it is recompiled.

[kevinb9n]

Sadly I think most of the value of the idea would be gone if it only benefits the users who know to opt into it. And even for the very-clued-in, it would be a mental burden to constantly have to decide which method to call.

So if the breakage is too big a problem I think we should back the whole thing out. :-(

[cpovirk]

A crazy thing we could try is:

interface ThrowingSupplier2 extends ThrowingRunnable {
  Object get() throws Exception;

  @Deprecated
  default void run() throws Exception {
    get();
  }
}

And then make the new method accept a ThrowingSupplier2 rather than a ThrowingSupplier.

Because ThrowingSupplier2 extends ThrowingRunnable, javac prefers ThrowingSupplier2 when possible.

But of course the extra type (which needs a better name...) is a wart, and a weird one at that.

[jbduncan]

@cpovirk Perhaps a helpful wart, in this case. :)

But yeah, although it may fix things, it's a bit strange and may require a lot of wordsmithing in the Javadocs. I'd be interested to hear what the JUnit 5 team think about this.

P.S. Fancy seeing you and @kevinb9n and @cushon here! I contributed to JUnit 5 in the past, so even if it's a bit late for me to say this, welcome to JUnit 5. :)

[sbrannen]

Tentatively slated for 5.3 M1 for _team discussion_

[sbrannen]

@cpovirk, when you say ThrowingRunnable, you mean ThrowingSupplier -- right?

[cpovirk]

Sorry, what I actually mean is Executable :) (I got mixed up with what JUnit 4 is probably going to call the type.) The important thing is that it's the type that's accepted by the other overload, so one overload becomes strictly more "specific" than the other.

[cpovirk]

Also: Rather than introduce a new type as I suggested, you could probably modify the existing ThrowingSupplier type to extend Executable. It would still have the default method, as above. I guess I was avoiding that because it made the existing type weird, and I was hoping to isolate the weirdness to assertThrows. But maybe it's more important to have only one ThrowingSupplier-like type.

[sbrannen]

Intriguing!

We'll ponder it.

Thanks for the feedback, @cpovirk.

[sbrannen]

Actually... after a bit of investigating, it turns out that half of our existing tests now use the ThrowingSupplier variants of assertThrows() instead of the Executable variants against which they were originally written and tested.

So that means the JUnit 5 test suite is not even testing what it's supposed to be testing.

We simply overlooked that since the tests continued to compile and pass. 😱

[sbrannen]

For example, for our very first test in AssertThrowsAssertionsTests:

    @Test
    void assertThrowsThrowable() {
        EnigmaThrowable enigmaThrowable = assertThrows(EnigmaThrowable.class, () -> {
            throw new EnigmaThrowable();
        });
        assertNotNull(enigmaThrowable);
    }

... the lambda expression there is inferred to be a ThrowingSupplier instead of an Executable (at least in Eclipse IDE).

[sbrannen]

Why on Earth the Java/Eclipse compiler thinks it returns something when it literally only throws something... well... that's beyond my comprehension. 😛

[sbrannen]

@cpovirk, applying your last suggestion:

public interface ThrowingSupplier<T> extends Executable {

    @Override
    default void execute() throws Throwable {
        get();
    }

    T get() throws Throwable;

}

Everything still compiles, and existing tests still pass, but that still leads to the compiler inferring the type to be ThrowingSupplier instead of Executable (as mentioned in my previous comment/finding).

I've not yet verified what happens with future::get and the like.

[sbrannen]

@cushon,

If the argument is a lambda the compiler will perform type inference and select one of the two overloads.

Is your compiler selecting the "correct" overload in such cases (i.e., in contrast to the behavior I reported above in Eclipse)?

[sbrannen]

Or.... is your compiler simply selecting "an" overload that just happens to compile and work?

[sbrannen]

I can confirm that given the following test...

    @Test
    void assertThrowsWithFutureMethodReference() {
        FutureTask<String> future = new FutureTask<>(() -> {
            throw new RuntimeException("boom");
        });
        future.run();
        ExecutionException exception = Assertions.assertThrows(ExecutionException.class, future::get);
        assertEquals("boom", exception.getCause().getMessage());
    }

It compiles and passes without the ThrowingSupplier variants in place.

And... when I reintroduce the ThrowingSupplier variants, I get the following compiler error in Eclipse.

The method assertThrows(Class<ExecutionException>, Executable) is ambiguous for the type Assertions

[sbrannen]

I can also confirm that using the _trick_ proposed by @cpovirk _works_ with the above future::get test method. It compiles and passes.

So, with the proposed _trick_, it would appear that everything still technically "works".

But... invocations of assertThrows() that were previously compiled against Executable will be compiled against ThrowingSupplier once recompiled.

I suppose that doesn't make much difference in the grand scheme of things, in terms of the outcome of the test.

[sbrannen]

In summary, it all boils down to two things.

1. Are we OK with having this _trick_ in place? (see https://github.com/junit-team/junit5/issues/1414#issuecomment-394673920)
2. Are we OK with the fact that code previously assumed to be executing as an Executable will now (in certain cases) be executed as a ThrowingSupplier even if the lambda expression does not return anything? (see https://github.com/junit-team/junit5/issues/1414#issuecomment-394669772 and https://github.com/junit-team/junit5/issues/1414#issuecomment-394680311)

[sbrannen]

With the current _trick_ in place, I've also rewritten the future::get test method as follows.

@Test
void assertThrowsWithFutureMethodReference() {
    FutureTask<String> future = new FutureTask<>(() -> {
        throw new RuntimeException("boom");
    });
    future.run();

    // Current compiler's type inference
    ExecutionException exception = Assertions.assertThrows(ExecutionException.class, future::get);
    assertEquals("boom", exception.getCause().getMessage());

    // Explicitly as an Executable
    exception = Assertions.assertThrows(ExecutionException.class, (Executable) future::get);
    assertEquals("boom", exception.getCause().getMessage());

    // Explicitly as a ThrowingSupplier
    exception = Assertions.assertThrows(ExecutionException.class, (ThrowingSupplier<?>) future::get);
    assertEquals("boom", exception.getCause().getMessage());
}

This technique allows us to verify that both variants work and simultaneously that type inference works regardless of what the current compiler thinks the lambda expression or method reference should be.

Thoughts?

[sbrannen]

For anyone interested in playing around with this, I've pushed my work to the following feature branch.

https://github.com/junit-team/junit5/commits/issues/1414-assertThrows-type-inference

[sbrannen]

And anyone interested in seeing the currently broken state of affairs can check out the change in 2b4ed35c2a47a2484dfdd503dec10d970176ce23 in master and uncomment the following line in an IDE: https://github.com/junit-team/junit5/commit/2b4ed35c2a47a2484dfdd503dec10d970176ce23#diff-bffc17213ec5b62b070d22200601c830R52

[sormuras]

I am OK with having the _trick_ in place, as long as the semantic of the assertion, the expected result, isn't changed.

[sbrannen]

For anyone interested in why future::get (which returns a value) worked when we only supported Executable (which does not return a value), here's an explanation from Brian Goetz:

Java allows you to _call_ a method and ignore the return value (a method invocation expression as a statement). Since we allow this at the invocation, we also allow this when adapting a method to a functional interface whose arguments are compatible but the functional interface is void-returning.

Note also that we'll do other adaptations (boxing, unboxing) to make the shape of the lambda match up with the expected shape of the functional interface. Ignoring a return value is just one of those adaptations.

[marcphilipp]

Thanks for the excellent analysis, @sbrannen!

1. Are we OK with having this _trick_ in place? (see https://github.com/junit-team/junit5/issues/1414#issuecomment-394673920)

I think the "trick" is ok since a ThrowingSupplier is _executable_.

1. Are we OK with the fact that code previously assumed to be executing as an Executable will now (in certain cases) be executed as a ThrowingSupplier even if the lambda expression does not return anything? (see https://github.com/junit-team/junit5/issues/1414#issuecomment-394669772 and https://github.com/junit-team/junit5/issues/1414#issuecomment-394680311)

I was wondering in which cases that might be because I was worried we might end up printing null as the actual returned result even though nothing ever returned a result. After a little experimentation it appears that the lambda needs to be written in a way so that all code paths end with return or throw. If there's a return, it wasn't an Executable in the first place so it wouldn't have compiled. If all paths lead to throw statements (like in our tests), it is now inferred to be a ThrowingSupplier. However, in that case we never reach the statement in AssertThrows that prints the actual returned value. Thus, I think we should be fine.

[sbrannen]

Thus, I think we should be fine.

I came to the same conclusion.

[sbrannen]

This has been fixed in master in commit 29911f2408c72b5873d63c5fe43cc51bedabe818.

[sbrannen]

@cpovirk, thanks again for sharing your insightful _solution_ to this problem! 👏

[sbrannen]

@jbduncan,

But yeah, although it may fix things, it's a bit strange and may require a lot of wordsmithing in the Javadocs.

Does this following _wordsmithing_ suffice for you? 😉

https://github.com/junit-team/junit5/blob/29911f2408c72b5873d63c5fe43cc51bedabe818/junit-jupiter-api/src/main/java/org/junit/jupiter/api/function/ThrowingSupplier.java#L26-L33

[jbduncan]

@sbrannen Yep, LGTM. Thank you very much! :smile:

[sormuras]

Just happend with 5.3.0 and IDEA:

Need to find the real cause for it, might be an IDEA-related issue.

[sormuras]

() -> object.wait() does work.

[sbrannen]

Although that screencast sure is _fancy_, it's a bit challenging to copy-n-paste the code from it. 😛

Though I suppose I can just type it into my IDE to test locally. 😇

[sbrannen]

It doesn't compile in Eclipse either.

I assume b/c of the overloaded wait() variants.

[sbrannen]

Eclipse says: Cannot infer type argument(s) for <T> assertThrows(Class<T>, ThrowingSupplier<?>)

[sbrannen]

It also doesn't compile using the Oracle JDK via the Gradle build:

/source/junit5/platform-tests/src/test/java/org/junit/platform/console/options/FooTests.java:14: error: incompatible types: cannot infer type-variable(s) T
                assertThrows(Exception.class, object::wait);
                            ^
    (argument mismatch; bad return type in method reference
      void cannot be converted to Object)
  where T is a type-variable:
    T extends Throwable declared in method <T>assertThrows(Class<T>,ThrowingSupplier<?>)

[sbrannen]

This is the test I'm using:

@Test
void test() {
    var object = new Object();
    assertThrows(Exception.class, object::wait);
}

[sbrannen]

In any case, the workaround is pretty straightforward:

@Test
void test() throws InterruptedException {
    var object = new Object();
    assertThrows(Exception.class, () -> object.wait());
}

Or (as you demonstrated):

@Test
void test() {
    var object = new Object();
    assertThrows(Exception.class, (Executable) object::wait);
}

[smoyer64]

@sbrannen Not working in Eclipse could also be a result of the _curiosity_ I reported in #1563.

[sbrannen]

Hi @smoyer64,

That's a different issue. #1563 was about the Stream variant.

The issue @sormuras reported above is about the compiler not being able to infer the generic type for an overloaded method (in this case Object.wait(...).

@sormuras discovered the issue in IntelliJ, and I verified it within Eclipse and from our Gradle build.

So, it would appear that type inference cannot be performed for the following compilers.

1. IntelliJ IDEA (don't know what version -- @sormuras would have to share that)
2. Eclipse Oxygen 4.7.3a using JDK 10
3. Oracle JDK 10.0.2

Thus, based on the above, my assumption is that the code in question will not compile with any common JDK.

In summary, I don't think it's necessarily an issue with our implementation of assertThrows() but rather with the compilers we use, and it could well be that Java simply does not support type inference for this particular scenario.

But then again... it _could_ also be a bug in the IntelliJ, Eclipse, and Oracle compilers. 😉

[sbrannen]

Trying to find something definitive with regard to lambdas and overloading.

Came across this explanation by Stuart Marks (for a different use case than ours): https://stackoverflow.com/a/21951311/388980

Excerpt:

The bottom line is that getting overload resolution right in the presence of type inference is very difficult in general, and that the language and compiler designers traded away power from overload resolution in order to make type inference work better. For this reason, the Java 8 APIs avoid overloaded methods where implicitly typed lambdas are expected to be used.

[sbrannen]

Based on various comments and tips I've seen on the Internet, I'm starting to think that overloading assertThrows() to accept different functional interfaces was perhaps not the best idea.

For example, various sources point out that the Stream API introduced methods with different names to support the following map variants:

<R> Stream<R> map(Function<? super T, ? extends R> mapper);
IntStream mapToInt(ToIntFunction<? super T> mapper);
LongStream mapToLong(ToLongFunction<? super T> mapper);
DoubleStream mapToDouble(ToDoubleFunction<? super T> mapper);

[sormuras]

Using object::notify works, as notify() is not overloaded.

assertThrows(IllegalMonitorStateException.class, object::notify);

[sbrannen]

Update:

I have verified that the example from @sormuras is in fact a regression (i.e., a bug in 5.3.0 GA).

The following compiles just fine.

import java.util.function.Supplier;

import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.function.Executable;

class AssertThrowsTests {

    public static <T extends Throwable> T assertThrows(Class<T> expectedType, Executable executable) {
        return Assertions.assertThrows(expectedType, executable);
    }

    @Test
    void test() {
        var object = new Object();
        assertThrows(Exception.class, () -> object.wait());
        assertThrows(Exception.class, (Executable) object::wait);
        assertThrows(Exception.class, object::wait);
    }

}

[sbrannen]

Thus, the introduction of overloaded assertThrows(...) methods that accept a ThrowingSupplier causes previously working code to no longer compile.

I will therefore open a new ticket to address this.

[sbrannen]

Please continue the discussion about this regression in #1576.