Solution.WithDocumentSyntaxRoot throws when analyzer config has non-rooted path

[tmat]

solution
  .AddAnalyzerConfigDocument(DocumentId.CreateNewId(projectId), "editorcfg", SourceText.From("config"))
  .WithDocumentSyntaxRoot(documentId, 
    SyntaxFactory.ParseSyntaxTree("class NewClass {}").GetRoot());

This is due to a check in AnalyzerConfig.Parse:

if (!Path.IsPathRooted(pathToFile) || string.IsNullOrEmpty(Path.GetFileName(pathToFile)))
{
    throw new ArgumentException("Must be an absolute path to an editorconfig file", nameof(pathToFile));
}

It doesn't seem like having a rooted path should be a requirement.

BTW, the message says "absolute path" yet the condition is checking a root path, which is not the same. Path.IsPathRooted("/a/b") is true on Windows, but it's not an absolute path.

See test SolutionTests.WithDocumentSyntaxRoot_AnalyzerConfigWithoutFilePath.

[tmat]

@jasonmalinowski @agocke

[agocke]

Requirement seems good, but yes it should check to see if the path is fully qualified, not if it's rooted.

[tmat]

@agocke Why is it needed though? We should be able to create a representation of a solution that only use paths relative to the repository root.

[agocke]

The section matching algorithm requires full paths to decide whether or not the matches are applicable.

[tmat]

@agocke Can you give an example? If that is so then it would depend on the directory on disk where a repository is located. It would make the build non-deterministic.

[agocke]

You create an AnalyzerConfig using path "Z:X\Y". You later need to calculate the configuration settings for the source file "Z:\X\Y\myfile.cs". We know the file applies at all because "Z:X\Y" is an exact substring of "Z:\X\Y\myfile.cs" and the string used for section name matching is the remainder after subtracting the substring, namely "myfile.cs".

This doesn't affect determinism as you are expected to move all analyzer config files to the same relative path if the source files are moved.

[tmat]

I don't see why we require a full path though. We don't require full path for SyntaxTree.FilePath. Can't we apply the same prefix matching when relative paths are used?

For example, AnalyzerConfig path is a/b/.editorconfig and syntax tree has path a/b/myfile.cs.

[agocke]

I think it's a bad idea to complexify this and potentially introduce bugs where you're comparing relative paths to absolute paths. I think it's better and more resilient to force resolution of canonical path at the point of construction.

[tmat]

@agocke No, it's not better. It adds extra restrictions on solution representation that we haven't had before. In some scenarios having a full path doesn't make sense. E.g. source files might be stored in cloud storage and all paths might be URLs. e.g. source://domain/a/b/.editorconfig, source://domain/a/b/x.cs.

[agocke]

You didn't address the point that canonicalization of paths prevents bugs where we compare paths of different form. Your argument seems to be that you're willing to risk that for more features.

Your example is also broken. Relative paths may be possible, but the editorconfig spec explicitly specifies that it works against paths, not URIs. You will need to transform your URI into a path for editorconfig support.

[tmat]

What are the different forms you have on mind? Do we actually canonicalize the path? I don't see that in Parse implementation. E.g. path C:\temp\a\b\..\c\d is full but not normalized.

Relative paths may be possible, but the editorconfig spec explicitly specifies that it works against paths, not URIs.

Fair enough. Why does it care about the prefix though? Aren't the globs in the editorconfig matched against the part of the file path relative to the path of the editorconfig file?

I think we could say that we accept an arbitrary string as a path and the only characters that are treated specially are \, which are normalized to /. It's up to the host to make sure the paths are normalized. So csc.exe/msbuild would do the real normalization (i.e. Path.GetFullPath) for both paths of .cs files and .editorconfig files.

This is generally how compiler has treated path strings elsewhere.

[cyungmann]

I am encountering a crash in VS 2019 preview and in the logs saw the exception mentioned by the OP, so wondering if the crash is related to this.

https://developercommunity.visualstudio.com/content/problem/1156133/vs-2019-preview-crash-when-previewing-configure-se.html