dotnet / project-system

The .NET Project System for Visual Studio
MIT License
969 stars 389 forks source link

Analyzer node files paths are invalid #2558

Open davkean opened 7 years ago

davkean commented 7 years ago

Each tree node representing a analyzer in the dependency tree is returning the following as its moniker/canonical name/FilePath:

[project-directory]\[target-framework]\AnalyzerDependency\[full-path-of-analyzer]

For example:

c:\users\davkean\Source\Repos\ConsoleApp61\ConsoleApp61\netcoreapp1.1\AnalyzerDependency\C:\Users\davkean\.nuget\packages\microsoft.codeanalysis.analyzers\1.1.0\analyzers\dotnet\cs\Microsoft.CodeAnalysis.Analyzers.dll

I believe we expect the moniker to be the analyzer file path, so this is likely breaking something @tmeschter what would be broken by this?

tmeschter commented 7 years ago

@davkean Nothing. The code to hook up diagnostics to the proper analyzer node in Solution Explorer expects this format for the canonical name it gets from the IVsHierarchy, and parsers out the full-path-of-analyzer part.

I would be much happier if there were a property on IVsHierarchy I could query to just get the full path on disk, for those IVsHierarchy items that have a representation on disk. Unfortunately there doesn't seem to be anything designed to serve that purpose. In the native C#/VB project system the canonical name was generally the full path.

Perhaps the right answer here is to define a new IVsHierarchy property that does exactly what we want.

davkean commented 7 years ago

@tmeschter What format is returned via legacy project? I'm okay with changing this syntax to be the actual path - would that work?

ProjectItem.FileNames is the way to get the file names from the project system.

tmeschter commented 7 years ago

@davkean In the legacy project system the canonical name is just the full path (except for projects, where it is the full path to the project's directory for no good reason). Would that work in multi-targeting scenarios? Possibly different targets could end up pulling in the same file.

Is it possible to go from an IVsHierarchy and item ID to the ProjectItem instance?

davkean commented 7 years ago

Yes, via __VSHPROPID.VSHPROPID_ExtObject.

tmeschter commented 7 years ago

@davkean ProjectItem.FileNames[...] always seems to return null for an analyzer, even though ProjectItem.FileCount returns "1" as I would expect. I'll try to debug through and see where things are going wrong.

davkean commented 7 years ago

@tmeschter Hmm, not sure about FileCount, maybe because this thing isn't marked with the file tree flags?

tmeschter commented 7 years ago

@davkean Yeah, that's what it turns out to be: we're not marking anything under Dependencies with ProjectTreeFlags.Common.FileOnDisk.

tmeschter commented 7 years ago

@davkean So tagging the node with ProjectTreeFlags.Common.FileOnDisk doesn't actually accomplish what we want. It allows us to query ProjectItem.FileNames, but what you get back is the same as the canonical name, in the [project-directory]\[target-framework]\AnalyzerDependency\[full-path-of-analyzer] format.

Based on some of the other exceptions I'm seeing, there are other problems as well. I haven't dug in too deeply, but it seems that once other components see the analyzer node as being a FileOnDisk they expect to be able to get a rational file path out of it, and choke on what we return instead.

I see two options:

  1. Leave this all as-is, with the Roslyn language service parsing the real file path out of the analyzer's canonical name, and accept that ProjectItems for the analyzer nodes (really, everything under the Dependencies node) won't be able to provide file paths.
  2. Update the Dependencies node code to only use real file paths. I don't fully grasp the downsides of this; I know that the current scheme allows us to differentiate nodes under different target frameworks but with the same path on disk, and I'm not sure what we would lose.
tmeschter commented 7 years ago

This isn't breaking anything at the moment, so I'm moving it out to 15.6.