Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

doublep
Copy link

@doublep doublep commented Sep 14, 2023

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
Copy link

google-cla bot commented Sep 14, 2023

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
Copy link
Contributor

brad4d commented Sep 25, 2023

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
Copy link
Contributor

brad4d commented Oct 4, 2023

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

@brad4d brad4d closed this Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants