Roslyn is unable to add diagnostic nodes properly in multi-targeting projects

[jasonmalinowski]

Roslyn is responsible for populating the individual diagnostics and source generators under an analyzer node. In a multi-targeting case, we have logic that attempts to walk up the hierarchy of nodes to figure out which TFM is active. For example, consider this screenshot:

The "SampleSourceGenerators" node is produced by CPS. Roslyn then walks up that node and finds the netcoreapp3.1 node, which has some special capabilities, namely a capability that starts with $TFM: -- that is then used to figure out which TFM we're looking under.

Looking at the Roslyn code however, I'm guessing this has been broken since we removed the IVsHierarchy wrappers around each target a long time ago. The problem is we may know which TFM we're looking at, but we have no way to figure out which IWorkspaceProjectContext this actually matches. The code currently looks at each IVsHierarchy for our project, and asks the IVsHierarchy for it's TFM. That works if we had separate IVsHierachies for each TFM but we don't (and we shouldn't...getting rid of that code was good.) Right now you'll see the feature work in one of the targets under Dependencies simply because when you ask an IVsHierarchy that multi-targets for it's TFM, you just get the first one.

Note the user impact here is worse now -- previously this only meant you couldn't browse diagnostics provided by an analyzer. This is also where we're showing documents from source generators, so you can't browse to other options.

My proposal is the project system should, rather than giving the $TFM: capability, give some capability that is:

1. The ID directly obtained from IWorkspaceProjectContext.Id that matches up to the Roslyn project created for that node. This means we could just match things directly. Note this option though presumes that the capabilities of a node can change if the design time build triggers a project reload.
2. The unique name given to IWorkspaceProjectContextFactory.CreateProjectContext(). This will be stable; there's also precedent for this being exposed as VSHPROPID_ActiveIntellisenseProjectContext from the IVsHierarchy. Risk here is if the name has spaces, since I guess capabilities can't have spaces?

Otherwise we need some way to directly be told the TFM for a project, but I don't like binding directly to TFM here since that will require this to be redesigned again if we ever have multiple Roslyn projects for the same TFM, say if we ever do the work to expose both debug/release, etc.

[drewnoakes]

What information is needed to look up an IWorkspaceProjectContext if the TFM is insufficient?

The target nodes (which currently have the $TFM:* flags) are agnostic of any particular type of dependency, and should probably not have Roslyn concepts baked into them. We populate the dependencies tree from MSBuild items and don't have any particular knowledge of Roslyn workspaces, so we'd have to look them up. I'm unclear how the project system would be in a better position to do this during tree construction than the Roslyn code that populates the analyzer subtrees would.

For legacy projects, Roslyn creates the Analyzers group node, nodes for individual analyzers, and diagnostic nodes beneath those. In CPS, the .NET Project System creates the first two (group and analyzer nodes) and Roslyn only populates the details beneath. I'm left wondering whether it makes sense for Roslyn to build this entire tree in both legacy and CPS based projects. I'm not familiar with the history here, so please fill me in if there's a good reason for the project system to be creating these first nodes.

[jasonmalinowski]

I think @tmeschter can chime in on the comparison to the legacy project case. I believe Roslyn creates the legacy analyzer nodes in the legacy project system case was simply a cost saving measure: updating Roslyn was cheaper than updating the legacy project system.

What information is needed to look up an IWorkspaceProjectContext if the TFM is insufficient?

My worry is scenarios like the project system implementing the long-standing request to have both debug/release projects sent over to the Roslyn workspace -- at that point we'd have multiple projects and wouldn't know which one to pick. Also how does this work with net5.0 also embedding platforms? If you have a project targeting both net5.0-windows and net5.0-ios, how does that work? (I don't even know if that's legal so feel free to correct me.)

[tmeschter]

I think @tmeschter can chime in on the comparison to the legacy project case. I believe Roslyn creates the legacy analyzer nodes in the legacy project system case was simply a cost saving measure: updating Roslyn was cheaper than updating the legacy project system.

Analyzer assemblies are project inputs just like references and source files. As such, the project system has an interest in displaying them to the user regardless of whether or not the language service shows the diagnostic nodes under them. Ideally the legacy project system would have been responsible for the creation of the Analyzers group node and the analyzer assembly nodes, but the only way to get the feature done in time was to implement it in the language service.

My worry is scenarios like the project system implementing the long-standing request to have both debug/release projects sent over to the Roslyn workspace -- at that point we'd have multiple projects and wouldn't know which one to pick. Also how does this work with net5.0 also embedding platforms? If you have a project targeting both net5.0-windows and net5.0-ios, how does that work? (I don't even know if that's legal so feel free to correct me.)

I view the idea of sending both debug and release configurations to the Roslyn workspace as highly theoretical at this point. That would require some amount of UX redesign and would significantly alter the concept of the "active" configuration across VS. In fixing this problem I don't think we should spend much effort trying to future-proof against an as-yet undesigned feature.

[drewnoakes]

Having the project system show the analyzer assembly nodes is reasonable. Aside: they are actually handled differently to other types of dependencies as they're not always provided via evaluation, and usually require a DTB to complete before they are known. You can see them appear slightly later than other dependency types when loading a project.

how does this work with net5.0 also embedding platforms? If you have a project targeting both net5.0-windows and net5.0-ios, how does that work? (I don't even know if that's legal so feel free to correct me.

Yes it is legal and should be a relatively common scenario, so we definitely have to consider it. Having said that, the tree isn't working as expected. We will fix that (#6826).

The intention is that each platform receives its own TFM node in the tree. The TFM will be net5.0-windows for example.

@jasonmalinowski would having a distinct TFM string be sufficient? If you need to know debug/release configuration you could use the active configuration for guidance. The intent (#3642) is for the tree to reflect the active configuration.

[jasonmalinowski]

@drewnoakes I don't like having the TFM because it tends to lead to abuse or bugs, but if there's no practical alternative then we can do that.

[drewnoakes]

I'm certainly open to alternatives so long as they're generally applicable, and we can get the information we need.

Exchanging data through items in the solution tree is a pain. Mushing specially encoded values in the capabilities property string feels inadequate. We have a similar issue exposing package identity/version data to NuGet.Client. I'm not sure there's a better approach for hierarchy node though.

[davkean]

DTE.ProjectItem.Properties is exactly that exchange mechanism.

[tmeschter]

There are two distinct questions here:

1. What information is needed to allow Roslyn to match the an IWorkspaceProjectContext up to the corresponding target framework node under the Dependencies node?
2. How do we expose that information both to the IWorkspaceProjectContext and through the items under the Dependencies node?

Right now we're exposing just the TFM through a capability on the node. Exposing the TFM seems reasonable but maybe not fully general, but doing so through a capability is something of a hack.

I propose that instead of the TFM, we utilize the full set of project configuration dimensions and values. Roslyn would need to treat this as a black box and avoid interpreting the names and values. This is important because it would sometimes, but not always, include the TFM--if you're not multitargeting, the TFM is not considered part of the configuration.

The project system would need to pass this black box to the IWorkspaceProjectContext and expose it through the DTE.ProjectItem.Properties collection or through an IVsHierarchy property.

Assuming we could do this, would this approach address everyone's concerns?

[jasonmalinowski]

So I had a few concerns about us just passing around TFM:

1. Right now Roslyn doesn't know the TFM for a project, and that's often good; whenever somebody starts asking "how do I know the TFM of a project?" we can quickly try to figure out what they're actually trying to do and steer them away.
2. TFM is an opaque string to us, so we can use TFM to choose between multiple targets, we're implicitly adding an implementation assumption that the targets displayed in solution explorer correlate to 1:1 with the the projects Roslyn is given, and TFM is always a sufficient way to differentiate between them. My concern for things like the net5-windows vs. net5-ios is fundamentally we on the IDE team don't even know what these mean (both in we can't decipher them, and also seriously I don't even know what those mean as a user).

If updating the project system to give us TFM avoids a lot of extra design work I'm OK with that (especially if we can then fix this in 16.9); I don't like it but I don't want to let perfect be the enemy of helping customers.

[drewnoakes]

@jasonmalinowski I'm considering the following steps:

1. Calling IWorkspaceProjectContext.SetProperty with properties that uniquely identify the project's configuration.
2. Adding these properties to the node in the dependency tree so that Roslyn (and other components) can differentiate correctly.

Open questions:

1. What properties are required to differentiate the nodes? Is TargetFramework enough?
2. Is putting these values in the capability flags the right thing, or can DTE properties work? I seem to remember trying to use DTE properties when working on the NuGet attached collection providers (for package/project reference desendents) but having difficulty.

I suspect this won't make 16.9 given our other commitments and the magnitude of the change involved.

I've a spike that adds project configuration properties to the tree's target nodes, as you can see here. There are still some bugs to iron out, but it's progress.

[davkean]

There's already a context id that we pass to Roslyn, just reuse it as the identifier.

[jasonmalinowski]

I suspect this won't make 16.9 given our other commitments and the magnitude of the change involved.

As a short fix if you just give us a TFM through IWorkspaceProjectContext.SetProperty we can at least make that work. I imagine that's a relatively small change on your side. It's not my preferred approach but if that's what we can do to fix this for 16.9 I'll take it.

There's already a context id that we pass to Roslyn, just reuse it as the identifier.

@davkean is correct here: when a project is created in Roslyn a unique name is given to the project that comes from the project system. That name is an internal name (not displayed to the user) and is an opaque string, but is unique. If a node had an API to give the equivalent string that'd be good for us. I recognize that the dependencies node and the design time build results are produced via two different areas of the project system; my hope is even if we have to duplicate the generation logic in both of those places in the repo that's at least better than it being duplicated across multiple repos.

[drewnoakes]

I've found the context ID @davkean is talking about. It has form:

C:\Project\Project.csproj (Debug;AnyCPU {72B509BD-C502-4707-ADFD-E2D43867CF45})

Tree capabilities are stored as space separated values, and this value contains spaces. If we wanted to add this there, we'd need to do some sort of escaping.

Better perhaps to add it to the browse object as a property on the target node that can be accessed via DTE (but which is not visible in the property grid, per the above screenshot).

I'll try and make the change for this as small as possible, but it'll still be a bigger diff than I'd like to put before the QB. Regardless, I'll dig into this more tomorrow.

Calling SetProperty with the TFM string is likely a much smaller change on our side. Let's see how I go tomorrow. I'll investigate both approaches.

[drewnoakes]

https://github.com/dotnet/project-system/pull/6893 implements the SetProperty approach. If that is suitable, we'll need to take it to the QB for 16.9. I'd expect we could revert it in 16.10 and go the DTE properties route for the longer term. @jasonmalinowski let me know if you'd like me to progress that. I may need your help with the Ask Mode details.

[drewnoakes]

Insertion PR containing #6932 for 16.9: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/303039

[drewnoakes]

This was fixed in https://github.com/dotnet/project-system/pull/6932