google / jsinterop-generator

Generates Java annotated with JsInterop from JavaScript extern sources
Apache License 2.0
78 stars 24 forks source link

Should "@return {void}" be the default. #39

Closed realityforge closed 5 years ago

realityforge commented 5 years ago

In commit https://github.com/google/j2cl/commit/2169efb26472cbea636d3859a772ad27aed3a6bf the @return {void} closure jsdoc has been removed from the generated output with reasoning Do not emit "@return {void}" since this is the default.

Should this tool follow this assumption as currently if a function is not annotated with a return of void or undefined, this tool will default to generating an Object return value.

gkdn commented 5 years ago

@rluble When I filed the feature request for that commit, I didn't double check if that's the default behavior or not. Did you check if calling a method with declared 'void' return and assigning it to something behaves any different than not declaring at all?

rluble commented 5 years ago

@return {void} is the default only when there are no returns with values in the body. When @return is omitted, IIRC, JsCompiler infers the return type. In J2CL because these are Java methods that are declared returning void we can safely make the assumption but it is not true for the general case.

I will double check.

rluble commented 5 years ago

In any case for the generator is @return is not specified I think it should be Object.

rluble commented 5 years ago

I checked. If all checks are enabled it only allows to omit @return if there are not returns with values ([JSC_MISSING_RETURN_JSDOC] Function with non-trivial return must have JSDoc indicating the return type.)

If the check is not enabled it infers void in that case and ? in all other cases.

I also checked explicitly on externs and in that case omitting @return results in the return being unknown ?.

gkdn commented 5 years ago

The problem in the generator case is; many times people simply forget to add the @return which causes us to generate bad API.

@jDramaix can verify but IIRC there was no way to separate missing return from declared return with undefined. In that case I think we have 2 options:

  1. We can assume return undefined is void and see which APIs negatively effected and make a decision from there.
  2. Do a feature request to JsCompiler to enforce @return on externs on per-library type checking (preferred).
jDramaix commented 5 years ago

So just to clarify the problem. The jscompiler behavior for evaluating the default return type is different if it's a closure library or an extern file. If you don't specify @return in a closure code, the compiler assumes the function return undefined. In an extern file, the compiler assumes the function retuns unknown type (?). The jsinterop generator does not read the jsdoc and rely on the type information provided by the JsCompiler. If the function return type is undefined, we convert that to void. If it's unknown type, we convert that to Object.

Most of closure developers do not know this difference for the default return type and as Goktug mentioned they forgot to specify @return {undefined} in extern files which generate bad API both in JS than in Java. The option 2 is for me the best option.