Replace Optional<X> with Optional<? extends X>

[ST-DDT]

I encountered a little problem while looking at the Sponge Implementation and extending existing events.

The issue
I can overwrite every method to return a more precise result than the base interface specifies. However using the Optional destroys that benefit if used the way we currerntly do.

Here is an example code

public interface Test {

    Optional<Test> getSelf();

    Optional<? extends Test> getBetterSelf();

}

and the second file

public class Proof implements Test {

    @Override
    // public Optional<Proof> getSelf() { - NOT POSSIBLE
    public Optional<Test> getSelf() {
        // return Optional.of(this); - NOT POSSIBLE
        return Optional.of((Test) this);
    }

    @Override
    public Optional<? extends Proof> getBetterSelf() {
        return Optional.of(this);
    }

    public void foo() {
        Proof self = getBetterSelf().get();
        // Proof self2 = test.getSelf().get(); - NOT POSSIBLE
        Proof self2 = (Proof) test.getSelf().get();
    }

}

Image the Sponge server itself has a specific subclass for one of its events, which uses a subclass of the original class/interface because a method was added that is required for further use. So if you listen to the given event and you try to get it, you will actually get the base interface because you have to downcast it, but if you use the <? extends T> for example this is no longer an issue you class can simply overwrite the return type and everything will be fine.

If we change it we have basically three benefits:

• Avoid unnecessary casting while creating the Optional
• Avoid unnecessary casting while getting the Optional's value
• Allow other APIs to extend our API more safely and freely.

I suggest replacing/changing it in the entire API (where the Optional references a class within the org.spongepowered.api.** package) , i also offer my help to change it.
This would also include immutable Iterables, Collections, ...., Maps.

[stephan-gh]

I can confirm this is a problem for some parts of the API, but we need to check where having more precise results are really need it. I had to use it in #367 in order to be able to override the return value in StatusPingEvent for a method with Optional. https://github.com/SpongePowered/SpongeAPI/pull/367/files#diff-70bb07711fbf5aabad54031ef7fd11aaR59

[ST-DDT]

IMO it is need everywhere or at least useful everywhere.
Think of CopyPasting the entire API and just replace all interfaces with implementations, including those in the optionals.
Think of CopyPasting the entire API and just extend the SpongeAPI in every of your interfaces, then you have GrantiteItemStacks or GlowKitHorse with just a few additional methods. This will include the return types of any Optionals as well.
In some cases you may not be able to declare the extended optionals (events with setters) but in that cases it does not hurt either, does it? Well server impls with their own API may want to overwrite it anyway and just cast it on set, because they can be sure that it the Entity reference is always a valid GlowKitEntity.

@Minecrell That piece of code actually triggered my interest in this topic in combination with SpongeGameRegistry

[stephan-gh]

I don't think you'll want (nor would it make sense) to have something like Optional<? extends String> or Optional<? extends Integer>. ;)

[ST-DDT]

@Minecrell Thats true. So everywhere is not entirely true.
With everywhere i actually meant that i would use a regex to search for occurrences and then replace it on the fly if necessary. Basically all places that use a class with org.spongepowered.api.* as package for the optional type. Sorry for not being precise enough.

[stephan-gh]

It wouldn't be possible to use this in the SpongeGameRegistry methods. The cast in https://github.com/SpongePowered/Sponge/blob/master/src/main/java/org/spongepowered/mod/registry/SpongeGameRegistry.java#L200 couldn't be prevented using this because the internal instances of Minecraft will only be patched with the mixins at runtime (AFAIK), therefore you couldn't return a more precise value there, the compiler will say that they don't extend the Sponge interfaces. So they wouldn't match Optional<? extends ItemType> at compile time.

[ST-DDT]

You are right once again.
But there are things in the Impl that will benefit from it. All those with a map in that class. In the other cases there will/may already be other server implementations that will bemefit from it.

[ST-DDT]

@Minecrell Shall i start working on it or is this something for the team to do?
I would do it for the impl side as well.
Notice: This would be a huge change, because it also affects Iterable/Collection/Set/List/Map....

[stephan-gh]

@ST-DDT We need some more discussion on this first.
@SpongePowered/developers Any other opinions on this?

[Lunaphied]

I'm not sure I understand the issue, can someone explain it more clearly?

[ST-DDT]

The issue is only an issue in terms of causing unnessesary casting. Lets say we are in the Sponge server impl. And we have a method getVehicle() that returns an Optional<Entity>. If we don't use the ? extends here we have to explicitly cast our SpongeEntity to Entity before wrapping it in an Optional otherwise it won't match the return type. On the other side we are also very sure that everys SpongeEntitys vehicle is a SpongeEntity as well. We may have to call an internal method there from time to time and we have a nice method that gets the vehicle from our entity but the return type is only Entity inside an Optional so we have to cast the returned vehicle to SpongeEntity before we can call our internal method.
So we have basically two casts just to stick to the API without resulting in any benefits for the API, the server impl or plugins that use the methods.

class Entity {

Optional<Entity> getVehicle();

}

class SpongeEntity implements Entity {

SpongeEntity vehicle;

Optional<Entity> getVehicle() {
    return Optional.fromNullable((Entity) vehicle);
}

void internal() {
    ((SpongeEntity) getVehicle().get()).internal();
}


}

The two unnessesary casts should be clearly visible here.

But if you define the method with Optional<? extends Entity> in the API you can either use the SpongeEntity implementation above or you could return Optional<SpongeEntity> or Optional<? extends SpongeEntity> and avoid the casting then.

class Entity {

Optional<? extends Entity> getVehicle();

}

class SpongeEntity implements Entity {

SpongeEntity vehicle;

Optional<SpongeEntity> getVehicle() {
    return Optional.fromNullable(vehicle);
}

void internal() {
    getVehicle().get().internal();
}

}

No casts and better code readibility for the server impl.

But there are also benefits for plugins although not in the example above.
Plugins will mostly benefit from this in events that return Optionals in some methods.
Lets asume there is an EntityEquipedEvent in the API. Like above there is a method returning Optional<Entity> called getEquiper(). The plugin provides a EntityEquipedByPlayerMagicEvent. The Equiper is always a Player in this case. But due to the fixed optional type you have to cast it to Entity to return it.
In addition to that you have an EventListener that should increase the players stat every time he equips an Entity using magic. Inside the event listener method you have to cast the returned Optional<Entity> player to Player again to add 1 to the players stats. Two casts with none of them needed but with an increased chance of encountering errors.
The example / solution is mostly the same as above so i wont write it another time.

[Lunaphied]

I somewhat prefer not having this mainly because its so incredibly long
with optional returns as it is. Using an additional <? extends X> is extra
long compared to a plain . However unless I misunderstand something,
internally Java does not know that <? extends X> is really a so it
won't actually solve this problem anyway.

Correct me if I am wrong there. If so an explanation of what's wrong with
that interpretation would be appreciated.

[octylFractal]

@modwizcode The point is not to make Java think that Optional<? extends X> is a Optional<Y>, but rather so we can specify that in subclasses of Events (Sponge implementations) and avoid casts that are dangerous.

[Lunaphied]

Ah I misunderstood the meaning of X. I'm in favor of this for the most
part. Although it seems like variable definitions will be kind of long.
Which is something I already dislike about Optionals.

[octylFractal]

Let's compare an example I wrote up from the API.

Cause.java has Optional<Cause> getParent():

    public Optional<Cause> getParent() {
        return parent;
    }

We now subclass Cause into PlayerCause (example, class does not exist) which wants to return itself as the parent cause (for some unknown reason, this is a bad example), but we can't change the return value!

PlayerCause.java

    @Override
    public Optional<PlayerCause> getParent() { // does not compile ;(
        return parent;
    }

Now, with a revised Cause.java:

    public Optional<? extends Cause> getParent() {
        return parent;
    }

Here's
PlayerCause.java

    @Override
    public Optional<? extends PlayerCause> getParent() {
        return parent;
    }

No problems! Yay!

I think we can sacrifice a few chars in places for compiling.