Throw exception or log warning upon drive enumeration glob anomaly detection

[mruxmohan4]

Context

Glob anomalies drive enumeration issue: #3642 (comment). Properties that evaluate to empty result in drive enumeration.

Changes Made

Logs an error via ProjectFileErrorUtilities.ThrowInvalidProject(...) with the resource name WildcardResultsInDriveEnumeration when \**, <drive>:\**, or <drive>:<any number of slashes>** are detected on the leftmost side while parsing and the environment variable MSBUILDFAILONDRIVEENUMERATINGWILDCARD is set to 1 (with the exception of one case listed in "Notes" below which simply logs an error). Note that a warning is logged (rather than an error) with the same resource name when the environment variable is set to 0.

Regarding more recent changes, logging a warning/throwing an InvalidProjectFileException has become more unified and placed in EngineFileUtilities.cs. Logging a warning/throwing an exception with the resource name requires extra information, including a SearchAction and ExcludeFileSpec, which are returned from FileMatcher into various codepaths (CreateItem & EngineFileUtilities), which obtain additional parameters from other codepaths, such as Evaluator, Expander, ItemGroupIntrinsicTask, LazyItemEvaluator.IncludeOperation, etc., to better identify the logging mechanism and attributes that should be passed to EngineFileUtilities.cs upon logging a warning/throwing an exception. There is no custom exception that is caught and then converted into an InvalidProjectFileException (all of the functionality that came with a custom exception has been removed with a prior commit for these changes). When the env var is set to 1, a nearly immediate return from functions occurs, as we want the build to fail and throw upon detection of a drive enumerating wildcard.

Testing

Larger integration tests with empty properties/imports were created to trigger the issue in targets. Smaller tests in FileMatcher_Tests.cs were created to check for particular wildcard patterns that should result in the custom exception being thrown.

Unit tests were added to: FileMatcher_Tests.cs, ProjectItem_Tests.cs, CreateItem_Tests.cs, and ProjectItemInstance_Tests.cs.

Notes

1. Uses MSB5029 error code (see shared resx).
2. For CreateItem, an error is logged instead of using ProjectFileErrorUtilities.ThrowInvalidProject(...)
3. The resource name used was WildcardResultsInDriveEnumeration to pass on element locations to InvalidProjectFileException.

[dnfadmin]

All CLA requirements met.

[dnfadmin]

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ mruxmohan-msft sign now
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[Forgind]

I hadn't really properly looked at what you were trying to do here before, but @ladipro made a similar PR here that prevented drive enumerations when the condition was false specifically. It would probably be easy to glom onto that to achieve what you're trying to do here—but although drive enumerations are probably not intended, this might be a bit much of a breaking change for 17.1. Might be good enough with a change wave? Not sure.

[mruxmohan4]

@Forgind @ladipro Right, that PR did alleviate drive enumerations, according to #3642 (comment), but this was an attempt to improve logging and surface an exception. Is this necessary given the following that was already completed in #5669, or does the existing logging below not surface all the way to the user?

                            if (MSBuildEventSource.Log.IsEnabled())
                            {
                                MSBuildEventSource.Log.ExpandGlobStop(_rootDirectory, glob, string.Join(", ", excludePatternsForGlobs));
                            }

[Forgind]

Users can get that information, but they have to specifically ask for it and almost always will not. I doubt anyone would find it unless they specifically looked.

[ladipro]

@mruxmohan-msft I'm afraid that requiring an opt-in via the MsBuildCheckWildcardDriveEnumeration environment variable is not going to be very helpful. The user has to be notified even if they don't expect this issue, i.e. they have not explicitly opted in.

We should try to come up with a solution which we can enable by default, while causing little-to-no pain to users who enumerate full drives intentionally. Perhaps a build warning would be acceptable as along as it:

• Is surfaced under default logging verbosity in both command line build (msbuild and dotnet build) and in Visual Studio.
• Can be easily disabled.

[rainersigwald]

I think this is starting to look great! I pointed out many smaller nits.

[ladipro]

I have not reviewed all files yet but I may not have more time today so publishing what I have. Overall I think the changes look great.

[mruxmohan4]

Is there a more optimal way to clean up the logic within these switch statements? I tried to reduce as many lines of code as possible.

Also, are the conditions correct? I want to make certain that I'm not causing any false positive/negative warnings or errors (in spite of test coverage), in addition to having the wrong attributes and elements associated with the warnings/errors (are there preferred ways to include specific metadata in tests without checking for particular messages contained in logs, as those may change over time?). This section might be more clear once I complete test cleanup and add more inline data in ProjectItem_Tests.cs and ProjectItemInstance_Tests.cs.

[ladipro]

Is there a more optimal way to clean up the logic within these switch statements? I tried to reduce as many lines of code as possible.

Just a few random ideas:

1. You could try to flatten the code to have only one switch with pattern matching. Something like: https://dotnetfiddle.net/DUHvNn
2. Or maybe it would be better to refactor the inner switches into their own helper method each.

[rainersigwald]

IIRC I started experimenting with approach 1 for this problem and didn't like where I wound up, so I think I'd lean toward approach 2, @mruxmohan4. In the general case I like 1 better though.

[Forgind]

I haven't looked to see if this is viable in this case, but it can sometimes be fairly clean to have two separate switches, one to select relevant arguments and one to select a delegate to apply to them. Then you can just apply it at the end.

[mruxmohan4]

I went ahead with approach 2 to make it less clunky in the inner switch statements.

[ladipro]

I've left a few comments inline, mostly nits.

[ladipro]

Is there a more optimal way to clean up the logic within these switch statements? I tried to reduce as many lines of code as possible.

Just a few random ideas:

1. You could try to flatten the code to have only one switch with pattern matching. Something like: https://dotnetfiddle.net/DUHvNn
2. Or maybe it would be better to refactor the inner switches into their own helper method each.

[rainersigwald]

IIRC I started experimenting with approach 1 for this problem and didn't like where I wound up, so I think I'd lean toward approach 2, @mruxmohan4. In the general case I like 1 better though.

[Forgind]

This is really more of a quick pass than a full review, but it sounded like rainersigwald gave more complete comments.