Create separate attribute for warning behavior differences

[jtschuster]

To help track differences in the warning behavior of the trimming related tools, this modifies how adds UnexpectedWarning, and requires an issue link to be provided when there is a ProducedBy argument to the constructor. To enforce that either both a ProducedBy and IssueLink is provided or neither, the ProducedBy property is removed and is provided as the second to last argument, and IssueLink is provided as the last argument.

• ExpectedWarning means that the correct behavior is to warn. Any attributes that expect it only from a subset of the tools must provide an issue link. (These are mostly blank strings now, though)

• UnexpectedWarning means that this warning is not the correct behavior. These attributes always include a ProducedBy anrdshould link to an issue.

Changes

• Look for a Tool attribute argument in the second to last position of ExpectedWarning and Unexpected warning when a ProducedBy property is not found.
• Find a replace existing ExpectedWarnings to use the new constructors.
• Adds issue links within AttributedMembersAccessedViaReflection.cs and in some places in ArrayDataFlow.cs

Follow up

• Add issue links to each attribute and rename some ExptectedWarnings to UnexpectedWarnings.
• Use the same constructor signatures in LogContains / LogDoesNotContain and add UnexpectedLogContains / UnexpectedLogDoesNotContain.

[vitek-karas]

Just suggestions:

• I would probably leave the ExpectedWarning and ExpectedMissingWarning unified. With a property (ProducedBy or something else) which differentiates the case where some tools don't have the correct behavior. To me this would read better. Maybe we could change the property be the negation - so for a case where there should be a warning, but analyzer doesn't produce it (but the other tools do) - that would be annotated with [ExpectedWarning(NotProducedBy=Analyzer, Reason="...")]
• I like the UnexpectedWarning addition to clearly separate "buggy" behavior from the expected correct behavior. I would amend this by also adding a ProducedBy (or similar) property to cover the cases of difference in behavior - for example if there's a case where analyzer produces unexpected warning, but illink and ilc don't - that would make sense to annotate with [UnexpectedWarning(ProducedBy=Analyzer, Reason="...")].

Basically, the two attributes would act as notations which behavior is correct (which is currently missing), but would allow for exceptions per-tool, with attached justification).

You could also modify the attributes to not rely on properties but on .ctors instead to enforce the Reason if one of the tools deviate from the correct behavior, so:

class ExpectedWarning
{
    public ExpectedWarning(string warningCode);
    public ExpectedWarning(string warningCode, Tool notProducedBy, string reason);
}

[jtschuster]

Maybe we could change the property be the negation

My concern with that is that if it's an argument, it's not immediately clear that it's now NotProducedBy, and since we've used it as ProducedBy previously it might cause confusion. Also, in ExpectedWarning, NotProducedBy makes sense, but ProducedBy makes more sense for UnexpectedWarning, and it might be confusing to have it mean different things.

I would probably leave the ExpectedWarning and ExpectedMissingWarning unified.

You could also modify the attributes to not rely on properties but on .ctors instead to enforce the Reason if one of the tools deviate from the correct behavior

This was my initial plan, but the test validators look for the ProducedBy property on any of the ExpectedWarning / LogContains attributes. This can be changed, though. I assumed ProducedBy was more prevalent, but looking into it now it's really just a few attributes. If we update the places that look for ProducedBy to also look at custom attribute arguments of type Tool we can make that work.

Another thing to consider is the params string[] messageContains argument for ExpectedWarning. In my opinion, the assertion reads best when the messageContains args are right next to the warning code, but with arguments instead of properties we'd need to put it last or make it a normal string[] parameter. I think it wouldn't be too bad to have overloads where messageContains is string or string[], especially with collection expressions. The readability would still probably be better than having different attribute names.

This is what I'm planning to update the PR to:

class ExpectedWarningAttribute : Attribute
{
    public ExpectedWarningAttribute(string warningCode, params string[] messageContains) {}
    public ExpectedWarningAttribute(string warningCode, string messageContains, Tool producedBy, string issueLinkOrReason) {}
    public ExpectedWarningAttribute(string warningCode, string[] messageContains, Tool producedBy, string issueLinkOrReason) {}
}

class UnexpectedWarningAttribute : Attribute
{
    public UnexpectedWarningAttribute(string warningCode, string messageContains, Tool producedBy, string issueLinkOrReason) {}
    public UnexpectedWarningAttribute(string warningCode, string[] messageContains, Tool producedBy, string issueLinkOrReason) {}
}