GROOVY-7971: do not save instanceof types under logical or

[eric-milles]

https://issues.apache.org/jira/browse/GROOVY-7971

This is the simplest approach I could think of. Instead of collecting all instanceof types when doing multiple checks like "it instanceof String || it instanceof Boolean || it instanceof Integer", do not collect the instanceof types because there could be any kind of boolean expresion on either side of the ||.

[paulk-asert]

I am not sure dumbing down the type checker is the best way. It would break code like the following (for the peek() case) which while not currently fully checked correctly is what we want to eventually aim for I would think:

@groovy.transform.TypeChecked
def method() {
    def component = new Random().nextBoolean() ? new ArrayDeque() : new Stack()
    component.clear() // 'clear' in LUB (AbstractCollection or Serializable or Cloneable)
    if (component instanceof ArrayDeque) {
        component.addFirst(1) // 'addFirst' only in ArrayDeque
    } else if (component instanceof Stack) {
        component.addElement(2) // 'addElement' only in Stack
    }
    if (component instanceof ArrayDeque || component instanceof Stack) {
        // checked duck typing
        assert component.peek() in 1..2 // 'peek' in ArrayDeque and Stack but not LUB
    }
}

method()

The above fails (randomly at runtime with a CastClassException) when using @CompileStatic instead of @TypeChecked but that is something we should fix in our bytecode generation. While not what we like to aim for, I think we should generate hidden instanceof checks here.

Having said that, we can improve how things are handled. We should have the concept of bottom in our type system and an arbitrary expression would yield that and then don't collect types for OR in that scenario. Then we could work on making the type checker smarter to be able to potentially handle expressions like def isString = value.class == String as in the test case here so that some expressions might not yield bottom. We also need both union types and intersection types represented (though I agree that those names aren't necessarily the best to use).

[eric-milles]

Is there a test case or open ticket for your example of ArrayDeque (aka Deque, aka Queue) and Stack? There were only two test cases for the or'ing of instanceof checks.

Not looking to nerf type inferencing, but in case of "... && x instanceof T" less needs to be known about what "..." represents. For "... || x instanceof T" it seems that only when "..." is another instanceof check or multiple "or instanceof" clauses do we want to track the types.

Anyway, thanks for the feedback. I wanted to get something simple out there so we could talk through the possibilities.

[eric-milles]

I've been looking at several cast exceptions over the last few days. In terms of static compilation, I find that type inferencing plays a big part. In your case "component.peek()" is probably re-written with a cast, like "((T) component).peek()". And I think T is "UnionType:ArrayDeque+Stack" in the node metadata but probably gets written out as "((java.util.ArrayDeque) component).peek()". So when component is a stack, there is a cast exception at runtime.

[eric-milles]

This bit in StaticInvocationWriter is where a dynamic call is replaced with a cast and call to ArrayDeque#peek under static compilation.

[paulk-asert]

Yes, we'd need to augment that with either instanceof checks or some kind of invokedynamic. In theory peek() should work for (x instanceof ArrayDeque && x instanceof Number) || (x instanceof Stack && x instanceof Thread) but just the vanilla OR would be a great first start.

[eric-milles]

So can this change go in to fix the cast exceptions and a new ticket get created for the multi-methods case?

[paulk-asert]

Well, with the current PR, we have to break the type checker to hide some of the cast errors. I'll ponder for a bit to see if we can't better fix the real underlying issues first.

[daniellansun]

@paulk-asert Any further comments on the PR?

[paulk-asert]

I have been doing a few spikes but nothing finalised.

I tried using Union and Intersection types to better model the type relationships. A logical OR with instanceof on both sides for example should have different semantics to a logical AND with instanceof on both sides. And we'd need a NotType as well at some point to have better handling of not in combination with instanceof for nested cases. I can get specific parts of our type checking processing to work as I'd hope with these in place but there are numerous places in the code which make different assumptions to what the new types would need. There are places where we ignore Object for instance which isn't what we'd now need but just removing the places where we make such tests breaks other things because various assumptions flow to other parts of the code.

So, as an intermediate step, I have a slightly improved mechanism that looks for any IfStatement where the test is a binary logical OR and treats that as two individual IfStatement's corresponding to each half of the OR. It then does a separate pass to get the UnionType we currently expect for the AND case. The UnionType needs to be catered for so that we have an invokeDynamic instruction produced when @cs is in place. I think it will be a few more days before I have anything ready for review.

[paulk-asert]

@eric-milles @danielsun1106 Let me know if #1293 works for you. Failed on one CI build but works for me locally with a similar JVM.