[RFC] Add support for controlling diagnostics severities at file-level granularity through command line (original) (raw)

Thanks a lot for the great interest and feedback, sorry for taking so long to get back to this.

In the light of these comments I’d like to revise the proposal to look like:

• Default warning severities are controlled by compile flags, mappings file can only be used to suppress warnings.
• There’s a new command line option --warning-suppression-mappings=mappings.txt rather than overloading current -Wfoo flags.
• We use a single mappings file, based on sanitizer special case lists, sections describing diagnostics groups and categories for including/excluding the globs.
• We load and diagnose discrepancies in mappings file at startup, when building diagnostic options. glob-list generation is still delayed.

e.g:

clang -Wfoo --warning-suppression-mappings=mappings.txt foo.cc

[foo]
src:*/third_party/*=suppress
src:*/third_party/absl/*=emit
src:*/third_party/absl/bad/dir/*=suppress

To go over some of the big topics that were discussed on the thread so far;

1. Reproducibility in preprocessed outputs && Presumed locations

Thanks this was a part of the design I didn’t explicitly spell out.

Existing functionality already accounts for presumed locations, e.g. checking system-header/macro state before emitting diagnostics, or checking whether a source location belongs to a main file. Basically, ~all the code paths that needs to deal with a source file, rather than a source location, accounts for presence of line directives.

Hence I was just assuming this feature wouldn’t be any different, e.g. if a certain section of a file is pretending to be some other file, we should perform analysis under that file’s name.

Looking at your examples, and a bunch more from other open source code bases, I think it’s a rarely used feature. Also feels like people are using it mostly for similar reasons to @jyknight mentioned (i.e. have PP’d version of a source file).

To sum up, I am inclined towards respecting #line directives in this new mechanism:

• This is mostly used for stitching sources into a single file, and existing diagnostic filtering mechanisms like system-header-prefixes already work with it.
• A codebase needs to opt-in to this new mechanism, so despite spelling them explicitly being tedious, i think it’s rather an easy workaround.

2. Overloading -Wfoo=mappings.txt vs new flag --warning-mapping=mappings.txt

This is definitely a decision I struggled with as well, and I am not feeling strongly about one way or the other. I believe there are enough valid concerns raised around overloading the -Wfoo=… syntax. So I am happy to change this to be single file and new flag instead.

Possible downsides of -warning-mappings-file=mappings.txt approach that made me shy away previously:

1. Figuring out which diagnostics are to be expected on a TU has one more layer now. Confusing/surprising if a warning is mentioned both in the command line and mapping file.
2. We need to perform IO much earlier in the process, since we need to diagnose misspelled warning groups.
3. Having possibly a really big file, if there are many warnings that’re partially enabled.

I believe we can address 1) by combining regular command line flags and treating the mapping file as a suppression file, i.e. clang -Wfoo -warning-supression-file=disablement_mappings.txt foo.cc, now the warning foo is emitted for all sources in TU, unless they’re matched by disablement_mappings.txt. Hence the file itself can’t turn on any new warnings.

This ensures warning groups and their severities to be expected are still obvious by looking at the command line flags. it needs further investigation to see if it’s suppressed, but that isn’t new it’s already required due to system-header-prefixes, line directives, diagnostics pragmas.

As for 2), I think it’s still better to diagnose the file once. The cost of doing this is likely nothing compared to parsing C++ , but this alternative also opens up the possibility for making 2) less severe, as we’re only using file to suppress warnings, if there’s a typo/error in the file, compilation will fail immediately.

For 3) we can extend the file format to have some basic #include support in the future.

3. Mappings file format

As mentioned above, and considering rest of the discussion in the thread, I believe having a format like:

# Suppression specs for -Wfoo
# It requires -Wfoo to be part of the compile flags, so by default it's enabled
# everywhere, and suppressed only for files that match a glob in `suppress` 
# category.
[foo]
src:*/third_party/*=suppress
src:*/third_party/absl/*=emit
src:*/third_party/absl/bad/dir/*=suppress

Using json vs special case list format

I don’t envision this mapping file growing more features, especially in a way that can’t be supported by special case lists, and I think such a text file is more readible for a human, compared to a json/yaml version like:

[ 
 {
   "diag": "foo",
   "suppress": ["*/third_party/*"],
   "emit": ["..."],
 }
]

So, I’d prefer to stick to a basic text format here.

4. Precedence over clang-diagnostics pragmas

I also feel like it’s better to give pragmas higher precedence. It both gives the developer a way to enable certain analysis for some parts of their code (e.g. constains some critical logic) but also I don’t see a reason to suppress a diagnostics, if the author of the code wanted to explicitly enable it.

Other topics;

I think this isn’t needed in the new iteration, as we don’t provide multiple files anymore.

Totally agreed, as also mentioned briefly above, I think this is a very big point for ease-of-use.

That’s a totally valid concern, but I think the current proposal won’t make it inherently harder. Any workflow that handles (no-)system-header-prefix should be able to handle this new flow, modulo the need for extra IO, but again any system that can deals with line directives or clang-diagnostics pragmas need that capability.

Yes, basically delay work until someone is asking for severity of a specific diagnostics.

The logic will likely be available only for diagnostics-engine internals initially, but if use cases arise I’d be happy to make the logic more publicly available.

Severity was always meant to be controlled by regular command line flag machinery, rather than the mappings file. Otherwise I agree that it gets both confusing and complicated really quickly.

SGTM!