cincheo / jsweet

A Java to JavaScript transpiler.
http://www.jsweet.org
Other
1.45k stars 160 forks source link

Vararg overloading method ordering #227

Open raiju opened 7 years ago

raiju commented 7 years ago

First of all, thanks for building JSweet! It's been a very pleasant experience this far.

While using JSweet to transpile code generated by Immutables, it generates invalid code on varargs overloaded methods: https://gist.github.com/raiju/3207af5a68644ab9c9ce6af0e83bf3a5 (note that, due to the spread operator, the checks on line 39 are being done on the array of arguments, and on line 40 the array is passed into a method expected a single argument).

Side-note: if the non-vararg method is first, it generates correct code.

This was using the jsweet gradle plugin, with jsweet-transpiler v1.2.1.

If you think it's reasonably bounded I'd be happy to take a stab at fixing this, but would appreciate being pointed in the right direction.

renaudpawlak commented 7 years ago

Sorry for responding so late!

I think that a known limitation with varargs and overloading... I kept this for later because it looked quite complex to fix. Varargs by themselves are already quite complex because we apply some translations to simulate the Java behavior.

So, if you still want to help, here is the process.

  1. Write the test that breaks the expected behavior (see other tests to know how to write a test in JSweet - some tests only use the compile test method, some tests use the eval test method). Here you probably want to write an eval test because you want to check that the program does not only compile, but also is running as expected.
  2. Fix the test by changing the way JSweet generates the TypeScript code. All is in Java2TypeScriptTranslator. Method declarations are printed starting here: https://github.com/cincheo/jsweet/blob/master/transpiler/src/main/java/org/jsweet/transpiler/Java2TypeScriptTranslator.java#L1628 (I have warned you, it's not simple). To know where varargs are dealt with, you can lookup the references to the Util.isVarargs function, which tells if a variable is a vararg parameter.
  3. Of course, make sure all other test still work before submitting a final PR (you can submit intermediate PRs to show WIP).

Last but not least, you need to work on version 2. Version 1.2.1 will be kept as is because we don't have the resources to maintain several versions (we will leave to 1.2.1 users the task to apply a patch if they want to keep using that version).

If you don't manage to fix the the problem, it is still good to have a test, so you could push the test only. It is still useful.

raiju commented 7 years ago

Sorry for the late reply, just got back from vacation. Thanks for the very complete reply. We have however migrated away from using JSweet (we now use scala.js to solve the particular problem we were facing), so I'm afraid I won't be able to put in the time this would need.

renaudpawlak commented 7 years ago

No problem. Thanks to catching up and good luck with Scala! :) If you find the time, take a look at JSweet 2 when it will be out because it will be bringing some new interesting features. This issue will be left open for further resolution.

gramacyan commented 7 years ago

Posting here to keep track of this issue. Bellow an example of what the OP and me are having problems with:

public void handle(Object... objects) {}
public void handle(String str) {}

Transpiles to:

Clazz.prototype.handle$java_lang_Object_A = function () {
  var objects = [];
  for (var _i = 0; _i < arguments.length; _i++) {
    objects[_i] = arguments[_i];
  }
};

Clazz.prototype.handle = function () {
  var objects = [];
  for (var _i = 0; _i < arguments.length; _i++) {
    objects[_i] = arguments[_i];
  }
  if (((objects != null && objects instanceof Array && (objects.length == 0 || objects[0] == null || (objects[0] != null))) || objects === null)) {
    return this.handle$java_lang_Object_A(objects);
  }
  else if (((typeof objects === 'string') || objects === null)) {
    return this.handle$java_lang_String(objects);
  }
  else
    throw new Error('invalid overload');
};

Clazz.prototype.handle$java_lang_String = function (str) {
};
renaudpawlak commented 7 years ago

That's a real issue but I'll have to push it away on v2.1.