rules_closure icon indicating copy to clipboard operation
rules_closure copied to clipboard
verified publisher icon bazelbuild

JsChecker may not honor function level suppressions

Open hochhaus opened this issue 9 years ago • 6 comments

When updating to library/compiler version v20160517 JsChecker reports the following warnings:

INFO: From Checking 812 JS files in //closure/library:library:
external/closure_library/closure/goog/datasource/expr.js:276: WARNING lintChecks JSC_MISSING_REQUIRE_CALL_WARNING - missing require: 'goog.ds.DataManager'
                                  goog.ds.DataManager.getInstance();
                                  ^

external/closure_library/closure/goog/graphics/vmlelement.js:116: WARNING lintChecks JSC_MISSING_REQUIRE_CALL_WARNING - missing require: 'goog.graphics.VmlGraphics'
      goog.graphics.VmlGraphics.toSizeCoord(height);
      ^

external/closure_library/closure/goog/graphics/vmlelement.js:276: WARNING lintChecks JSC_MISSING_REQUIRE_CALL_WARNING - missing require: 'goog.graphics.VmlGraphics'
  style.height = goog.graphics.VmlGraphics.toSizePx(height);
                 ^

external/closure_library/closure/goog/graphics/vmlelement.js:421: WARNING lintChecks JSC_MISSING_REQUIRE_CALL_WARNING - missing require: 'goog.graphics.VmlGraphics'
  style.height = goog.graphics.VmlGraphics.toPosPx(height);
                 ^

external/closure_library/closure/goog/net/browsertestchannel.js:376: WARNING lintChecks JSC_MISSING_REQUIRE_CALL_WARNING - missing require: 'goog.net.BrowserChannel'
      goog.net.BrowserChannel.createChannelRequest(this, this.channelDebug_);
      ^

external/closure_library/closure/goog/net/channelrequest.js:1129: WARNING lintChecks JSC_MISSING_REQUIRE_CALL_WARNING - missing require: 'goog.net.BrowserChannel'
  this.watchDogTimerId_ = goog.net.BrowserChannel.setTimeout(
                          ^

0 error(s), 6 warning(s)

However, each of these functions already contains a function level suppression:

closure/goog/datasource/expr.js closure/goog/graphics/vmlelement.js closure/goog/net/browsertestchannel.js closure/goog/net/channelrequest.js

More research should be done to understand what is going on here. It could be possible that JSChecker is not honoring suppressions on function bodies.

hochhaus avatar May 25 '16 05:05 hochhaus

I did not explore this in depth but after a quick glance at the code I think this might be caused because MISSING_REQUIRE_CALL_WARNING is part of the lintChecks diagnostic group. However, MISSING_REQUIRE_WARNING is part of the missingRequire diagnostic group (which is the group being suppressed in closure-library).

Note: MISSING_REQUIRE_CALL_WARNING was added in https://github.com/google/closure-compiler/commit/7ef0f0d1. I was not familiar with the difference between the MISSING_REQUIRE_{,CALL_}WARNING groups so I found this comment helpful:

 +  // Essentially the same as MISSING_REQUIRE_WARNING except that if the user calls foo.bar.baz()
 +  // then we don't know whether they should require it as goog.require('foo.bar.baz') or as
 +  // goog.require('foo.bar'). So, warn but don't provide a suggested fix.
 +  static final DiagnosticType MISSING_REQUIRE_CALL_WARNING =
 +      DiagnosticType.disabled(
 +          "JSC_MISSING_REQUIRE_CALL_WARNING", "No matching require found for ''{0}''");

My guess is that fixing this TODO will resolve the issue.

  // TODO(tbreisacher): Move MISSING_REQUIRE_CALL_WARNING to missingRequire group
  // as soon as projects that enable that group are fixed.
  CheckRequiresForConstructors.MISSING_REQUIRE_CALL_WARNING,

If projects enabling the group are not fixed, then I think explicitly listing JSC_MISSING_REQUIRE_CALL_WARNING might be our best work around.

hochhaus avatar May 25 '16 21:05 hochhaus

The issue might have something to do with the way compiler passes are configured in the JSchecker source code. Find the Java file that configures it. Then reconcile that with the latest version of the almost identical Lint*.java file in the closure compiler codebase. That might fix things. On May 25, 2016 5:44 PM, "hochhaus" [email protected] wrote:

I did not explore this in depth but after a quick glance at the code I think this might be caused because MISSING_REQUIRE_CALL_WARNING is part of the lintChecks diagnostic group https://github.com/google/closure-compiler/blob/master/src/com/google/javascript/jscomp/DiagnosticGroups.java#L505. However, MISSING_REQUIRE_WARNING is part of the missingRequire diagnostic group https://github.com/google/closure-compiler/blob/master/src/com/google/javascript/jscomp/DiagnosticGroups.java#L408 (which is the group being suppressed in closure-library).

Note: MISSING_REQUIRE_CALL_WARNING was added in google/closure-compiler@ 7ef0f0d https://github.com/google/closure-compiler/commit/7ef0f0d1. I was not familiar with the difference between the MISSING_REQUIRE_*WARNING groups so I found this comment helpful:

  • // Essentially the same as MISSING_REQUIRE_WARNING except that if the user calls foo.bar.baz()
  • // then we don't know whether they should require it as goog.require('foo.bar.baz') or as
  • // goog.require('foo.bar'). So, warn but don't provide a suggested fix.
  • static final DiagnosticType MISSING_REQUIRE_CALL_WARNING =
  •  DiagnosticType.disabled(

  •      "JSC_MISSING_REQUIRE_CALL_WARNING", "No matching require found for ''{0}''");

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub https://github.com/bazelbuild/rules_closure/issues/73#issuecomment-221717411

jart avatar May 25 '16 23:05 jart

Good point, I clearly should have updated JsCheckerPassConfig.java as well. However doing so does not resolve the MISSING_REQUIRE_CALL_WARNING issue. It also catches a few new sets of warnings that need suppression (but I think this is expected).

hochhaus avatar May 26 '16 00:05 hochhaus

Does the MISSING_REQUIRE_CALL_WARNING call happen when you run linter.jar on the Closure Library sauces directly? If not, I'll offer to fetch your branch and take a look myself.

jart avatar May 26 '16 12:05 jart

Great question. Yes, it also occurs with linter.jar. For example:

ahochhaus@ahochhaus-pc:~/rules_closure$ java -jar ~/closure-compiler-20160517/build/linter.jar bazel-rules_closure/external/closure_library/closure/goog/datasource/expr.js
bazel-rules_closure/external/closure_library/closure/goog/datasource/expr.js:276: WARNING - missing require: 'goog.ds.DataManager'
                                  goog.ds.DataManager.getInstance();
                                  ^

bazel-rules_closure/external/closure_library/closure/goog/datasource/expr.js:321: WARNING - Prototype property parts_ should be a primitive, not an Array or Object.
goog.ds.Expr.prototype.parts_ = [];
^

0 error(s), 2 warning(s)

hochhaus avatar May 26 '16 15:05 hochhaus

I also confirmed that moving MISSING_REQUIRE_CALL_WARNING from lintChecks to the missingRequire diagnostic group makes the warning go away (since it will be correctly suppressed by the function level suppress).

410c410,411
<           CheckRequiresForConstructors.MISSING_REQUIRE_WARNING);
---
>           CheckRequiresForConstructors.MISSING_REQUIRE_WARNING,
>           CheckRequiresForConstructors.MISSING_REQUIRE_CALL_WARNING);
505,507d505
<               // TODO(tbreisacher): Move MISSING_REQUIRE_CALL_WARNING to missingRequire group
<               // as soon as projects that enable that group are fixed.
<               CheckRequiresForConstructors.MISSING_REQUIRE_CALL_WARNING,

I'm happy to send a PR for this, but per the TODO legacy users are potentially preventing this from happening.

hochhaus avatar May 26 '16 15:05 hochhaus