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

Add T[] Value.toJavaArray(T[]) #1582

Closed
danieldietrich opened this issue Sep 24, 2016 · 6 comments
Closed

Add T[] Value.toJavaArray(T[]) #1582

danieldietrich opened this issue Sep 24, 2016 · 6 comments

Comments

@danieldietrich
Copy link
Contributor

(See #1578)

These are our options:

// unsafe
T[] toJavaArray(Class<? super T> componentType) // one size fits all

vs

// safe
T[] toJavaArray(Class<T> componentType) // variant for non-generic types
T[] toJavaArray(T[] array)

I think the first, unsafe variant is no viable solution.

Example:

// before
Value<CharSequence> v = Option.of("");
CharSequence[] s = v.toJavaArray(CharSequence.class);

// after
Value<String> v = Option.of("");
String[] s = v.toJavaArray(CharSequence.class); // will eventually throw at runtime

The existing method toJavaArray(Class<T>) is still useful for non-generic types,
we do not need to deprecate it.

@l0rinc
Copy link
Contributor

l0rinc commented Sep 24, 2016

T[] toJavaArray(T[] array)

Because of erasure I don't think it's possible to make this completely type-safe.

I think the first, unsafe variant is no viable solution.

Doing it the Java way would result in the same unsafety

@SuppressWarnings("unchecked")
Option<Integer>[] options = singletonList(Option.some(1)).toArray(new Option[0]);
System.out.println(options[0].get());

i.e you cannot call toArray with new Option<Integer>[0] either ...

@danieldietrich
Copy link
Contributor Author

@paplorinc you are right - it makes no sense to have toArray(T[]). That is the reason I introduced the simpler toArray(Class<T>).

If an application of toArray(Class<T>) compiles, it is correct. Therefore this is the best solution we can come up with.

For now, we will not implement the unsafe case, I don't see the urge to do it. We have many other things to do.

@l0rinc
Copy link
Contributor

l0rinc commented Sep 25, 2016

If an application of toArray(Class) compiles, it is correct.

Unfortunately that's not true either:

Array.of(1).toJavaArray(int.class);

errs at run time with:

java.lang.ClassCastException: [I cannot be cast to [Ljava.lang.Object;

I still think the <? super T> variant was the best compromise.

@danieldietrich
Copy link
Contributor Author

I'm not sure. Is it possible to fix our variant, e.g. by first checking type.isPrimitive() and then decide further how to proceed?

I will play around a bit.

Safety first.

@l0rinc
Copy link
Contributor

l0rinc commented Sep 25, 2016

checking type.isPrimitive() and then decide further how to proceed?

and throw a different kind of RuntimeException?

Safety first.

I agree, but I don't think it's possible, because of type erasure, it's inherently unsafe :/

@danieldietrich
Copy link
Contributor Author

See #1585

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

No branches or pull requests

2 participants