Support for external project collections linking. Add object model re…

[svetkereMS]

No description provided.

[dnfclas]

All CLA requirements met.

[rainersigwald]

I haven't made it through the full review, but let me dump what I do have before I check out for the long weekend.

Meta-comments so far:

• What are the rules we should follow in the future regarding the link when adding to or altering the public API surface? Is it something like "all public APIs of an object that implements ILinkableObject should use the Link member?
• Would it be possible to write a Roslyn analyzer to enforce those rules? We don't necessarily have to do this now but it might be nice to have the backstop.

[svetkereMS]

I haven't made it through the full review, but let me dump what I do have before I check out for the long weekend.
Meta-comments so far:

What are the rules we should follow in the future regarding the link when adding to or altering the public API surface? Is it something like "all public APIs of an object that implements ILinkableObject should use the Link member?
Would it be possible to write a Roslyn analyzer to enforce those rules? We don't necessarily have to do this now but it might be nice to have the backstop.

First, regarding how to manage “Link” layer when changing the MSBuild API going forward.

That’s depend on the type of change, but basically the way I see it is this:

1. Changes to existing api (new public method/property in existing classes)

This might require a new methods or properties added to corresponding “Link” class, not necessarily exactly the same, but whatever is needed for exported to facilitate the new feature.

The rule would be to add a new “virtual” (not abstract) method with a default implementation on the Link class. It is ok Implementation to be “throw new NotImplementException()”.

2. Adding new public Types. Here there are two options
   A) Add a new “Link” class similar to the existing patter.
   Design the new class in a way it can be remoted without “Link”, for example if all public surface area are “virtual” methods/properties (or interfaces – but MSbuild does not use interfaces) we don’t need Link. Or if the type is “read only snapshot” and only exposes simple types (like strings, integers etc, and not references to other non – remotable, msbuild objects) and has a public c-tor that one can create such object from these simple types we don’t need to add “Link” either.

I personally don’t see it as high importance to make new API surface a “remote”-enabled right away. We should highly discourage usage of “back-door” aka GlobalProject collection access in general, and for legitimate usage it would always require a change in our external projects provider anyway, so as I see that “remoting” capability could be added later and only when actually needed (basically on Demand).

As for analyzer. I personally am not familiar with the extend of Roslyn analyzers capabilities so I don’t know the answer for that question.

[rainersigwald]

I don't think I have any major objections.

We're hoping to minimize use of this over time, right? How can we make sure we have appropriate telemetry/complaining about this so that VS extension authors feel some pressure to move to the VS APIs you'd rather they use?

[Mohit-Chakraborty]

We're hoping to minimize use of this over time, right? How can we make sure we have appropriate telemetry/complaining about this so that VS extension authors feel some pressure to move to the VS APIs you'd rather they use?

My opinion is that we don't need to do anything different here. A holistic approach to discourage usage of GlobalProjectCollection is certainly welcome.
Maybe we can start by putting out blog posts and such laying out the issues code can run into if it relies on GlobalProjectCollection instead of querying the project system directly using IVs* APIs. Perhaps add to the doc about the class laying out its limitations and alternatives.
What do you think @svetkereMS ?

[svetkereMS]

We're hoping to minimize use of this over time, right? How can we make sure we have appropriate telemetry/complaining about this so that VS extension authors feel some pressure to move to the VS APIs you'd rather they use?

My opinion is that we don't need to do anything different here. A holistic approach to discourage usage of GlobalProjectCollection is certainly welcome.
Maybe we can start by putting out blog posts and such laying out the issues code can run into if it relies on GlobalProjectCollection instead of querying the project system directly using IVs* APIs. Perhaps add to the doc about the class laying out its limitations and alternatives.
What do you think @svetkereMS ?

As for proliferating the use of "Link" mechanism as a means to support (or even worse - extend and encourage) "backdoor" GPC access in VS - totally agree, I don't see any effort to optimize that as a positive, and we do need to deter people of using GPC instead. We must start with the bunch of rogue (lazy hack) features that exist in our VS code first before we ask customer not to do what we have already done :).

Now, on the other side, it is possible we find a legitimately useful cases for the MSBuild remoting feature. For example a good candidate is offloading CPS non-default config (platforms) evaluation OOP and use the "Link" to facilitate the integration of results into CPS API ecosystem (which is very MSBuild based). This will help remove huge Memory burden while allow us to scale (parallelizing) the work similarly to what we get in OOP evaluation for CSProj. Since the official CPS api include MSBUild types, it is no other easy way around than using Link mechanism there (it is not backdoor in this case).

Anyway I would prefer not to speculate too much on future possibilities now to much and focus on what we need at this moment. It is hard to predict how we end up really ...

[cdmihai]

Putting two operational modes (link vs real) in every Project*Element class and separating them with an if feels to me that it lowers the maintainability / readability of the code. To alleviate this I propose that you separate the two operational modes such that one can reason about each mode in isolation. This would involve something like:

• Unchanged surface APIs. The public Project*Element APIs would remain unchanged, and they would remain to be used by the rest of the msbuild code (e.g. ProjectParser and Evaluator would still use the old Project*Element types). This avoids having to change the existing type casts.
• Extract an interface with defaults / abstract class for each Project*Element class
• The implementation of each Project*Element class would be carved out and moved to a parallel hierarchy that implements the newly extracted supertypes. For example, a ProjectChooseElement would have its implementation moved into a ProjectChooseElementImpl : IProjectChooseElement, and ProjectChooseElement methods would delegate into corresponding ProjectChooseElementImpl methods.
• The proxy objects would be a second implementation of the IProject*Element interfaces: the Project*ElementLink hierarchy.
• Project*Element classes would have a field pointing to its corresponding IProject*Element implementation and delegate their methods to the interface. The interface would hide either a real Project*ElementImpl implementation, or a Project*ElementLink proxy.

[svetkereMS]

Putting two operational modes (link vs real) in every Project*Element class and separating them with an if feels to me that it lowers the maintainability / readability of the code. To alleviate this I propose that you separate the two operational modes such that one can reason about each mode in isolation. This would involve something like:

Unchanged surface APIs. The public ProjectElement APIs would remain unchanged, and they would remain to be used by the rest of the msbuild code (e.g. ProjectParser and Evaluator would still use the old ProjectElement types). This avoids having to change the existing type casts.
Extract an interface with defaults / abstract class for each ProjectElement class
The implementation of each ProjectElement class would be carved out and moved to a parallel hierarchy that implements the newly extracted supertypes. For example, a ProjectChooseElement would have its implementation moved into a ProjectChooseElementImpl : IProjectChooseElement, and ProjectChooseElement methods would delegate into corresponding ProjectChooseElementImpl methods.
The proxy objects would be a second implementation of the IProjectElement interfaces: the ProjectElementLink hierarchy.
ProjectElement classes would have a field pointing to its corresponding IProjectElement implementation and delegate their methods to the interface. The interface would hide either a real ProjectElementImpl implementation, or a ProjectElementLink proxy.

There are downsides on the above proposal:
(first note that this is the way we applied the "Link" layer on Project element and learned the downsides from there).

1. Increase the memory footprint and DOUBLE the number of allocations. Note with experiments I did on these (adding additional allocations and new) when I first observed that (by simply add a object foo = "new" object(); to ProejctElement and some evaluation objects) was ~ 5-10% memory increase and close to 5-8% decrease in evaluation performance.

2. because of thight coupling between public and internal surface area, we need to address case by case each object and put non-trivial effort to define the proper interfaces.

3. The code area that we need to significantly change is very large. Construction classes combined are ~8k lines of code so we can expect at least as much new code churn. (entire change inside production code now is ~3k)

4. we need to duplicate some logic between Link/non link implementations.

5. Breaking each class to Shim and Impl has its own negatives for maintainability. (each change can be potentially work on 1 interface and 2 classes, instead of single class).

Currently we do have ~80 places (roughly 80 lines of code) affected by this same pattern. Note that these are all very stable (and generally simple) public API (typically get/set) that are rarely changed (if at all).

(50% of this is in ProjectRootElement. )

So IMO that is not a good trade-off, but it is you guys call here. I'll open a mail thread on that since it can be a significant work associated ...

[dasMulli]

I'd appreciate a bit of documentation, e.g. adding something to /documentation.
Is there anything to keep in mind / test when adding new / modifying APIs?
I'm not even sure I understand what this feature is aimed to be used for..