Expose Properties and Items data enumeration

[JanKrivanek]

Fixes #10770

Context

The ProjectEvaluationFinishedEventArgs and ProjectStartedEventArgs Properties and Items members are nongeneric and need involved custom code to enumerate. While we do not want to expose exact internals to prevent ourselves from future changes - we should at least expose the way how the data can be traversed in string form.

Previous state

Users needed to recreate EnumerateItems and EnumerateProperties methods, which calls other internal helper methods and together use several internal types (ItemDictionary, IItem, IProperty, IKeyed, etc.) - so some of the cases need to be skipped by the custome code, and hence despite involved, the code is usually single purpose.

Sample of that can be found e.g. here:

https://devdiv.visualstudio.com/DevDiv/_git/vs-code-coverage/pullrequest/582161?path=/src/cts/Build/BinlogReader.cs&version=GBmain&line=108&lineEnd=128&lineStartColumn=1&lineEndColumn=20&type=2&lineStyle=plain&_a=files&iteration=11&base=0

with

https://devdiv.visualstudio.com/DevDiv/_git/vs-code-coverage/pullrequest/582161?path=/src/cts/Build/MSBuildHelper.cs&version=GBmain&line=36&lineEnd=60&lineStartColumn=1&lineEndColumn=6&type=2&lineStyle=plain&_a=files&iteration=11&base=0

So something like:

        // the helper needs to be defined as well - attempting various casting. Similar to the internal EnumerateItems linked above
        MSBuildHelper.EnumerateItems(projectEvaluationFinishedEventArgs.Items, entry =>
        {
            if (entry.Key.Equals(ProjectReferencePropertyName))
            {
                if (entry.Value is ITaskItem)
                {
                    string referenceProjectPath = ((ITaskItem)entry.Value).ItemSpec;

                    // ...
                }
            }
        });

Proposed

The discussed code would change to:

        foreach ((string itemType, IItemData itemValue) tuple in projectEvaluationFinishedEventArgs
                     .EnumerateItems()
                     .Where(i => i.itemType.Equals(ProjectReferencePropertyName)))
        {
            string referenceProjectPath = tuple.itemValue.GetEvaluatedInclude();
            // ...
        }

The enumeration is as well demonstrated by tailored unit tests: src/Build.UnitTests/BuildEventArgsDataEnumeration.cs

Notes

The decision on extension methods vs. member methods

In some cases (e.g. the EnumerateMetadata and GetEvaluatedInclude for ITemData) it was necessity not to break backward compatibility of interfaces. Nor could I introduce new interfaces and return wrapped types - as some of the existing consumers (including our code) relies on casting of enumerated data to specific types. Wrapping the data would break this.

As for the EnumerateItems and EnumerateProperties methods for data on BuildEventArgs - I have no strong opinions on this, and ready to flip to member methods if it seems better fit.

Testing

• Added enumeration unit tests
• Manual (via project the prototype project that cosumed the data)

FYI @jakubch1

[JanKrivanek]

Can you write up some motivation for doing it with extension methods, and what an old/new calling pattern would be?

@rainersigwald I've added a motivation + sample to the description - please have a look if it makes sense.

As for 'extension vs member methods' - no strong opinions :-) it can be changed easily if it feels more proper.

[JanKrivanek]

The discussed code would change to:

        foreach ((string itemType, IItemData itemValue) tuple in projectEvaluationFinishedEventArgs
                     .EnumerateItems()
                     .Where(i => i.itemType.Equals(ProjectReferencePropertyName)))
        {
            string referenceProjectPath = tuple.itemValue.GetEvaluatedInclude();
            // ...
        }

Is this what we want to expose, or should it be something more like what we expose on Project{Instance}, such as ProjectInstance.GetItems(string type)?

I'd prefer to avoid that - while already public, those types are R/W. So let's expose a common base of those types - that still gives read access to important data (EvaluatedInclude, Metadata a string-string map) - which is more than what's made available today, and we can allways expose more if needed.

What do you think?

[JanKrivanek]

The discussed code would change to:

        foreach ((string itemType, IItemData itemValue) tuple in projectEvaluationFinishedEventArgs
                     .EnumerateItems()
                     .Where(i => i.itemType.Equals(ProjectReferencePropertyName)))
        {
            string referenceProjectPath = tuple.itemValue.GetEvaluatedInclude();
            // ...
        }

Is this what we want to expose, or should it be something more like what we expose on Project{Instance}, such as ProjectInstance.GetItems(string type)?

I'd prefer to avoid that - while already public, those types are R/W. So let's expose a common base of those types - that still gives read access to important data (EvaluatedInclude, Metadata a string-string map) - which is more than what's made available today, and we can allways expose more if needed.

What do you think?

Actualy it's even more complicated - we pass only ProjectItem and ProjectItemInstance to the EventArgs during creation, but on node-2-node translation and binlog de/serialization those can be changed to TaskItemData. So without breaking existing public API we do not have any common type hierarchy. At the same type we cannot wrap - not to break consumers that cast the items.

So the way around is via an accessor object (something like flyweight pattern) - the target object is still just original object and can be casted, but the iterator provides accessor methods for strong-type-like accessing EvaluatedInclude and metadata.