Allow to configure flatc diagnostics (was: disable lowercase snakecase warning for fields) - #9244
Conversation
This patch allows to disable the lowercase snake_case warning via the global `--no-warnings` command line option. Simple, but maybe too broad. The patch also adds tests to test if `--no-warnings` successfully disables the snake_case warning. This partly fixes google#9243, but not in a nice way.
`--no-warnings` currently is all or nothing: either every diagnostic the parser emits is shown, or none of them are. Projects that cannot follow one particular recommendation therefore have to choose between build logs spammed by a warning they have consciously decided to accept, and losing every other warning along with it. Neither is acceptable when a real problem must not drown in the noise. Give `--no-warnings` an optional, comma separated list of keys naming the warnings to inhibit. Without an argument it keeps inhibiting all of them, so existing command lines are unaffected. Only the `=` form takes an argument, because `flatc --no-warnings schema.fbs` is valid usage and must keep treating `schema.fbs` as an input file. An inhibited warning is not reported and does not set `has_warning_`, which preserves the established interaction with `--warnings-as-errors`: what is not warned about cannot fail the build. Unknown keys are rejected rather than ignored. A silently accepted typo would leave the warning enabled and make the option useless in CI. `IDLOptions::no_warnings` becomes `IDLOptions::disabled_warnings`, a bit set of the new `IDLOptions::Warning` values. The member is renamed rather than retyped in place so that code still assigning a `bool` to it fails to compile instead of silently inhibiting a single warning. This fixes issue google#9243.
`--warnings-as-errors` is all or nothing, which makes it unusable next to `--no-warnings=<keys>`: a project that has consciously accepted one recommendation still wants the remaining warnings to fail its builds, and today it can only choose between failing on all of them or on none. Give `--warnings-as-errors` the same optional, comma separated list of keys that `--no-warnings` accepts. Without an argument it keeps promoting every warning, so existing command lines are unaffected, and as with `--no-warnings` only the `=` form takes an argument. `Parser` now records which warnings it has reported rather than just whether it reported any, so that a warning can be promoted individually. A warning inhibited by `--no-warnings` is never reported and therefore never becomes an error, which keeps the established interaction between the two options intact. Also document `--warnings-as-errors`, which was missing from flatc.md entirely. This is a logical addition to the fix for issue google#9243.
|
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. |
CLA should be signed. |
Problem
flatchas two knobs for diagnostics, and they are binary.--no-warningssilences all of them,--warnings-as-errorspromotes allof them. There is nothing in between.
That leaves any project that has consciously decided not to follow one
particular recommendation with two bad options. The recurring case is the
field naming warning from #6005: schemas that mirror or are generated from
an existing code base cannot rename their fields without breaking the very
thing they exist to describe. Such a project can either
until real diagnostics are invisible in the noise, or
--no-warnings, and give up every other diagnostic flatc offers.The second is especially painful in CI, where
--warnings-as-errorsisexactly what you want for the warnings you have not opted out of — and
it is unreachable today, because opting out of one warning means opting
out of all of them.
This has now come up in #6032, #7111, #8612 and #9243.
What this changes
Both options take an optional, comma separated list of warning keys:
strict-field-namesimplied-attributerepeated-attributeunsigned-bit-flagsbit_flagsenum has a signed underlying typeallA warning inhibited by
--no-warningsis never reported and thereforenever becomes an error, which is the behaviour
--no-warningsalready hastoday with respect to
--warnings-as-errors.What this does not change
options behave exactly as before. Only the
=form takes a list, soflatc --no-warnings schema.fbsstill treatsschema.fbsas input.wording, same locations.
Notes for review
accepted typo would leave the warning enabled and make the option
useless in CI, which is the main place it is wanted.
IDLOptions::no_warningsbecomesdisabled_warningsandwarnings_as_errorsbecomes aWarningFlagsbit set. Both members arerenamed rather than retyped in place, so downstream code assigning a
boolfails to compile instead of silently meaning "one warning".IDLOptions::Warningfollows theLanguage/lang_to_generatepatternalready in
IDLOptions: an unscopedk-prefixed enum with a fixedunderlying type, stored in an integer. No casts, no operator overloads.
warning_options; the parser and the--helptext share that table.--warnings-as-errorswas not documented inflatc.mdat all. It isnow.
Tested with
FLATBUFFERS_STRICT_MODE=ON;flattestspasses at eachcommit, and the new cases cover per-key inhibition, per-key promotion, key
combination, and the interaction between the two options.
Closes #9243.