Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEATURE] Make the @SuperBuilder's *BuilderImpl class public #3791

Open
ilia1243 opened this issue Dec 2, 2024 · 5 comments
Open

[FEATURE] Make the @SuperBuilder's *BuilderImpl class public #3791

ilia1243 opened this issue Dec 2, 2024 · 5 comments

Comments

@ilia1243
Copy link

ilia1243 commented Dec 2, 2024

Describe the feature

@SuperBuilder's *BuilderImpl class is currently private, and cannot be used as a generic type argument. Making it public will allow to use it in the following way:

@SuperBuilder
abstract class Base {
}

@SuperBuilder
class Derived extends Base {
}

abstract class BaseService<T extends Base, B extends Base.BaseBuilder<T, B>> {
    abstract B newBuilder();
}

class DerivedService extends BaseService<Derived, Derived.DerivedBuilderImpl> {
    @Override
    public Derived.DerivedBuilderImpl newBuilder() {
        return Derived.builder();
    }
}

Note: this will also allow for .builder() to return *BuilderImpl class instead of *Builder<?, ?>. This will likely fix issue #3445.

Workaround.

The above code will compile if the derived class has customized .builder() method, and customized *BuilderImpl class.

@SuperBuilder
class Derived extends Base {
    public static DerivedBuilderImpl builder() {
        return new DerivedBuilderImpl();
    }

    public static class DerivedBuilderImpl extends DerivedBuilder<Derived, DerivedBuilderImpl> {
    }
}
@ilia1243 ilia1243 changed the title [FEATURE] Make the @SuperBuilder's *BuilderImpl class public to use as type argument [FEATURE] Make the @SuperBuilder's *BuilderImpl class public Dec 2, 2024
@janrieke
Copy link
Contributor

janrieke commented Dec 3, 2024

Yeah, well, but where is the use case? I don't understand why you'd want to do this.
However, to be fair, probably nothing will convince me that this is a good idea. Making that BuilderImpl visible for the world will create a lot of confusion. But if you'd describe your use case, we may figure out another way that doesn't lead to users having a thousand ways of messing up a @SuperBuilder.

@ilia1243
Copy link
Author

ilia1243 commented Dec 3, 2024

I was not accurate enough. In the issue description, it is possible to use abstract builder class with wildcards.

abstract class BaseService<B extends Base.BaseBuilder<?, ?>> {
    abstract B newBuilder();
}

class DerivedService extends BaseService<Derived.DerivedBuilder<?, ?>> {
    public Derived.DerivedBuilder<?, ?> newBuilder() {
        return Derived.builder();
    }
}

But BaseService.newBuilder() cannot be used inside the BaseService itself. Consider a bit more complex example:

@SuperBuilder
public abstract class Base {
    private boolean base;
}

@SuperBuilder
public class Derived extends Base {
}

public abstract class BaseService<B extends Base.BaseBuilder<?, ?>> {
    protected abstract B createBuilder();

    public B newBuilder() {
        return createBuilder()
                .base(true);
    }
}

public class DerivedService extends BaseService<Derived.DerivedBuilder<?, ?>> {
    @Override
    protected Derived.DerivedBuilder<?, ?> createBuilder() {
        return Derived.builder();
    }
}

This does not compile with error in BaseService class:

BaseService.java:8: error: incompatible types: CAP#1 cannot be converted to B
                .base(true);
                     ^

If using recurring generics (this fixes the compilation error in BaseService) class BaseService<B extends Base.BaseBuilder<?, B>> I came to the problem in the issue description. Abstract builder class cannot be used as a type argument in the DerivedService. For example, class DerivedService extends BaseService<Derived.DerivedBuilder<?, ?>> produces:

DerivedService.java:3: error: type argument DerivedBuilder<?,?> is not within bounds of type-variable B
public class DerivedService extends BaseService<Derived.DerivedBuilder<?, ?>> {
                                                                      ^

I currently see only one way to fix this by making DerivedBuilderImpl public and using as type argument class DerivedService extends BaseService<Derived.DerivedBuilderImpl>, though probably missing something.

@janrieke
Copy link
Contributor

janrieke commented Dec 3, 2024

The issue with using DerivedBuilderImpl as type argument at that point is that you cannot have another sub-class SubDerived extends Derived with an associated "sub-service", e.g. SubDerivedService extends DerivedService. It does not work because the SubDerivedBuilderImpl is not a sub-class of DerivedBuilderImpl, but only of DerivedBuilder.

@ilia1243
Copy link
Author

ilia1243 commented Dec 3, 2024

Here is the way to have deeper inheritance (Base and BaseService are omitted for brevity).

@SuperBuilder(builderMethodName = "derivedBuilder")
public class Derived extends Base {
}

@SuperBuilder(builderMethodName = "subDerivedBuilder")
public class SubDerived extends Derived {
}

public abstract class AbstractDerivedService<B extends Derived.DerivedBuilder<?, B>> extends BaseService<B> {
}

public class DerivedService extends AbstractDerivedService<Derived.DerivedBuilderImpl> {
    @Override
    protected Derived.DerivedBuilderImpl createBuilder() {
        return Derived.derivedBuilder();
    }
}

public class SubDerivedService extends AbstractDerivedService<SubDerived.SubDerivedBuilderImpl> {
    @Override
    protected SubDerived.SubDerivedBuilderImpl createBuilder() {
        return SubDerived.subDerivedBuilder();
    }
}
  • This still requires to have BuilderImpl public in both Derived and SubDerived (I omitted this W/A in the code above). builder() method also returns BuilderImpl in both classes.
  • I had to use different builderMethodName in Derived, and SubDerived. Otherwise, I cannot return BuilderImpl due to compile error error: builder() in SubDerived cannot hide builder() in Derived ... return type SubDerivedBuilderImpl is not compatible with DerivedBuilderImpl.

In fact, it seems unnatural, as we do not really have instances of Animals, we have instances of Cats that we can work with as Animals. I mean, in practice Derived will likely also be abstract, and there will be no final DerivedService in this case. This way there will be no need to have different builderMethodName.

@ilia1243
Copy link
Author

ilia1243 commented Dec 4, 2024

I had to use different builderMethodName in Derived, and SubDerived.

That said, to preserve the compatibility, there can be two builder methods generated

@SuperBuilder
public class Derived extends Base {
    // This is exactly the same as it is generated now
    @Generated
    public static DerivedBuilder<?, ?> builder() {
        return new DerivedBuilderImpl();
    }

    // This returns *BuilderImpl, and has name formed from the name of Derived class.
    @Generated
    public static DerivedBuilderImpl derivedBuilder() {
        return new DerivedBuilderImpl();
    }

    @Generated
    public static final class DerivedBuilderImpl extends DerivedBuilder<Derived, DerivedBuilderImpl> {
        ...
    }
}

For compatibility, before generating check that custom derivedBuilder is not present in the class.

Alternatively, generate one builder method the same as it is generated now, but make BuilderImpl constructor public. This will provide another alternative how to instantiate the builder.

@SuperBuilder
public class Derived extends Base {
    // This is exactly the same as it is generated now
    @Generated
    public static DerivedBuilder<?, ?> builder() {
        return new DerivedBuilderImpl();
    }

    @Generated
    public static final class DerivedBuilderImpl extends DerivedBuilder<Derived, DerivedBuilderImpl> {
        @Generated
        public DerivedBuilderImpl() {
        }
        ...
    }
}

public class DerivedService extends BaseService<Derived.DerivedBuilderImpl> {
    @Override
    protected Derived.DerivedBuilderImpl createBuilder() {
        return new Derived.DerivedBuilderImpl();
    }
}

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants