[#4365] Do not allow --Wdisable or --Wwarn to demote errors. Allow --Winfo=diagnostic to work for diagnostics that can be both warnings and errors.

[kfcripps]

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): [--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  --Wdisable=invalid:

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  --Winfo=invalid
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()
       ^

[vlstill]

(I've restarted a failed test-p4c job as it looked like a transient failure)

[kfcripps]

I went with the second approach mentioned in #4365 (comment) (and therefore reverted #4197).

@dmatousek @vlstill @ajwalton Ready for review.

[kfcripps]

Updated the PR based on feedback in #4365 (comment). New output for e.g. p4test testdata/p4_16_errors/accept_e.p4 --Wdisable=type-error, p4test testdata/p4_16_errors/accept_e.p4 --Wwarn=type-error, p4test testdata/p4_16_errors/accept_e.p4 --Winfo=type-error:

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

[fruffy]

Do not have a strong opinion on this topic but this seems like a good change to me.

[fruffy]

Just return false here? Once otherDiagnostic is set the result is always false.

[fruffy]

Just a nit, maybe also check that the value is within both ends of the error range.

[fruffy]

Add a comment here that actions for errors can never be overridden. Maybe even return DiagnosticAction::Error always?

[fruffy]

There is a ingnored typo in the comment below.

[kfcripps]

Maybe even return DiagnosticAction::Error always?

Do you mean like this:

if (defaultAction == DiagnosticAction::Error) return DiagnosticAction::Error;

What is the benefit?

[fruffy]

Well, it would make it explicit and one could not circumvent this invariant by passing in another default action. But if someone goes through the trouble of doing that they may have a reason.

[fruffy]

~~Also I feel like the error catalogue should be a map? But this can be done some other time. ~~

Nevermind, it is. But it is not 2-dimensional.

[kfcripps]

@fruffy I have implemented several of your suggestions. Please let me know if the changes look ok and I will rebase & merge this.