rules_closure icon indicating copy to clipboard operation
rules_closure copied to clipboard

Each library should perform full checks

Open pcj opened this issue 7 years ago • 5 comments

Similarly, how to suppress errors like these?

external/closure_library/closure/goog/ui/keyboardshortcuthandler.js:912: ERROR - goog.object.isEmpty expects an object, not an array. Did you mean to use goog.array?
    if (goog.object.isEmpty(node.next)) {
        ^
  ProTip: "JSC_ARRAY_PASSED_TO_GOOG_OBJECT" or "analyzerChecks" or "analyzerChecksInternal" can be added to the `suppress` attribute of:
  @io_bazel_rules_closure//closure/library:library

external/soy_jssrc/soyutils_usegoog.js:802: ERROR - The return type of the function "soy.$$getDelegateFn" is nullable, but it always returns a non-null value. Consider making the return type non-nullable.
soy.$$getDelegateFn = function(
                      ^
  ProTip: "JSC_NULLABLE_RETURN_WITH_NAME" or "analyzerChecks" or "analyzerChecksInternal" can be added to the `suppress` attribute of:
  @io_bazel_rules_closure//closure/templates:soy_jssrc

2 error(s), 0 warning(s)

pcj avatar Nov 12 '16 12:11 pcj

I feared this would have happened.

The short answer is to use the escape hatch and add suppress_on_all_sources_in_transitive_closure = ["analyzerChecks"] to your closure_js_binary rule.

The long answer is we probably need to modify closure_js_library to compile the entire transitive closure without dead code elimination so we can guarantee that the suppress attribute is complete. One of the tradeoffs made by the compiler is that dead code elimination happens before compiler checks are performed. So there's still a lot more suppress codes that we probably need to add to @io_bazel_rules_closure//closure/library that we haven't discovered yet.

jart avatar Nov 12 '16 21:11 jart

I'm not following regarding the influence of dead-code elimination (but I'm glad you have a better clue than me ):

It's well-known that that closure library generates thousands of errors/warnings when applied to the strictest set of checks. Given that the closure-compiler always does global analysis with the complete set of inputs (transitive closure of closure-library + workspace js file input), I don't understand how it could selectively apply a different set of checks to sub-partitions of these input files, without modification of the compiler itself.

Said another way, without the ability for the compiler to generate some sort of intermediate object file representation that can be re-used as an (incremental) input to a subsequent compilation (loosely similar to a jar file), I don't see how it's possible to correctly implement this (same reason compilation times will always be relatively long).

Hopefully my reasoning is wrong based on incomplete understanding of how the compiler works.

pcj avatar Nov 12 '16 21:11 pcj

That being said, it was a bit of shocker of how many more errors were present in my codebase that I did not see before! Even if one has to enable this feature only once-in-a-while, it's still incredibly useful.

pcj avatar Nov 12 '16 21:11 pcj

I don't understand how it could selectively apply a different set of checks to sub-partitions of these input files, without modification of the compiler itself.

That's exactly what Closure Rules does! see JsCompiler, JsCompilerWarnings.

the ability for the compiler to generate some sort of intermediate object file representation that can be re-used as an (incremental) input to a subsequent compilation

Got you covered. See the ClosureJsLibrary protobuf. Great minds think alike.

I'm hoping it should be relatively trivial to adapt JsChecker to perform all checks. Compilation won't be incremental. But compile safety would be.

jart avatar Nov 12 '16 22:11 jart

I made some progress on fully compiling the libraries to find all suppress types in https://github.com/bazelbuild/rules_closure/pull/174. Also I'm looking for feedback on https://github.com/bazelbuild/rules_closure/pull/175.

hochhaus avatar Dec 02 '16 21:12 hochhaus