Implement ability for project properties to suppress and elevate warnings

[jeffkl]

Introduce MSBuildTreatWarningsAsErrors property that when set to true will treat all warnings as errors when building a project.
Introduce MSBuildWarningsAsErrors property which is a semicolon delmited list of warning codes to treat as errors when building a project.
Introduce MSBuildWarningsAsMessages property which is a semicolon delimited list of warning codes to treat as low importance messages.

This allows users to control warnings at the project level via standard MSBuild properties. They can be set in a project, an import, or from the command-line.

The limitation here is that the warnings can only include ones that occur during a build. This is because the properties are not read until after parsing so warnings generated during parse/evaluation happen too soon. For these warnings, users will only be able to treat them as errors/messages with the /WarnAsError and /WarnAsMessage command-line arguments.

Closes #1886

[jeffkl]

FYI @jaredpar

[jaredpar]

Nice!

[cdmihai]

Hopefully this is the only place where MSBuild evaluates a project file during build. :)

[jeffkl]

Do you foresee any performance degradation by adding this call? I assumed it was safe since the project has "started" building...

[cdmihai]

It runs once for each project evaluation, so it should scale linearly with number of evaluations (Project nr x (msbuild tasks per project)). I think ~5000 projects is considered "outlier big".

[cdmihai]

Offline discussion: My bad, this actually happens only once per node.

[cdmihai]

Check this works on a p2p node as well. Nodes initially assign a temp project ID (maybe the invalid one?) and I don't remember whether they get the global ID before or after they evaluate the project.

[cdmihai]

What if the user defines TreatWarningsAsErrors=True and also WarningsAsErrors="a;b;c;d"? I'd expect msbuild to treat only the specified warnings as errors, as opposed to ignoring the warning list.

[jeffkl]

At the moment, MSBuildTreatWarningsAsErrors will override any list. This is the same way that argument works for the compilers.

[cdmihai]

Cool, didn't know. Go with the same behaviour as the compilers then. Except for the warning list separator :)

[cdmihai]

I'd only keep semicolons, as they are the msbuild approved way of separating things. And then maybe use ExpressionShredder.SplitSemiColonSeparatedList

[jeffkl]

Done

[cdmihai]

If you are using a concurrent dictionary, rather than locking, you can use a thread safe Lazy to initialize it, and then not use the external lock when using it.

[jeffkl]

I tried this out but hit a snag. I'm using the state of null as an indicator if any warnings should be treated as an error/message. If I use a Lazy<T>, then I would have to expose it as such so that the caller can determine if HasValue is true. Leaving it the way it is, the property remains of type IDictionary and a simple null check is done. If its okay to have IBuildEventSink have a property that is of type Lazy<T>, then I'll do it.

[cdmihai]

What if the user has the same warning code as both a message and an error? I'd expect error to win, on the grounds that errors are more serious than messages. Or maybe emit a warning saying that both were specified and the intent is ambiguous. The user can then treat that new warning as a message (but hopefully not as an error as well) :)

[jeffkl]

Suppressing a warning overrides elevating one. This is so you can treat all warnings as errors but still turn a few of them off.

[cdmihai]

I'd rename WarningsAsErrors/Messages to something like GlobalWarningsAsErrors/Messages

[jeffkl]

The name is technically part of our public API already as a property to BuildParameters. I could rename the internal property on EventSourceSink if you feel strongly about it.

[cdmihai]

It was more of a nit. Thought it's worth making it clear in the ILoggingService that WarningsAsErrors is the global, master switch and not the local per project one.