projectlombok / lombok

Very spicy additions to the Java programming language.
https://projectlombok.org/
Other
12.93k stars 2.4k forks source link

[FEATURE] Generating visitor pattern boiler plate code using @Visitor #2707

Open tko007 opened 3 years ago

tko007 commented 3 years ago

Describe the feature As a big fan of the visitor pattern, it would be easy to create and maintain the visitor interface using the lombok power.

Considering the visitor pattern, looks like as described in the following diagram: image

Proposal would be the following:

  1. Create manually the visitor interface empty body as a placeholder. This will hold the generated visit functions with the correct element types.
    public interface Visitor {
    ...
    }
  2. Create any concrete element and mark it with the @Visitor annotation, using the correct visitor interface type created in the first step. This can be done for N elements further.
    @Visitor(Visitor.class)
    public class ConcreteElementA extends Element {
    ...
    }
    ...
    @Visitor(Visitor.class)
    public class ConcreteElementN extends Element {
    ...
    }

    Still the concrete visitor needs to be implemented, but creating and maintaining the visitor interface functions can be handy for pattern users, whenever adding and removing elements from our object structure.

And the Vanilla code:

public interface Visitor {
   void visit(ConcreteElementA element);
   ...
   void visit(ConcreteElementN element);
}
public class ConcreteElementA extends Element {
   public void accept(Visitor visitor) {
      visitor.visit(this);
   }
}
...
public class ConcreteElementN extends Element {
   public void accept(Visitor visitor) {
      visitor.visit(this);
   }
}
Rawi01 commented 3 years ago

I may have missed something but I think this can be done using a regular annotation processor. Doing this in lombok is not that easy because it requires resolution.

tko007 commented 3 years ago

I was looking at annotation processors as well, but that approach can not modify the compiled code, you could have only the tree as read-only, and there is where lombok would come into play, to generate those functions for concrete elements and for the visitor interface. Could be that I am wrong, but giving exactly the type of visitor interface, that should work with lombok own resolver, or?

Rawi01 commented 3 years ago

Lombok cannot (or not that easy) handle things that require details about other files (e.g. adding a method to the Visitor interface, even if it is referenced properly). It can easily handle things in the same file (e.g. generating the accept method).

My previous statement that this can be done properly using a regular annotation processor seems to be wrong. It can generate the Visitor interface if you do not create a placeholder. The accept methods in ConcreteElementX are harder as they require a concrete type to call the right method. A solution for this problem might be a dispatching method like this one:

public interface Visitor {
    default void visit(Element element) {
        if (element instanceof ConcreteElementA) {
            visit((ConcreteElementA) element);
        }
        ...
    }
}

but thats somehow weird if you use the visitor pattern for double dispatching.

arpit-pp commented 3 years ago

Hi,

I've been spending some time over it at weekends. And have a working demo. I can draft a PR if this gets enough attention.

image
lotuc commented 3 years ago

Add mechanism for generating visit parameters for private fields would be nice.

@Visitable
public class Cat extends Animal {
  @Visitable
  private String a;
  private String b;
}

// or sth like

@Visitable(withFields={"a"})
public class Cat extends Animal {
  private String a;
  private String b;
}

-->

public interface AnimalVisitor<T> {
  T visit(Cat cat, String a);
}
arpit-pp commented 3 years ago

@lotuc I can't think of a usecase for this. Shouldn't this be possible since you already have the Cat object available in Visitor?

new AnimalVisitor<String>() {
    String visit(Cat cat) {
        return cat.getA();
    }
}
ladislavlencucha commented 3 years ago

What about visitor for enumerations?

@Visitor 
enum Animals {
  DOG,
  CAT
}

which would generate:

enum Animals {
  DOG {
    @Override
    public <T> T visit(Visitor visitor) {
      return visitor.visitDog();
    }
   },
  CAT {
    @Override
    public <T> T visit(Visitor visitor) {
      return visitor.visitCat();
    }
   },

  public abstract <T> T visit(Visitor visitor);

  class Visitor<T> {
    T visitDog();
    T visitCat();
  }
}
code-hunger commented 2 years ago

@arpit-pp looks nice, it's sad no one took interest in merging this so far, I think it can be helpful in a lot of times where the visitor pattern is used.

I just refactored some code that uses visitors and was somewhat disgusted by the amount of boilerplaty visit() methods on each child + the Visitor interface that could b generated.

I find it more easily comprehensible when all childs are defined as static nested classes of the parent, that is, in the same file, which leads me to this comment:

Lombok cannot (or not that easy) handle things that require details about other files (e.g. adding a method to the Visitor interface, even if it is referenced properly). It can easily handle things in the same file (e.g. generating the accept method).

I don't know if java folks write visitors with nested classes (I find it makes sense, otherwise the parent is uninformatively empty), but in that case, according to this comment, lombok should be perfectly able to do it?

abstract class Material {
    static class Iron { private static int someProperty; }
    static class Wood { private static int color; }
    /* Lombok could generate the Visitor interface (like arpit-pp showed)
      + all the necessary visit/accept methods
      (maybe take care of static modifiers on the children too) */
}
code-hunger commented 2 years ago

Actually, on a first sight, in case the children are nested classes of the parent, it seems to me that generating the visitor interface+methods should be not much different from generating the nested Builder class + adding the appropriate builder() method on the parent.

rzwitserloot commented 2 years ago

I like where this is going. Before I sign up to support this for the next 10 years or so I'd like to think the feature through fully. What about this plan: @rspilker and I discuss this next Thursdayevening, and hopefully we get this into 'we'd accept a PR' territory?

Rawi01 commented 2 years ago

The initial example by @arpit-pp requires resolution, the generated AnimalVisitor contains a method that references Cat but that type is not mentioned in the base class. If everything is in the same file it should be possible to generate everything.

rzwitserloot commented 2 years ago

@Rawi01 Oof, I missed that. Now I'm wondering how @arpit-pp 's prototype managed that.

arpitsingla96 commented 2 years ago

@rzwitserloot @code-hunger Sorry I don't own the @arpit-pp Github ID anymore and just saw the above replies.

I think my example with Animal and Cat only worked because they were in the same package. Moving Cat to a different package results in compilation failure. I think one way to solve it would be to tell lombok explicitly about sub-types and super-type for visitor generation with the annotation itself. For example:

@Visitor(child = {Cat.class})
public interface Animal {
}
@Visitable(for = Animal.class)
public class Cat implements Animal {
}
dschulten commented 1 year ago

@rzwitserloot is there any chance to move this into the "we'd accept a PR" territory? @arpitsingla96 do you still have the prototype/can you publish it?

arpitsingla96 commented 1 year ago

@dschulten Here is the draft I had worked upon. https://github.com/arpitsingla96/lombok-extensions I'm happy to make a PR once I have the blessing of @rzwitserloot 😄

RobertDiebels commented 6 months ago

Is this FR still on the radar? Or did this land in "we won't accept a PR" territory? Asking because I keep coming back to this ticket every few months and I would love to see this happen.

RobertDiebels commented 6 months ago

I don't want to be that guy, but since no-one else seems to want to be that guy either and given that it's been a long time, maybe I'll be forgiven for this shameless bump and mention.

Any chance of getting a response from @rzwitserloot regarding the "PR-territory" question?