partiql-ir-generator icon indicating copy to clipboard operation
partiql-ir-generator copied to clipboard

Make visitors able to visit any domain type

Open dlurton opened this issue 4 years ago • 4 comments

Given a simple domain:

(define (domain simple (product foo))

For the Kotlin target we get something like:

class Simple {
    ...
    abstract class SimpleNode { ... } 
    ...
}

However, none of the generated visitors can accept instances of SimpleNode. This is a problem if you have an instance of SimpleNode and want to send it thru one of the visitors. The user has to write a when statement and dispatch to the appropriate visitor method. PIG's own tests (within PermuteTransformTests) for example contain the following code:

        // https://github.com/partiql/partiql-ir-generator/issues/66
        val actual = when(tc.input) {
            is TestPermuteDomainA.ProductA -> tt.transformProductA(tc.input)
            is TestPermuteDomainA.RecordA -> tt.transformRecordA(tc.input)
            is TestPermuteDomainA.SumB -> tt.transformSumB(tc.input)

            // `else` is needed since TestPermuteDomainA.TestPermuteDomainANode is not currently a sealed class
            else -> fail("unexpected type")
        }

This is potentially a pain point for users and should really be part of the generated visitors:

class Simple {
     open class Visitor : DomainVisitorBase() {
        ... 
        // Add this:
        fun walkNode(node: SimpleNode) { T.B.D. }
        ... 
     }

    open class VisitorFold<T> : DomainVisitorFoldBase<T>() {
        ... 
        // Add this:
        open protected fun walkNode(node: SimpleNode, accumulator: T): T = T.B.D.
        // Add this:
        open fun walkNode(node: SimpleNode, accumulator: T) = T.B.D.
        ... 
    }

    abstract class VisitorTransform : DomainVisitorTransformBase() {
        ... 
        // Add this:
        open fun transformNode(node: SimpleNode): SimpleNode = T.B.D.
        ... 
    }
}

As part of this we should consider if it is possible and advisable to make SimpleNode a sealed class.

  • [ ] Evaluate if it is feasible and wise to make the generated domain node super class (i.e. SimpleNode) sealed, and if so, make it so.
  • [ ] Add the noted function for in generated DomainVisitorBase implementations
  • [ ] Add the noted functions DomainVisitorFoldBase<T> implementations
  • [ ] Add the noted function DomainVisitorTransformBase implementations
  • [ ] Remove the (now) redundant when block (shown in the example above) from PermuteTransformTests

dlurton avatar Jan 18 '21 00:01 dlurton

I'm curious how this is different than using accept? I believe the intention of an accept method is to invoke the appropriate visitor method when then concrete type in unknown. A "reverse" accept like the when clause you are using is useful when you cannot modify or do not have an accept method of nodes. Considering we generate the code, we should be generating the appropriate accept methods.

val actual = tc.input.accept(tt)

rchowell avatar Jun 30 '22 18:06 rchowell

If visitors have parameterized return types like discussed here -- https://github.com/partiql/partiql-ir-generator/issues/123 then you include the parameterized return type and context objects in the accept. The current state of visitors/transformers/etc. is all confusing and should be consolidated to a single abstraction to avoid this confusion

rchowell avatar Jun 30 '22 18:06 rchowell

Here's a modified example from an old project

interface IRVisitor<R, C> {

    fun visit(node: IRNode, ctx: C): R? = defaultVisit(node, ctx)

    // "reverse" visit for nodes A, B extending X
    // you are proposing generating one of these for each sum type and the whole domain
    fun visit(node: IRNodeX, ctx: C): R? = when (node) {
        is IRNodeA -> visit(node, ctx)
        is IRNodeB -> visit(node, ctx)
    }

    // .... removed

}


sealed interface IRNode  {
    override fun <R, C> accept(visitor: IRVisitor<R, C>, ctx: C): R? = visitor.visit(this, ctx)
}

Then given some IRVisitor and an arbitrary IRNode, we can call accept on the IRNode to invoke the appropriate visitor method via function overloading.

Am I missing something here?

rchowell avatar Jun 30 '22 18:06 rchowell

You call ctx accumulator, but the visit doesn't have to be a fold. Visitor ctx like this is useful for passing arbitrary context (which may be an accumulator) like indentation level for pretty printing for example

rchowell avatar Jun 30 '22 18:06 rchowell