Split TestAnnotations into transforms and markers

[dmcg]

This aims to solve the fundamental problems that I ran into because I was combining annotations with transforms.

I wanted to have TestAnnotation<in F> to allow TestAnnotation<Any?> (for example SKIP) to be combined with TestAnnotation<MyFixture>, but because a TestAnnotation carried a transform

val transform: NodeTransform<@UnsafeVariance F>? get()

I ended up with the ability to add a transform that could send the wrong fixture type in to a test - https://github.com/dmcg/minutest/blob/master/core/src/test/kotlin/dev/minutest/experimental/AnnotationTortureTests.kt#L76

What I've done is to separate transforms from markers, and have the ability to add either to a NodeBuilder https://github.com/dmcg/minutest/compare/annotations-split?expand=1#diff-158654f07c4f987b79c4f9694e9571e8L14

TestAnnotation now doesn't have any generic parameter, and can either add itself to the [NodeBuilder] as a marker, or a transform, or both.

interface TestAnnotation {
     fun applyTo(nodeBuilder: NodeBuilder<*>)

It can't though know anything about the fixture type. That doesn't stop it implementing SKIP - because SKIP ignores the fixture.

JUnit Rules support and fixture sequence flattening are simplified because they can directly add a transform to do their thing, rather than creating an annotation to add a transform. Note that inside a context block transforms have access to the fixture type, it's only TestAnnotations that don't.

So far so good?

There is one place that I can't make the typesystem work for this. TransformingAnnotation - https://github.com/dmcg/minutest/compare/annotations-split?expand=1#diff-a074010cc4b7550504b6d6291daefc88R15

I think that TransformingAnnotation is safe, because it takes a NodeTransform<*>, which can't assume anything about the fixture type. So I think that it should be safe to add that transform to a NodeBuilder<*> - but the compiler thinks different. I've shoehorned it in with a cast

class TransformingAnnotation(
    private val transform: NodeTransform<*>
) : TestAnnotation {

    override fun applyTo(nodeBuilder: NodeBuilder<*>) {
        nodeBuilder.addTransform(transform as (Node<out Any?>) -> Nothing)
    }
}

Is this actually safe?

No, that isn't a rhetorical question, it's the reason that this is a PR rather than just smashed in to master.

[dmcg]

OK, so now I've had a day to step back from this, I see that NodeTransform<*> isn't safe, because I can build a transform that replaces a node with one that expects Int

val transform: NodeTransform<*> = { node: Node<*> ->
    Test<Int>("name", emptyList()) { anInt, _ ->
        anInt + 2
    }
}

and use that to build a TransformingAnnotation.

So that begs the question, what type can we pass to TransformingAnnotation that makes it impossible to assume the fixture type?

[dmcg]

NodeTransform<Nothing> is an obvious candidate, but in practice it restricts any assumptions at all about the fixture type, including its very existence.

NodeTransform<Any?> seems to have the desired characteristics - see the checks for its ability to access the fixture type in 15a84b2

The only thing that it doesn't have is the ability to call NodeBuilder<*>.addTransform( ) with it.

[robstoll]

NodeTransform<Any?> is probably better than NodeTransform<*> but still not safe. You can ask yourself why do you need F in Node in the first place. As far as I can see it is used in Testlet (which is just an alias for a function (F, TestDescription) -> F). This means, if you add a Testlet<Int> (if I get it right this has the semantic of a test with some fixture of type Int) and you are able to Transform this to Testlet<Any> with the help of an unsafe cast, then you can image how this can go wrong.

Maybe have a look at the heterogeneous map example from effective java: http://www.informit.com/articles/article.aspx?p=2861454&seqNum=8
You could use a similar approach for Node (Context in particular).
Notice thought, that you don't achieve compile time safety with it, only runtime safety (which means it will at least blow up at the immediate place where you currently do your unsafe cast and not somewhere else (later on) with something like MethodNotFoundException).
For a compile time safe approach I suggest to turn your transformations into async transformations which are applied immediately (but executed later on). This way you can transform a Node<List<String>> into multiple nodes Node<String> and each of them into a Node<Int> in a safe way.

In your case it would mean, remove addTransform from NodeBuilder and transform your NodeBuilder<F> into a NodeBuilder<G> (e.g. with the help of NodeTransform<F, G> where it is an alias for (Node<in F>) -> Node<out G>)

[dmcg]

Thanks Rob, can I ask for some clarification?

NodeTransform<Any?> is probably better than NodeTransform<*> but still not safe. You can ask yourself why do you need F in Node in the first place. As far as I can see it is used in Testlet (which is just an alias for a function (F, TestDescription) -> F). This means, if you add a Testlet<Int> (if I get it right this has the semantic of a test with some fixture of type Int) and you are able to Transform this to Testlet with the help of an unsafe cast, then you can image how this can go wrong.

You are correct that the F is the fixture type which is supplied by a container to its child nodes. So we need to make sure that a context cannot build the wrong fixture type, nor have a child that is expecting a different type than its context supplies. This applies not only to Test but also to Context - which can transform the fixture supplied by its parent.

I believe (based on my (demonstrably flawed) knowledge and intuition and failure to find counter- examples) that the current code will not let you supply a transform that can make any assumptions about the fixture type, other than it is a subtype of Any?. So you can't replace a node with one that assumes a fixture type - these things don't compile

    // A TransformingAnnotation

    // * should not be able to return a node that expects a particular fixture type to be supplied
    
    val `transforms to node that expects a particular fixture type` = TransformingAnnotation { node ->
        Test<Int>("name", emptyList()) { anInt, _ -> // compile failure - Required: Node<Any?>, Found: Test<Int>
            anInt + 2
        }
    }

    // * should not be able to assert that the node requires a particular fixture type
    
    val `transforms expects a particular fixture type` = TransformingAnnotation {
        node: Node<Int> -> node // compile failure - Expected parameter of type Node<Any?>
    }

    // * should not be able to accidentally pass a typed transform to TransformingAnnotation
    
    val intTransform: NodeTransform<Int> = { node -> node }
    val `can't pass transform with type to TransformingAnnotation` = 
        TransformingAnnotation(intTransform) // compile failure - Required: NodeTransform<Any?>, Found: NodeTransform<Int>
    
    @Suppress("UNCHECKED_CAST")
    val `could cast, but on your head be it`  = TransformingAnnotation(intTransform as NodeTransform<Any?>)

    val `can't do an unsafe thing by assignment`: NodeTransform<Any?> = 
        intTransform // compile failure - Required: NodeTransform<Any?>, Found: NodeTransform<Int>
    
    @Suppress("UNCHECKED_CAST")
    val `can cast, but but on your head be it`: NodeTransform<Any?> = intTransform as NodeTransform<Any?>

Can you find a place where you can actually get a ClassCastException with the current code without writing an UNCHECKED_CAST to force it through?

Maybe have a look at the heterogeneous map example from effective java: http://www.informit.com/articles/article.aspx?p=2861454&seqNum=8
You could use a similar approach for Node (Context in particular).

Do you mean for the (what I'm now calling) markers or for transforms?

For a compile time safe approach I suggest to turn your transformations into async transformations which are applied immediately (but executed later on). This way you can transform a Node into multiple nodes Node and each of them into a Node in a safe way.

In your case it would mean, remove addTransform from NodeBuilder and transform your NodeBuilder into a NodeBuilder (e.g. with the help of NodeTransform<F, G> where it is an alias for (Node<in F>) -> Node<out G>)

I'm having trouble grokking applied immediately (but executed later on) - could you expand? I tried to make transforming NodeBuilders work following your previous suggestion, but it's hampered by NodeBuilder just being an interface, so not having the nice sealed Node restricted to Context or Test.

I'm trying to achieve a balance of safety and convenience here, hampered by my own lack of intuition in variance. Something is going right, because so far I haven't had to change the public interface through all these changes, but there are limitations on what is a non-breaking change.

My gut feel is that the approach of only allowing TransformingAnnotations that cannot make assumptions about the fixture type is safe enough, but of course only if I can express that restriction. Can anyone find any code that compiles and shouldn't? My current set of attempts

[robstoll]

Just by reading your additional commits I guess you have already found a case where one can sneak in wrong types.
There are multiple places where you could use heterogeneous List-- following a few:

• TestBuilder.transforms
• MinutestContextBuilder.transforms
• Node.markers
• Context.children

With heterogeneous Lists in place you could get rid of your UNCHECKED_CASTs and be safe during runtime.

I'm having trouble grokking applied immediately (but executed later on) - could you expand?

Maybe I get to you by another analogy: think in terms of Sequence rather than List
With sequence you can do listOf(1,2,3).asSequence().map { it + 1 } and the initial list stays the same until you use something like toList
Currently you apply the transformers not in this way but more like listOf(1,2).map{ it + 1} which immediately results in a new list

[dmcg]

I think that this branch is in a better place than master and has (probably) no effective breaking changes, so I'm going to merge it in.

I'm still interested in any places where the annotations can apply an unsafe transform.