google / closure-compiler

A JavaScript checker and optimizer.
https://developers.google.com/closure/compiler/
Apache License 2.0
7.31k stars 1.15k forks source link

Add '--fail_on_warnings', issue #3451 #4124

Closed doublep closed 9 months ago

doublep commented 9 months ago

This PR adds option --fail_on_warnings as requested in #3451 (and that I missed myself too). It is obviously not finished, but before adding documentations, tests, etc. I'd like to see some feedback if this option is fine in general.

Instead of meddling with existing warning guards as suggested by brad4d in the issue, I decided to keep a simple boolean flag and at the last possible point check it: if we still have a warning level by then, simply turn it into an error. I feel it is cleaner this way (and also easier, yeah), because configuration of warning guards (which is quite extensive, with all the different named categories) is completely independ from "fail on warnings" setting and you don't need to coordinate changes in them anywhere. In effect, the "fail on warnings" setting is simple "whatever I want as warnings usually, should become an error in this context" and is not dependent on how you set that "whatever" — be that the defaults or extensive custom configuration.

A possible improvement would be to pass to error reporter a boolean "error elevated from warning" so that it could report that if it so chooses.

I verified that it does produce the desired change:

$ java -jar bazel-bin/compiler_uberjar_deploy.jar ~/test/test.js --checks_only --warning_level verbose || echo DOES NOT COMPILE
/home/paul/test/test.js:3:0: WARNING - [JSC_TYPE_MISMATCH] assignment
found   : string
required: number
  3| a = 'foo'
     ^^^^^^^^^

0 error(s), 1 warning(s), 100.0% typed
$ java -jar bazel-bin/compiler_uberjar_deploy.jar ~/test/test.js --checks_only --warning_level verbose --fail_on_warnings || echo DOES NOT COMPILE
/home/paul/test/test.js:3:0: ERROR - [JSC_TYPE_MISMATCH] assignment
found   : string
required: number
  3| a = 'foo'
     ^^^^^^^^^

1 error(s), 0 warning(s), 100.0% typed
DOES NOT COMPILE

Where ~/test/test.js is:

/** @type {number} */
let a = 10;
a = 'foo'
google-cla[bot] commented 9 months ago

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

brad4d commented 9 months ago

I've imported this change and have looked at it. I see the reason for not doing what I suggested on the related issue. Maybe this is OK, but I feel like this change should probably be in the error manager implementation code.

I'm going to ask @concavelenz to take a look also.

brad4d commented 9 months ago

I'm afraid that this approach isn't going to fly. It is simpler, true, but that's because it's trying to completely ignore the established mechanisms for controlling the reporting of errors and warnings.

Doing that seems likely to have unforeseen consequences.

I quite agree that the current mechanism is complicated, but it was developed to meet particular needs and shouldn't just be worked around.

I honestly do not have the time and energy to put into figuring out the details necessary to make the option desired here work. What I can do is tell you that the inside-google-only version of the compiler does have such an option and it accomplishes it by using StrictWarningsGuard as I suggested on #3451