Convert relative paths to absolute paths for analyzer references

[tjchester]

This PR fixes an issue where the AnalyzerResultExtensions.GetAnalyzerReferences method allows the return of relative paths instead of absolute paths as expected by Roslyn and causing an exception to be thrown.

Depending on the execution environment: from within Visual Studio, from the command line by running the executable, or via dotnet run some references may be passed in as relative paths. If these references are then passed to the Roslyn subsystem then you will see an exception similar to below (call stack has been abbreviated and code related to PR is highlighted):

Unhandled exception. System.ArgumentException: Absolute path expected. (Parameter 'fullPath')

***
at Roslyn.Utilities.CompilerPathUtilities.RequireAbsolutePath(String path, String argumentName)
at Microsoft.CodeAnalysis.Diagnostics.AnalyzerFileReference..ctor(String fullPath, IAnalyzerAssemblyLoader assemblyLoader)
at Buildalyzer.Workspaces.AnalyzerResultExtensions.<>c__DisplayClass9_0.<GetAnalyzerReferences>b__0(String x)
***

at System.Linq.Enumerable.WhereSelectArrayIterator`2.MoveNext()
at System.Collections.Generic.LargeArrayBuilder`1.AddRange(IEnumerable`1 items)
at System.Collections.Generic.EnumerableHelpers.ToArray[T](IEnumerable`1 source)
at System.Collections.Immutable.ImmutableArray.CreateRange[T](IEnumerable`1 items)
at Roslyn.Utilities.EnumerableExtensions.ToBoxedImmutableArray[T](IEnumerable`1 items)
at Microsoft.CodeAnalysis.ProjectInfo.Create(ProjectId id, VersionStamp version, String name, String assemblyName, String language, String filePath, String outputFilePath, CompilationOptions compilationOptions, ParseOptions parseOptions, IEnumerable`1 documents, IEnumerable`1 projectReferences, IEnumerable`1 metadataReferences, IEnumerable`1 analyzerReferences, IEnumerable`1 additionalDocuments, Boolean isSubmission, Type hostObjectType, String outputRefFilePath)
at Buildalyzer.Workspaces.AnalyzerResultExtensions.GetProjectInfo(IAnalyzerResult analyzerResult, Workspace workspace, ProjectId projectId)
at Buildalyzer.Workspaces.AnalyzerResultExtensions.AddToWorkspace(IAnalyzerResult analyzerResult, Workspace workspace, Boolean addProjectReferences)
at Buildalyzer.Workspaces.ProjectAnalyzerExtensions.AddToWorkspace(IProjectAnalyzer analyzer, Workspace workspace, Boolean addProjectReferences)
at Buildalyzer.Workspaces.ProjectAnalyzerExtensions.GetWorkspace(IProjectAnalyzer analyzer, Boolean addProjectReferences)
...

This PR fixes that issue by using the Path.GetFullPath .NET method to convert all paths in the extension method to absolute paths before returning them from the method.

[Corniel]

I would wonder, can you create a unit tests that fails without your changes, and succeeds with?

[tjchester]

To create unit tests is kind of chicken and egg problem because in order for me to do that, I would need to include a copy of the old method which does not have the file path conversion code in it which wouldn't make sense. I can provide a logical justification though which is from the Microsoft Roslyn code that is being called.

Here is a link to the Microsoft code for the AnalyzerFileReference that is being created in that method. A copy of the constructor follows for reference:

/// <summary>
/// Creates an AnalyzerFileReference with the given <paramref name="fullPath"/> and <paramref name="assemblyLoader"/>.
/// </summary>
/// <param name="fullPath">Full path of the analyzer assembly.</param>
/// <param name="assemblyLoader">Loader for obtaining the <see cref="Assembly"/> from the <paramref name="fullPath"/></param>
public AnalyzerFileReference(string fullPath, IAnalyzerAssemblyLoader assemblyLoader)
{
    CompilerPathUtilities.RequireAbsolutePath(fullPath, nameof(fullPath));

    FullPath = fullPath;
    _assemblyLoader = assemblyLoader ?? throw new ArgumentNullException(nameof(assemblyLoader));

    _diagnosticAnalyzers = new(this, typeof(DiagnosticAnalyzerAttribute), GetDiagnosticsAnalyzerSupportedLanguages, allowNetFramework: true);
    _generators = new(this, typeof(GeneratorAttribute), GetGeneratorSupportedLanguages, allowNetFramework: false, coerceFunction: CoerceGeneratorType);

    // Note this analyzer full path as a dependency location, so that the analyzer loader
    // can correctly load analyzer dependencies.
    assemblyLoader.AddDependencyLocation(fullPath);
}

You can see from both the XML doc comments and the first line of the constructor that a full path is expected so defensive programming would dictate that the caller's of this method should verify they are passing full paths. With my patch that is now what is occurring.

[Corniel]

To create unit tests is kind of chicken and egg problem because in order for me to do that, I would need to include a copy of the old method which does not have the file path conversion code in it which wouldn't make sense. I can provide a logical justification though which is from the Microsoft Roslyn code that is being called.

I think you misunderstood me. I asked for a test that would fail if you (temporary) disable your change, and would succeed again, after you re-enable it. Just to ensure that nobody in the future can break it again (unintentionally).

(Note that I'm not the owner of the lib, so it are just my 2cts anyway)

[tjchester]

Here is a minimum VS2022 C# project which demonstrates calling the AnalyzerFileReference constructor with relative and absolute paths. The relative path test will throw the exception I was seeing in the code and the absolute path test uses the method I usesd in the PR to get the full path.

AnalyzerFileReferencePathTest.zip

[daveaglick]

Thanks for looking into this (and sorry for the late reply)! I guess my question/concern here is what base path is being used to convert a relative path into an absolute path? In the new calls to Path.GetFullPath() we're only passing the relative path. I believe that means the relative path will be appended/joined with the "current directory". Is that stable enough, or should we be using the version of Path.GetFullPath() that takes an explicit base path and passing in the path of the project file being analyzed (or some other well-known explicit base path)?

[tjchester]

I am not sure of the correct answer to your inquiry but I can show the relative references that were received by the extension and what full path references it resolved them to. In this example the relative references were resolved to the bin\packages folder of my console project.

Here are the relative references as passed into the GetAnalyzerReferences method:

[
  "..\\..\\packages\\Microsoft.CodeAnalysis.NetAnalyzers.7.0.3\\analyzers\\dotnet\\cs\\Microsoft.CodeAnalysis.CSharp.NetAnalyzers.dll",
  "..\\..\\packages\\Microsoft.CodeAnalysis.NetAnalyzers.7.0.3\\analyzers\\dotnet\\cs\\Microsoft.CodeAnalysis.NetAnalyzers.dll",
  "..\\..\\packages\\StyleCop.Analyzers.1.1.118\\analyzers\\dotnet\\cs\\StyleCop.Analyzers.CodeFixes.dll",
  "..\\..\\packages\\StyleCop.Analyzers.1.1.118\\analyzers\\dotnet\\cs\\StyleCop.Analyzers.dll"
]

Here is how Path.GetFullPath() resolved them:

[
  "C:\\Code\\SOLUTION_FOLDER\\projects\\PROJECT_FOLDER\\bin\\packages\\Microsoft.CodeAnalysis.NetAnalyzers.7.0.3\\analyzers\\dotnet\\cs\\Microsoft.CodeAnalysis.CSharp.NetAnalyzers.dll",
  "C:\\Code\\SOLUTION_FOLDER\\projects\\PROJECT_FOLDER\\bin\\packages\\Microsoft.CodeAnalysis.NetAnalyzers.7.0.3\\analyzers\\dotnet\\cs\\Microsoft.CodeAnalysis.NetAnalyzers.dll",
  "C:\\Code\\SOLUTION_FOLDER\\projects\\PROJECT_FOLDER\\bin\\packages\\StyleCop.Analyzers.1.1.118\\analyzers\\dotnet\\cs\\StyleCop.Analyzers.CodeFixes.dll",
  "C:\\Code\\SOLUTION_FOLDER\\projects\\PROJECT_FOLDER\\bin\\packages\\StyleCop.Analyzers.1.1.118\\analyzers\\dotnet\\cs\\StyleCop.Analyzers.dll"
]

For background my project is .NET 6.0 console program that uses the NTypeWriter library to generate TypeScript references for our ASP.NET (.NET 4.8) web project.

[daveaglick]

I'm trying to move forward with some outstanding Buildalyzer stuff this week, so I finally gave this a closer look. I understand the problem, and we probably don't want relative paths here. That said, a relative path is always relative to something and I still don't love that this change is based on the current path, whatever that is. This is from the docs:

If path is a relative path, this overload returns a fully qualified path that can be based on the current drive and current directory. The current drive and current directory can change at any time as an application executes. As a result, the path returned by this overload cannot be determined in advance. To return a deterministic path, call the GetFullPath(String, String) overload. You can also call the IsPathFullyQualified method to determine whether a path is fully qualified or relative and therefore whether a call to GetFullPath is necessary.

The examples of what's it's returning and what you're expecting were very helpful. From that I can see that the relative paths are rooted at the project file, and I think that's a reasonable path to use as the base (I honestly don't know what else we possibly could use).

I'll merge this PR, but then I'll go ahead and patch the code to use the path of the project file as the base path to make the absolute path calculation deterministic.

[daveaglick]

Thanks for working on this!