java.util collection subclasses may warn if it specializes generic type and override the methods

[niloc132]

This may be by design, but is a bit irritating for an app which doesn't intend to expose java collections to JS. Example .java file:

package j2cl.samples;

import java.util.AbstractList;
import java.util.ArrayList;

public class App {

    public static void main() {
        MyList list = new MyList();
        list.add(new Item());
        list.get(0);
    }

}

class Item {

}

class MyList extends AbstractList<Item> {
    private ArrayList<Item> backingList = new ArrayList<>();
    public Item get(int index) {
        return backingList.get(index);
    }
    public int size() {
        return backingList.size();
    }
}

J2cl output:

Warning:App.java:22: [unusable-by-js] Return type of 'Item MyList.get(int index)' is not usable by but exposed to JavaScript.
Suppress "[unusable-by-js]" warnings by adding a `@SuppressWarnings("unusable-by-js")` annotation to the corresponding member.

Adding a warning could be a workaround, but only applies when the application author can control the original java.

While I recognize that this message is technically correct on its surface, it is also misleading in that it suggests that simply declaring the field ArrayList<Item> backingList = new ArrayList<Item>() is somehow safer to be exposed to JS. The message also isn't useful in cases where there is no jsinterop-annotated method/property which can expose this type to JS in the first place.

Similarly I recognize that we can't expect to completely analyze which types are actually exposed to JS and where, but perhaps it would make more sense to phrase these in terms of where the List type is exposed rather than only how subtypes and not instances are declared?

[gkdn]

If you put ArrayList or List on a JS accesible method, you will get a warning. A type declaration like ArrayList by itself is not JS accessible doesn't expose a danger. So this is working as expected.

And if you just extend AbstractList without overriding get method that introduces unsafe type; it will work exactly like above.

However in this particular case, user explicitly overrides a JS accessible method and return a type that is not JS friendly. Even if that's not the intention of the user MyList's unsafe method will be visible from JS.

I can't think of a good way to downgrade these checks.

@rluble any ideas?

[rluble]

I think the problem lies on the fact that we assume that T is always accessible by Js, but there are not many good alternatives either.

I think that if you instantiate T to a non accessible-by-js class then methods that specialize T should not be checked unless they specifically have a @JsMethod (or the type a JsType via standard JsType rules).

This would not be a perfect solution either since you could pass an instance of MyList as List to js.

[stockiNail]

I have got the same warnings that @niloc132 reported into issue. I see that the transpiler emits a suggestion:

Suppress "[unusable-by-js]" warnings by adding a `@SuppressWarnings("unusable-by-js")`
annotation to the corresponding member.

Is this a workaround to avoid warnings, waiting for what @rluble suggested or could be figured out as the solution? I'm asking that because usually I don't linke to use SuppressWarnings annotation if I can. Thank a lot

[rluble]

I don't think there is a perfect solution. It is unclear whether the intention of the programmer is that the class MyList is meant to be passed to JavaScript or not.

We could maybe not emit a warning on declared members of a class that does not have any interop annotation, but that will also miss the other case when you subclass and only need to override a few methods and not necessarily need to add annotations because a supertype already provides those.

This is the scenario that @SuppressWarnings is designed to cover. It is always better to emit warnings that are suppressible than not.

[niloc132]

That being the case, it seems like any declaration of List<NonJsType>, whether or not it is exposed, should be warned then? The risk is the same, if I understand you correctly, to making your own MyList extends AbstractList<NonJsType> and not exposing it to js through any jsinterop-annotated methods/fields.

Or perhaps I'm misunderstanding and you're saying "yes, this is definitely a false positive, but we don't have a good way to shut it down"?

[rluble]

Or perhaps I'm misunderstanding and you're saying "yes, this is definitely a false positive, but we don't have a good way to shut it down"?

I don't consider this a false positive. It is just difficult to guess the programmer intention. I don't think the situation of just using the type List<NonJsType> is equivalent to extending it and implementing a@JsMethod which is the case in the example (even though in both cases you'd end with a @JsMethod that returns a type that is not js-accessible).

The only way to have a precise error here would be to do a global analysis which would also be conservative, cannot be done in the J2CL compilation and is probably a costly overkill.

As I suggested we could consider classes that do not have any annotations exempt of the check even if they override JsMethods. But I am sure that also in taking this approach we will not be perfect either.

IMHO, it is a warning. If it does not apply to a concrete case the you should suppress it.

[stockiNail]

@rluble Finally I'm using the suppress warning annotation, because developing a library and I don't want the users will come asking about that warning.

I understand that to transform the warning into an error, checking if used by js, it's a tough task. My suggestion is to document it well in order that whoever will face it, can understand well why that warning is emitted and understand if it must ignore or not.

IMHO, it is a warning. If it does not apply to a concrete case the you should suppress it

Let's imagine you are driving to your car and a yellow light comes on. Maybe the first thing you are doing is to read the car manual where the reason is not clearly written but it's written that if you want to ignore, you can stick black scotch tape on top. Then maybe you are thinking that's something strange and you go to the garage where they will tell you that a sensor can not recognize well and you can ignore. What would you really think about?

Anyway I agree with you this is not a simple topic to address. Wherever you go, there will be someone not happy for that.

[gkdn]

That being the case, it seems like any declaration of List, whether or not it is exposed, should be warned then? The risk is the same, if I understand you correctly, to making your own MyList extends AbstractList and not exposing it to js through any jsinterop-annotated methods/fields.

As I explained earlier, a type declaration like ArrayList by itself doesn't expose a danger. It is not a problem unless you put ArrayList or List on a JS accesible method. Here we can easily understand the intention and with the absense of other evidence we can confident to not warn about it.

In the other case reported in the bug, we cannot identify the user's intention since it explicitly creates and exposes the method at the time of declaration.

[gkdn]

@stockiNail To follow your analogy, there are sensors in the car or other safety sense that "warn" you to get you attention; parking sensors, freeze sensors etc. If you get out of the car and see if there is no freeze you won't put a black scotch tape on it (hopefully :)).

I can find many other analogies but the point is; this is a warning that you need to pay attention to and decide yourself if you choose to ignore it or not; and if you choose to ignore it, it is best to document it where you have added to suppression.

[gkdn]

BTW, I will correct the title. Concrete java.util collection implementations doesn't always warn; you won't see such warnings in Guava. You are getting a warning due to the template 'specialization' in the subclass.

[rluble]

BTW, I will correct the title. Concrete java.util collection implementations doesn't always warn; you won't see such warnings in Guava. You are getting a warning due to the template 'specialization' in the subclass.

The real cause is that the class overrides a JsMethod. You could extend a class and specialize the type variable and still will not warn unless you add a JsMethod (explicitly or implicitly by overriding one as in this case) with parameters/returns that are not usable by js.

[gkdn]

Agreed, that's what I meant. pls see the title.

[stockiNail]

@gkdn apologies if I looked like disrespectful, really I didn't want. I have just explained what I thought. Sorry.

The real cause is that the class overrides a JsMethod. You could extend a class and specialize the type variable and still will not warn unless you add a JsMethod (explicitly or implicitly by overriding one as in this case) with parameters/returns that are not usable by js.

Maybe I'm wrong and getting so old but I'm not able to see the case in my classes (here).

The type variable of the list is an Integer and I don't override any JsInterop methods. And the base List class implements the List interface, without extending existing List implementation.

Anyway as said, I've added suppress warning annotation to avoid those warning.

Thanks a lot!

[niloc132]

@stockiNail Google (not gwtproject) requires that java.util.List itself have jsinterop annotations on the emulation for List.java and a few other java.util collections for their own reasons, so this is not something which can be avoided in your own plain java.

[stockiNail]

@niloc132 ah ah! I wasn't aware at all about that. I was assuming the interface was a JsType object. Now I can understand why @rluble was talking about "override" of JsMethod because I was getting crazy to find out where I was using it.... And I can understand the warning as well because finally I'm using a JsType object... Thank you guys, now it's really clear to me and apologize if I needed more time to understand it.

May I propose if this could be documented in some pages (like done for Enum emulation)?

[gkdn]

@stockiNail it didn't sound disrespectful; sorry if I made sound like that.

May I propose if this could be documented in some pages (like done for Enum emulation)?

Sure but I'm not sure what you are referring to with "like done for Enum emulation".

@rluble For the warning maybe we can point that this is coming from override in the message.

[stockiNail]

@gkdn it's my bad English. Sorry. English is not my mother tongue therefore sometimes I can not explain well and with the right words what I have in my mind.

About proposal, 3 weeks ago I've started going in deep into J2CL and the first thing I did was to read the available doc into repo, getting-started.md and limitations.md. Into limitations.md there is a paragraph called No Support for Enum Reflective APIs. That's what I meant. I was thinking that in the same doc file we could add some sentences how java.util interfaces (or other java emulation classes as well) are JsType and they are expecting to collect JsType instances as elements and a warning will be emitted if the object is not JsType and then not usable into a js. This is because they are thought to be used a List (or map) into JS part as well.

This is a difference comparing with a normal JAVA usage of collections where you can use whatever object you want. But after a long walking, I was also thinking that maybe it's only my fault because I didn't go so in deep and I'm the only one who faced this warning. And anyway this issue is anyway documentation therefore whoever will have the same wanrning can read here why.

Thanks a lot!

[gkdn]

I was thinking that in the same doc file we could add some sentences how java.util interfaces (or other java emulation classes as well) are JsType and they are expecting to collect JsType instances as elements and a warning will be emitted if the object is not JsType and then not usable into a js.

But that's no true. You can use whatever you want with the collections. The warning is coming in only the case if you do 3 things together:

1. Extend the collection class.
2. Specialize the parent class to use a non-JsType
3. Override a method like get and similar.

That's not something that users do a lot.

[stockiNail]

@gkdn yeah! Agree!