Generate closure @suppress jsdoc tag on method when @SuppressWarning present in java

[realityforge]

I would like J2CL to generate @suppress jsdoc tags when a specially formatted @SuppressWarning() is present in java code. To start with the simplest option would be to detect the @SuppressWarning() on the java method and then translate to a @suppress on the corresponding method implementation - while detecting appropriate @SuppressWarning() in other contexts may be possible, it would also be more complex.

For now let us assume that any SuppressWarnings key that had a prefix of CL: should be translated into a closure suppression.

So we could easily imagine code such as:

  @SuppressWarnings( "CL:checkDebuggerStatement" )
  public static void pauseIfInDebugger() {
    Js.debugger();
  }

Generating the following javascript code:

  /**
   * @return {void}
   * @public
   * @suppress {checkDebuggerStatement}
   */
  static void pauseIfInDebugger() {
    debugger;
  }

A few questions:

• Would a PR containing this be accepted?
• Should I wait until JDT is eliminated or is it acceptable to start with what is present now?
• Do you expect it to be possible for me to write tests in the OSS variant of the library or should I look at that first?

[gkdn]

Given that we have survived for 3+ years with this, I'm little bit skeptical on the value. The particular example doesn't help since it doesn't seem like something you want to suppress and there is already suppressing available at library build level.

[realityforge]

I guess that may be true in the google context where everything is built from source and the project that defines the artifact is also responsible for defining the blaze/bazel library.

However if you are in another context and pulling down the artifact as an external dependency (i.e. from a maven repository) or are using a different build toolchain then I believe that this capability would make things a bit easier. Right now if you are using an external library, the consumer needs to know about the suppressions that need to be applied to the library rather than the library suppressing it locally.

For example, in current Bazel I end up defining

http_file(
    name = "org_realityforge_braincheck__braincheck__1_16_0",
    downloaded_file_path = "org/realityforge/braincheck/braincheck/1.16.0/braincheck-1.16.0.jar",
    sha256 = "9370edd88e26d5442ca2a083c77fd71e6ea212bb1848c2a56ad1d921cae61dfc",
    urls = ["https://repo.maven.apache.org/maven2/org/realityforge/braincheck/braincheck/1.16.0/braincheck-1.16.0.jar"],
        )

And then

j2cl_library(
    name = "braincheck-j2cl",
    srcs = ["//thirdparty:braincheck"],
    deps = [
        ":javax_annotation-j2cl",
        ":gwt-javaemul-internal-annotations-j2cl",
        ":jsinterop_annotations-j2cl",
        ":jsinterop_base-j2cl",
    ],
    js_suppress = ["checkDebuggerStatement"],
)

I could imagine that this capability would also be useful if you are using maven as the build tool.

[gkdn]

I think I understand the use case.
Though, I may still disagree with the idea of providing a suppressed debugger statement within a library :)

@rluble any opinions on this feature request?

[rluble]

I think in this case, it is better that the library user decide that a debugger statement is ok in their application. But I also understand that there might be use cases where we would want to pass-through suppressions.

In general I don't particularly like passing through verbatim text to the output.

On the technical side this would not be difficult to implement but I would like to have a more compelling case.

[realityforge]

In this particular use case the braincheck library is an invariant checking library that is completely optimized away in production builds. In GWT terms, the debugger statement is only really present when running in SuperDev mode.

The real code looks something like:

  private static void doFail( @Nonnull final Supplier<String> message )
  {
    /*
     * This flag will only be present and set when GWT is compiling the source code and the relevant
     * compile time property is defined. Thus this will be false in normal jre runtime environment.
     */
    if ( "ENABLED".equals( System.getProperty( "jre.debugMode" ) ) )
    {
      Js.debugger();
    }
    if ( BrainCheckConfig.verboseErrorMessages() )
    {
      throw new IllegalStateException( BrainCheckUtil.safeGetString( message ) );
    }
    else
    {
      throw new IllegalStateException();
    }
  }

If invariant checking is disabled at compile time, then this code is completely stripped. If the user is running in devmode then I would like to pause on invariant failures. If running with invariants enabled but not in devmode it just throws an error. It would be really nice to stop users of this library having to explicitly sprinkle the suppress statement throughout their applications build files and allow the library author to indicate that this is expected.

As for passing verbatim text through, another option would be to use the existing DiagnosticGroups and DiagnosticType registry in closure as a whitelist that would make this slightly safer and poking through the source it seems it jscomp is already a dependency of the transpiler.

I have hacked together an implementation and it does seem easy to implement but I have had no luck in getting test infrastructure working 🤷‍♂

[realityforge]

@rluble I am guessing that the above use case did not make the grade? :)

Another scenario where I want to use it is to make it easier to share code between J2CL/GWT2.x. In this scenario, I have a java class that extends a native js class. i.e.

@JsType( isNative = true, namespace = JsPackage.GLOBAL, name = "React.Component" )
public abstract class NativeComponent
{
  ...
}

private static final class MyNativeReactComponent extends NativeComponent
{
  ...
}

To expose lifecycle methods to the react runtime you need to name the method with a specific name. The easiest way to do this is to define an interface ala:

@JsType( isNative = true, namespace = JsPackage.GLOBAL, name = "?" )
public interface OnComponentDidUpdate
{
  void componentDidUpdate( @Nonnull JsPropertyMap<Object> prevProps );
}

And have the java implementation implement it such as

private static final class MyNativeReactComponent
  extends NativeComponent
  implements OnComponentDidUpdate
{
  ...
}

In GWT2.x the alternative is to use @JsMethod and selectively export symbols but this somewhat bloats the code compared with the interface and requires a significant amount of bookkeeping. However, when compiling the same code in j2cl the componentDidUpdate(...) method is forced to be public which conflicts with the externs which require it to be protected. I can fix this by changing the externs although my preference would be to just suppress this warning on the MyNativeReactComponent class. (The MyNativeReactComponent class is actually a class generated by an annotation processor so it would be trivial to generate it with an appropriate suppress).

While you may think that supporting both GWT2.X as well as J2CL is not useful in your context it would sure help us at least during the transition.

Thoughts?

[rluble]

I think the suppression related to public/protected should be included in the suppressions that we do for J2CL since J2CL never emits @Protected if I remember correctly. Could you post a small repro that has the warning?

[gkdn]

Side note: avoid ? whenever possible. public interface OnComponentDidUpdate should point to some extern.