vavr-io / vavr

vʌvr (formerly called Javaslang) is a non-commercial, non-profit object-functional library that runs with Java 8+. It aims to reduce the lines of code and increase code quality.
https://vavr.io
Other
5.68k stars 630 forks source link

narrow() naming #2407

Closed Abnaxos closed 5 years ago

Abnaxos commented 5 years ago

narrow() actually widens the type:

narrow() does the former, so its name is inverted. Or am I missing something here?

Proposals: widen(), maybe toCovariant(), …?

Abnaxos commented 5 years ago

asCovariant()

Abnaxos commented 5 years ago

Sorry about this, anything with "covariant" is an extremely bad call, because it does exactly the same mistake. We're talking about contravariants. It should toContravariant() for that matter. But somehow, this feels to technical. My new favourite proposal:

asSuper()

danieldietrich commented 5 years ago

Yes, you are right. The name is confusing. I thought in the sense of type inference, where the upper bound is lowered (however, this makes only sense in the presence of intersection types, which is not the case here). My brain tricked me, the type is widened, as you said.

I would not change the API in 0.x.

In 1.0, narrow() will disappear. It solves a type-related problem in a non-native way at runtime, which should be solved at compile time. It is a convenient method for casting.

I've learned in the past that it is better not to work around native Java. We always go with Java instead.

Abnaxos commented 5 years ago

I fully agree with going with Java, not against it.

But I think it's a very convenient method. I also think it's a severe design flaw in Java not to allow co/contravariant type variables as Scala does. I'm sure they discussed it, and they probably decided it would be too complex, but omitting this seems contra productive to me. Type declarations like Function<? super<? super T>, ? extends Option<? extends R>> aren't exactly easy to understand neither. Moving part if such declarations to the type variable itself would actually have simplified things.

But back to topic: it's perfectly fine to cast these types to a super type, so providing a method that allows such casts for well-known types without a compiler warning is a good thing, IMHO. After all, the compiler is wrong, it's accusing a perfectly safe cast of being unsafe). I don't feel that such a method is working against Java. Forcing the developer to spread @SuppressWarnings("unsafe") all over his code is, though.

The fact that Option is an interface makes it a bit dangerous, though. We might consider making Option an abstract class so we can declare the constructor package local so we can ensure that any implementation is actually covariant.

I think the method should stay, but it should be named correctly.

danieldietrich commented 5 years ago

Yes, in 1.0 Option is a sealed type. E.g. it is abstract, having a (package) private constructor. There are only two implementations. (In a future version of Java we might have real sealed types...).

We can't enforce the variance constraint in other places we have interfaces. For example collections or functional interfaces. Maybe it is better to offer the same API in all places: native Java.

Abnaxos commented 5 years ago

OK, you're probably right. Iterable, Traversable etc. aren't sealable and it would be inconsistent to provide such a utility for some types, but not for all. Rather discard it. Bummer.

I personally will introduce such methods because I like to isolate SuppressWarnings to exactly one statement where I know what I'm doing. It would be useful to have it in Vavr, but then again, that's exactly why I have my copy-paste pool where I collect such common utilities.

mperktold commented 5 years ago

How about upcast?

Because since Option is actually covariant (which Java doesn't know, unfortunately), that's really what happens here, and I think that's what the implicit conversion of non-generic types is usually called.

// implicit upcast
Integer i = Integer.valueOf(0);
Number n = i;

// explicit upcast
Option<Integer> optI = Option.some(i);
Option<Number> optN = Option.upcast(optI);

Sure, Java should be able to do this automatically, and it might in the future: http://openjdk.java.net/jeps/300

Until then, methods like this are a clever and useful trick. 👍

danieldietrich commented 5 years ago

I don't want to discuss about naming. powerthesaurus.org has plenty of options.

I think the original intent of narrow was misunderstood. It is all about narrowing the generic type bound from ? extends T to T.

More specifically:

List<? extends Number> given = ....;
List<Number> want = given;

Java's type system does not allow this kind of cast (explicitly) without casting given to Object beforehand.

Other statically typed language have a so-called unsound type system in order to allow such type conversions natively. In fact, there may occur problems in the presence of mutable objects. However, in our case, we only face immutable/persistent objects.


Back to topic: it is only a matter of interpretations what narrow means. Changing the name does mean breaking backw'compat - even if we deprecate the narrow method.

I want to keep all as-is and just clarify in the docs that the generic upper type bound is narrowed (aka restricted) to accept only exact types <T> instead of types that match the widened form <? extends T>.

Keep it simple!