google / j2cl

Java to Closure JavaScript transpiler
Apache License 2.0
1.22k stars 143 forks source link

"Native JsType method '...' should be native, abstract or JsOverlay." error prevents default implementation for vanilla Java #221

Closed kohlschuetter closed 8 months ago

kohlschuetter commented 8 months ago

Describe the bug I'm trying to write a Java class that can be used both in vanilla Java as well as in a JavaScript environment via j2cl.

I have a java interface with a static newDefaultInstance-style method that provides an instance of a class implementing interface. The implementation details of that class are JVM-specific, potentially using Java SPI to find the right implementation, etc. Therefore, I want an optimized implementation when running my application via j2cl.

I provide this optimized JavaScript implementation by annotating the interface with @JsType(isNative = true), through a custom JavaScript file that I supply to j2cl/jscomp.

Unfortunately, j2cl complains that my Native JsType method '...' should be native, abstract or JsOverlay..

However, when I simply disable that check in JsInteropRestrictionsChecker.checkMemberOfNativeJsType, everything compiles and works just fine.

I wonder if that check is required at all, since the code in the static method is actually not transpiled.

Maybe we could add a new annotation to signal that the method's implementation is meant for vanilla Java only, but not for JavaScript environments — essentially the opposite of @JsOverlay. Maybe something like @JsImplementationProvidedSeparately?

To Reproduce

Java code:

package com.google.j2cl.samples.helloworldlib;

import java.util.HashMap;
import java.util.Map;

import jsinterop.annotations.JsType;

@JsType(isNative = true, namespace = "kohlschutter.coding", name = "Dictionary")
public interface Dictionary<V> {
  boolean containsKey(String k);

  V get(String k);

  void put(String k, V v);

  void remove(String k);

  public static <V> Dictionary<V> newDictionary() {
    Map<String, V> map = new HashMap<String, V>();
    return new Dictionary<V>() {

      @Override
      public boolean containsKey(String k) {
        return map.containsKey(k);
      }

      @Override
      public V get(String k) {
        return map.get(k);
      }

      @Override
      public void put(String k, V v) {
        map.put(k, v);
      }

      @Override
      public void remove(String k) {
        map.remove(k);
      }
    };
  }
}

JavaScript:

goog.module("kohlschutter.coding.Dictionary");

class Dictionary {
    constructor() {
        this.dict = {};
    }

    containsKey(k) {
        return typeof this.dict[k] != "undefined";
    }

    get(k) {
        return this.dict[k];
    }

    put(k, v) {
        this.dict[k] = v;
    }

    remove(k) {
        delete this.dict[k];
    }

    static newDictionary() {
        return new Dictionary();
    }
}

exports = Dictionary;

and then something like

var Dictionary = goog.require('kohlschutter.coding.Dictionary');
Dictionary.newDictionary();

Expected behavior The class should transpile correctly.

rluble commented 8 months ago

This is normally achieved by what we call super-sourcing, where you pass different versions of the source when building for different targets. You can look at the way j2cl provides different implementation of some of the JRE classes for wasm (search for super-wasm).

In general, native types should only be present for web pathway.

gkdn commented 7 months ago

Maybe we could add a new annotation to signal that the method's implementation is meant for vanilla Java only, but not for JavaScript environments

There is GwtIncompatible which is shortcut for super-sourcing to enable reuse but here it seems like you want the method to be available in all environments in someway (i.e. "native" in JavaScript and implemented behavior in Java for JVM).

For something like that, you can delegate the static method interface to a class (e.g. Platform.java) where you have different version for specific platforms like rluble described. Or you can put factory method to another class similar to Guava where you can have different implementation for the factory itself (Dictionaries.newDict). These approaches make it clear to the user what is going on without adding another magical behavior hidden by annotations.

kohlschuetter commented 7 months ago

The "super sourcing" approach is something that doesn't work well for libraries, because you would have to provide different dependencies for each of your environment targets, which is a big code smell, in my opinion.

See commit a5b37d6 for a fix in my fork.

However, I understand that this change may not be a focus for mainline j2cl, so I'm only adding this reference for posterity.