bazelbuild / rules_closure

Closure rules for Bazel
https://developers.google.com/closure
Apache License 2.0
152 stars 114 forks source link

Each library should perform full checks #169

Open pcj opened 7 years ago

pcj commented 7 years ago

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)
jart commented 7 years ago

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.

pcj commented 7 years ago

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 commented 7 years ago

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.

jart commented 7 years ago

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.

hochhaus commented 7 years ago

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.