API changes: extend visitor with return values

[wumpz]

If you try to build a CopyVisitor of some kind of object hierarchy, one has to build some kind of Stack structure, to give the actual visitor a hint, where it should write its copied data.

I suggest to extend all visitor methods with a return value:

MyType visitor(MyType param);

The standard implementation should return the actual object inserted.

public interface StatementVisitor {
    Comment visit(Comment comment);
    Commit visit(Commit commit);
    Delete visit(Delete delete);
    Update visit(Update update);

[wumpz]

Using this we would be able to create some kind of functional visitor implementation.

[wumpz]

There is a thumbs up above, but I am more interested in textual comments :).

[AnEmortalKid]

I'm having a bit of a hard time sort of visualizing this, so correct me if I wrong.

Let's assume we just had a SelectVisitor to copy the Select.

Old Way:

class OldSelect implements SelectVisitor {

private Select copy = new Select();

// super naive and bad just illustrating examples
Select copy(Select original) {

  visit(original);

return copy;
}

  @Override
    public void visit(Select select) {
      copy.setBody(deepClone(select.getSelectBody());

        // potentially the with items would be
      copy.setWithItems(deepClone(select.getWithItems()));
    }
}

With new pattern:

class NewCopySelect implements SelectVisitor {


// super naive and bad just illustrating examples
Select copy(Select original) {
  return visit(original);
}

  @Override
    public Select visit(Select select) {
   // and other copying stuff
     Select copy = new Select();
      copy.setBody(deepClone(select.getSelectBody());

    // potentially the with items would be
    copy.setWithItems(visit(select.getWithItems());
      return copy;
    }
}

Or am I Missing something.

[wumpz]

Correct. With this new style the visitors itself return the copy and the caller could immediately put it in the right place. So you don't need to organise your copied items in some sort of stack to find the last generated. Imho a global variable here should be avoided but cannot within the actual implementation.

[AnEmortalKid]

extend all visitor methods with a return value

Would it be valuable to have CopyStatementVisitor and the current StatementVisitor ? If not, then changing StatementVisitor would be fine, probably just a big warning to all consumers that things no longer return void.

[wumpz]

Indeed this would be a massive api change. That is why we would go for version 4.

If there are two visitor interfaces then we would need to visit methods per class. So that is IMHO a no go.

[AnEmortalKid]

+1 to new version and breaking API then!

…

On Mon, Jan 13, 2020 at 00:34 Tobias ***@***.***> wrote: Indeed this would be a massive api change. That is why we would go for version 4. If there are two visitor interfaces then we would need to visit methods per class. So that is IMHO a no go. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#901?email_source=notifications&email_token=AA2QHQLZ6H7UIQTBGUFMTK3Q5QDN3A5CNFSM4JRBKMX2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIXVFDY#issuecomment-573526671>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA2QHQOIBT4Y2UAC6SWRHP3Q5QDN3ANCNFSM4JRBKMXQ> .

[sivaraam]

IIUC, the suggested change is that all the visit methods of the visitors would be extended to return a copy of the object they receive as the argument to help with implementing the copy visitor. Is that correct?

If it is then, is my understanding that this is useful only to those who want a deep copy of an object, correct? I'm just wondering whether this huge API change is worth the migration burden on the users who want to move to version 4 of the library? That's because they would have to change every extension of every visitor they would have implemented as a consequence of this change. That sounds like a lot of work and might not be a good thing to bring upon the users. Just my opinion.

[wumpz]

@sivaraam: that is correct. As well this change does not support replacement by different types, e.g. Column to literal or CaseExpression or you name it. That is why I am getting away from this. The problem here to solve IMHO the backreference to the parent object. How to replace an expression, while there is no reference to the parent? Now you have to adapt every possible parent. To make things worse you not only need the reference to the parent but to the attribute as well.

[sivaraam]

As well this change does not support replacement by different types, e.g. Column to literal or CaseExpression or you name it. That is why I am getting away from this.

Just for the sake of discussion, is it not possible to overcome this by making the visit methods return the appropriate super-classes? For example, in the case you mentioned we could just make the visit method corresponding to the Column of the appropriate visitor to return a generic Expression object and the implementation of the visit method could think about what type of Expression should be returned. I think this would work for any given visitor, and the objects it is visiting. Of course, this would possibly make it difficult to comprehend why the visit methods return a different type. But otherwise, does this sound like plausible solution?

That said, I still believe that modifying the existing visitor methods would place a huge migration burden on the users. To overcome this I think we can re-consider the following which was suggested before:

Would it be valuable to have CopyStatementVisitor and the current StatementVisitor ?

That would of course double the amount of visitor classes but at least it would not introduce any migration burden on the existing users. Also, having separate visitor classes would ensure that we don't add any additional (unrelated) responsibility to the visit methods. IOW, it would ensure SRP.

[wumpz]

So how to overcome this problems of the visitor pattern? Any ideas?

[SerialVelocity]

I agree with @sivaraam but would go one step further and make the extra visitor generic:

public <T> T accept(Generic*Visitor<T> visitor);

This would:

• Allow CopyVisitor to be written. Each visitor would be allowed to have its own type so you can have implements GenericStatementVisitor<Statement>, GenericExpressionVisitor<Expression>, etc.
• Allow custom types to be returned too. e.g. the TableVisitor could have T set to List<TableName> (this has the added benefit of easily making the class thread-safe because you no longer have a field lying around).

[AnEmortalKid]

So how to overcome this problems of the visitor pattern? Any ideas?

Would it be possible to invert things for consumption sake. Right now we have

interface SimpleVIsitor {

 void  visitFoo(Foo foo);
}

And the proposed change is:

interface SimpleVisitor {
Foo visitFoo(Foo foo);
}

What if we introduced the CopyVisitor as a lower level stack, so we could have:

interface CopyableVisitor {

 Comment copy(Comment comment);
}

interface SelectVisitor  extends CopyableVisitor {
default void visit(Comment comment) { copy(comment); }
}

Since this is java 8, we could use the default feature to make this at least easy for consumers?