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

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

Closed
kohlschuetter opened this issue Nov 20, 2023 · 3 comments

Comments

@kohlschuetter
Copy link

kohlschuetter commented Nov 20, 2023

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
Copy link
Collaborator

rluble commented Nov 21, 2023

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.

@rluble rluble closed this as completed Nov 21, 2023
@gkdn
Copy link
Member

gkdn commented Nov 27, 2023

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 added a commit to kohlschutter/j2cl that referenced this issue Nov 28, 2023
Sometimes, we want to have a class that has a vanilla Java
implementation, but in the context of j2cl/JavaScript it should have a
JavaScript-native implementation.

Right now, specifying a class with @jstype(isNative=true) prevents us
from doing that.

The official recommendation is to resort to "super-sourcing", which
means using different source code for each target environment
(Java/JavaScript). Unfortunately, this makes the development of shared
libraries unnecessarily difficult.

This change adds support for methods that have a new
@JsImplementationProvidedSeparately annotation. Such methods have their
method body for vanilla Java (as usual), but when transpiling to
JavaScript, the JavaScript code will be taken from where the rest of the
class (i.e., the other "native" methods) are defined.

This change only covers support for the jdt frontend and the closure
backend.

The annotation is defined elsewhere, and expected to be as follows:

@retention(RetentionPolicy.RUNTIME)
@target({ElementType.METHOD})
@documented
public @interface JsImplementationProvidedSeparately {
}

Also see
google#221
@kohlschuetter
Copy link
Author

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.

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

No branches or pull requests

3 participants