Clarify expected behavior of `--W*=diagnostic` option when `diagnostic` can be an error

[kfcripps]

Some warnings and errors have the same name, for example ERR_INVALID and WARN_INVALID:

// map from errorCode to ErrorSig
std::map<int, cstring> ErrorCatalog::errorCatalog = {
    // Errors
    {ErrorType::LEGACY_ERROR, "legacy"},
    {ErrorType::ERR_UNKNOWN, "unknown"},
    {ErrorType::ERR_UNSUPPORTED, "unsupported"},
    {ErrorType::ERR_UNEXPECTED, "unexpected"},
    {ErrorType::ERR_EXPECTED, "expected"},
    {ErrorType::ERR_NOT_FOUND, "not-found"},
    {ErrorType::ERR_INVALID, "invalid"},
    {ErrorType::ERR_EXPRESSION, "expr"},
    {ErrorType::ERR_OVERLIMIT, "overlimit"},
    {ErrorType::ERR_INSUFFICIENT, "insufficient"},
    {ErrorType::ERR_UNINITIALIZED, "uninitialized"},
    {ErrorType::ERR_TYPE_ERROR, "type-error"},
    {ErrorType::ERR_UNSUPPORTED_ON_TARGET, "target-error"},
    {ErrorType::ERR_DUPLICATE, "duplicate"},
    {ErrorType::ERR_IO, "I/O error"},
    {ErrorType::ERR_MODEL, "Target model error"},
    {ErrorType::ERR_RESERVED, "reserved"},

    // Warnings
    {ErrorType::LEGACY_WARNING, "legacy"},
    {ErrorType::WARN_FAILED, "failed"},
    {ErrorType::WARN_UNKNOWN, "unknown"},
    {ErrorType::WARN_INVALID, "invalid"},
    {ErrorType::WARN_UNSUPPORTED, "unsupported"},
    {ErrorType::WARN_DEPRECATED, "deprecated"},
    {ErrorType::WARN_UNINITIALIZED, "uninitialized"},
    {ErrorType::WARN_UNUSED, "unused"},
    {ErrorType::WARN_MISSING, "missing"},
    {ErrorType::WARN_ORDERING, "ordering"},
    {ErrorType::WARN_MISMATCH, "mismatch"},
    {ErrorType::WARN_OVERFLOW, "overflow"},
    {ErrorType::WARN_IGNORE_PROPERTY, "ignore-prop"},
    {ErrorType::WARN_TYPE_INFERENCE, "type-inference"},
    {ErrorType::WARN_PARSER_TRANSITION, "parser-transition"},
    {ErrorType::WARN_UNREACHABLE, "parser-transition"},
    {ErrorType::WARN_SHADOWING, "shadow"},
    {ErrorType::WARN_UNINITIALIZED_USE, "uninitialized_use"},
    {ErrorType::WARN_UNINITIALIZED_OUT_PARAM, "uninitialized_out_param"},
    {ErrorType::WARN_IGNORE, "ignore"},
    {ErrorType::WARN_INVALID_HEADER, "invalid_header"},
    {ErrorType::WARN_DUPLICATE_PRIORITIES, "duplicate_priorities"},
    {ErrorType::WARN_ENTRIES_OUT_OF_ORDER, "entries_out_of_priority_order"},

As a result, for example, --Wdisable=invalid will disable errors of type ERR_INVALID. Is this expected?

I have the same question about the --Wwarn option.

Also, the --Winfo option seems to explicitly disallow diagnostics that are both errors and warnings, which is inconsistent with how --Wwarn and --Wdisable treats such diagnostics.

My opinion is that allowing errors to be disabled is a bad idea, as disabling some error could result in unexpected behavior, but maybe there was a reason to allow this...

A few examples:

p4test testdata/p4_16_errors/accept_e.p4:

testdata/p4_16_errors/accept_e.p4(18): [--Werror=invalid] error: Invalid parser state: accept should not be implemented, it is built-in
    state accept { // reserved name
          ^^^^^^
testdata/p4_16_errors/accept_e.p4(16): [--Werror=invalid] error: Parser parser p has no 'start' state
parser p()
       ^

p4test testdata/p4_16_errors/accept_e.p4  --Wwarn=invalid:

testdata/p4_16_errors/accept_e.p4(18): [--Wwarn=invalid] warning: Invalid parser state: accept should not be implemented, it is built-in
    state accept { // reserved name
          ^^^^^^
testdata/p4_16_errors/accept_e.p4(16): [--Wwarn=invalid] warning: Parser parser p has no 'start' state
parser p()
       ^
testdata/p4_16_errors/accept_e.p4(18): [--Werror=duplicate] error: accept: Duplicates declaration accept
    state accept { // reserved name
          ^^^^^^

p4test testdata/p4_16_errors/accept_e.p4  --Wdisable=invalid:

testdata/p4_16_errors/accept_e.p4(18): [--Werror=duplicate] error: accept: Duplicates declaration accept
    state accept { // reserved name
          ^^^^^^

p4test testdata/p4_16_errors/accept_e.p4  --Winfo=invalid:

[--Werror=invalid] error: Error invalid cannot be demoted

[kfcripps]

https://github.com/p4lang/p4c/pull/4366 is a potential fix for this, but I am not confident that it is the "correct" fix.

[vlstill]

Disclaimer: I wasn't there when most of this was designed. This is my opinion.

As a result, for example, --Wdisable=invalid will disable errors of type ERR_INVALID. Is this expected?

That seems quite bad to me. We probably should not have the same string key for both.

Also, the --Winfo option seems to explicitly disallow diagnostics that are both errors and warnings, which is inconsistent with how --Wwarn treats such diagnostics.

This one was intentional when the info diagnostic kind was added recently. I think this was a correct decision.

My opinion is that allowing errors to be disabled is a bad idea, as disabling some error could result in unexpected behavior, but maybe there was a reason to allow this...

I agree. I guess a lot of things will break if an error is ignored. I'm not sure if this was a deliberate decision or if it is just not checked (maybe @mihaibudiu or @ChrisDodd could know).

I would suggest:

• errors cannot be demoted/disable at all
• warnings and infos can be disabled or promoted, but there is probably no reason to demote them (i.e. warning -> info)
  • promoting info -> warning is a bit weird, but probably there is no harm in in
  • warning -> error (and possibly info -> error too) is similar to a commonplace -Werror option and makes sense

I'm not sure what could break if we changed this though.

[ajwalton]

I'm largely in agreement with @vlstill - errors should not be demotable, but info and warning should be promotable.

I think demoting (or suppressing) info's and warnings should be allowed - if you've acknowledged a warning and it's OK in your environment, then removing this to prevent it obscuring other, unknown/unexpected, warnings should be OK. (Other systems/tools permit this.)

As a general point, I believe that we should make more use of the error_type for warnings/info so that individual messages can be suppressed, rather than classes or messages. Again, this is common with messaging utilities.

[kfcripps]

This one was intentional when the info diagnostic kind was added recently. I think this was a correct decision.

As a side-effect, https://github.com/p4lang/p4c/pull/4197 also disallows --Winfo=diagnostic when the diagnostic can also be a warning. For example, --Winfo=invalid could mean that the user wants to demote the WARN_INVALID warnings, but #4197 no longer allows this.

An alternate approach is https://github.com/p4lang/p4c/pull/4366, which allows things such as --Wdisable=invalid, but only disables the diagnostic when it is a warning, and does nothing for the corresponding error of the same type.

If the side-effect I mentioned above was unintentional, then https://github.com/p4lang/p4c/pull/4366 might be a correct fix for --Wdisable and --Wwarn (and can also be applied to --Winfo by removing the error message added in #4197). Otherwise, a correct fix might be to not have any error and warning codes that map to the same name in the error catalog (in other words, --W*=diagnostic will no longer be ambiguous as to whether the intent is to apply the option to an error or a warning).

[kfcripps]

As a side note, the description for --Werror is "Report an error for a compiler diagnostic, or treat all warnings as errors if no diagnostic is specified." Does it make sense for [--Werror=diagnostic] to be printed for errors, or only for warning/info messages? Errors are already errors.. Example:

p4test testdata/p4_16_errors/accept_e.p4:

testdata/p4_16_errors/accept_e.p4(18): [--Werror=invalid] error: Invalid parser state: accept should not be implemented, it is built-in
    state accept { // reserved name
          ^^^^^^
testdata/p4_16_errors/accept_e.p4(16): [--Werror=invalid] error: Parser parser p has no 'start' state
parser p()
       ^

[kfcripps]

As a side-effect, https://github.com/p4lang/p4c/pull/4197 also disallows --Winfo=diagnostic when the diagnostic can also be a warning. For example, --Winfo=invalid could mean that the user wants to demote the WARN_INVALID warnings, but https://github.com/p4lang/p4c/pull/4197 no longer allows this.

An alternate approach is https://github.com/p4lang/p4c/pull/4366, which allows things such as --Wdisable=invalid, but only disables the diagnostic when it is a warning, and does nothing for the corresponding error of the same type.

If the side-effect I mentioned above was unintentional, then https://github.com/p4lang/p4c/pull/4366 might be a correct fix for --Wdisable and --Wwarn (and can also be applied to --Winfo by removing the error message added in https://github.com/p4lang/p4c/pull/4197). Otherwise, a correct fix might be to not have any error and warning codes that map to the same name in the error catalog (in other words, --W*=diagnostic will no longer be ambiguous as to whether the intent is to apply the option to an error or a warning).

@ajwalton @vlstill @fruffy @jafingerhut Let me know which approach you prefer and I will update #4366 accordingly.

[vlstill]

As a side note, the description for --Werror is "Report an error for a compiler diagnostic, or treat all warnings as errors if no diagnostic is specified." Does it make sense for [--Werror=diagnostic] to be printed for errors, or only for warning/info messages? Errors are already errors.. Example:

p4test testdata/p4_16_errors/accept_e.p4:

testdata/p4_16_errors/accept_e.p4(18): [--Werror=invalid] error: Invalid parser state: accept should not be implemented, it is built-in
    state accept { // reserved name
          ^^^^^^
testdata/p4_16_errors/accept_e.p4(16): [--Werror=invalid] error: Parser parser p has no 'start' state
parser p()
       ^

I think the diagnostic kind (e.g. invalid in this case) should be there. For warning/info it is necessary to allow supressions and for errors it can still be useful (if not for anything else then for testing the compiler itself). We could maybe the output a little, or put it at the end of the main message, similarly as GCC does it (I personally find it a bit noisy at the current location).

[vlstill]

This one was intentional when the info diagnostic kind was added recently. I think this was a correct decision.

As a side-effect, #4197 also disallows --Winfo=diagnostic when the diagnostic can also be a warning. For example, --Winfo=invalid could mean that the user wants to demote the WARN_INVALID warnings, but #4197 no longer allows this.

Yes, this can be confusing. At the same time, applying -Wdisable=invalid and then seeing --Werror=invalid messages in the output can be confusing too.

An alternate approach is #4366, which allows things such as --Wdisable=invalid, but only disables the diagnostic when it is a warning, and does nothing for the corresponding error of the same type.

If the side-effect I mentioned above was unintentional, then #4366 might be a correct fix for --Wdisable and --Wwarn (and can also be applied to --Winfo by removing the error message added in #4197). Otherwise, a correct fix might be to not have any error and warning codes that map to the same name in the error catalog (in other words, --W*=diagnostic will no longer be ambiguous as to whether the intent is to apply the option to an error or a warning).

I can imagine doing what you did in #4366 plus adding a warning into a -Winfo= & -Wwarn for the cases when this tries to disable an error (too). Otherwise I'm concerned there would be options that are accepted and silently ignored, e.g. -Winfo=type-error would be completely silently ignored in your current implementation.

But ultimately I think we should try to avoid having the same diagnostic kind in multiple of the error/warning/info categories.

[kfcripps]

Otherwise I'm concerned there would be options that are accepted and silently ignored, e.g. -Winfo=type-error would be completely silently ignored in your current implementation.

You are right. I can add back the #4197 error only for diagnostics that can only be errors.

[fruffy]

4366 is merged, is this issue resolved? Or is there follow-up work planned?

[kfcripps]

Or is there follow-up work planned?

Nope, I have nothing else planned for this.

is this issue resolved?

4366 addressed my primary concern by not allowing errors to be demoted. But feel free to leave this open to address the remaining concern:

But ultimately I think we should try to avoid having the same diagnostic kind in multiple of the error/warning/info categories.

I'm not sure a consensus has been reached here. Depending on what the conclusion is, we may wish to either remove all such overlapping diagnostics, or just leave them as they are.

[kfcripps]

@asl @vlstill @fruffy @ajwalton

I have a new question related to this. Currently, the following options, for example:

--Werror --Wdisable=uninitialized_use --Wdisable=invalid_header

Will promote all messages to errors, and therefore, the requested uninitialized_use and invalid_header message types will not be disabled because errors cannot be demoted/disabled as of #4366.

Is this the desired behavior? Or would it make more sense if --Werror only applies to message types that aren't also disabled?

[asl]

IMO, explicit "disable" should always be honored. I guess for warnings we need to have a tri-state:

• enabled
• disabled
• promoted to error

[kfcripps]

IMO, explicit "disable" should always be honored.

Do you mean even for message types that are originally errors? Or only for message types that are originally warning/info and get promoted to error via -Werror?

[asl]

Do you mean even for message types that are originally errors? Or only for message types that are originally warning/info and get promoted to error via -Werror?

For only promoted ones.

[kfcripps]

@fruffy @vlstill @ajwalton If no other opinions, I will proceed with implementing Anton's suggestion later.

[ajwalton]

I agree that messages that were originally errors should never be demoted. The compiler might not generate valid output when an error is generated, so we can't allow these to be ignored.

[vlstill]

I agree. In summary a tri-state for warnings where disabled takes precedence over promoted with -Werror, there is no possibility to change level of errors. Although we want probably an error when commandline options are inconsistent (e.g. non-wildcard -Werror=diag && -Wdisable=diag or both wildcard -Werror && -Wdisable).

There is also a way to wildcard-promote all infos to warnings (-Wwarn) again, I think in this case disable should take precedence. And warnings can be demoted to infos (only one-by-one it seems from help). In general, I'd say the non-wildcard variants should be honored or they should lead to commandline parsing errors if they can't be honored. For the windcard once, -Wdisable should probably conflict with any other wildcard (also it would make more sense to have -Wno-warn & -Wno-info than -Wdisable in my opinion).