Fsharp: [FCS] `CheckResults.GetDeclarationLocation` returns nonsensical locations for files that have sourcelink enabled

[baronfel]

I've been working on enabling support in FSAC for using embedded or pdb-provided sourcelink information to power go-to-declaration/implementation without decompilation of dlls. As part of this, I use the CheckResults.GetDeclarationLocation API from FCS to get the location of the declaration of a particular ident island in some source code. There are a few oddities I see with this API especially with respect to sourcelink:

• There's not a distinct return case on the FSharpFindDeclResult to mark that you as the client should attempt a sourcelink lookup. This is because the API currently returns FSharpFindDeclResult.DeclFound with a range.
• The range returned as part of the FSharpFindDeclResult.DeclFound may not exist in the user's system, because the range reported in this case is from the full path of the host that _built_ the assembly. For example, taking the button function from Giraffe's ViewEngine module at version 4.0.1 as an example, the path reported for the file containing the member is C:\projects\giraffe\src\Giraffe\GiraffeViewEngine.fs (sorta, read on),
• The range reported is nonsensical in that it contains duplicated 'build root' portions. The _actual_ filename reported in the example above is C:\projects\giraffe\src\Giraffe/C:\projects\giraffe\src\Giraffe\GiraffeViewEngine.fs, which has a duplicated C:\projects\giraffe\src\Giraffe portion coupled with a unix-style / path separator.

Repro steps

If you checkout the PR above and run dotnet run from the test/FsAutoComplete.Tests.Lsp directory, the tests I have written for Giraffe will fail due to the above reasons.

Expected behavior

The API should provide a signal that Sourcelink has been provided or that the range is non-local in some way, or
The API should provide a range that is consistent with the path that sourcelink is configured to expect (i.e. no duplicate root portions or incorrect separators).

If we had option A, then we could detect the need to try sourcelink more easily, but if we had B we could workaround the existing API.

Actual behavior

The above ranges are returned and it becomes very hard to work around the invalid paths in a consistent way.

Known workarounds

🍺

Related information

Provide any related information (optional):

• Operating system: Mac OSX 10.14
• .NET Runtime kind: .Net Core 3.x
• Editing Tools: VS Code/Ionide/FSAC

[baronfel]

After some digging, I've found that the logic in fileNameOfItem, which is used to make the file portion of the reported range on the declaration list items, is buggy when dealing with paths that are compared across file systems (at least on my macos machine when dealing with Giraffe's paths, which are windows-style and anchored at C:).

Here's the implementation of fileNameOfItem currently:

    /// Work out the source file for an item and fix it up relative to the CCU if it is relative.
    let fileNameOfItem (g: TcGlobals) qualProjectDir (m: range) h =
        let file = m.FileName 
        if verbose then dprintf "file stored in metadata is '%s'\n" file
        if not (FileSystem.IsPathRootedShim file) then 
            match ccuOfItem g h with 
            | Some ccu -> 
                Path.Combine(ccu.SourceCodeDirectory, file)
            | None -> 
                match qualProjectDir with 
                | None     -> file
                | Some dir -> Path.Combine(dir, file)
         else file

In my circumstance:

• qualProjectDir is /Users/chethusk/oss/FsAutoComplete/test/FsAutoComplete.Tests.Lsp/TestCases/GoToTests/GoToTests.fsproj (the project being typechecked)
• m.FileName is C:\projects\giraffe\src\Giraffe\GiraffeViewEngine.fs, and
• ccu.SourceCodeDirectory is C:\projects\giraffe\src\Giraffe

So what happens when I debug is that FileSystem.IsPathRootedShim "C:\\projects\\giraffe\\src\\Giraffe\\GiraffeViewEngine.fs" returns false, so we then lookup the ccu, and Path.Combine("C:\projects\giraffe\src\Giraffe", "C:\projects\giraffe\src\Giraffe\GiraffeViewEngine.fs") which results in the mangled path I see. FileSystem.IsPathRootedShim as far as I know, delegates to the default implementation which is System.IO.Path.IsPathRooted which returns false on my MacOs Catalina machine.

I think we need to change/fix the implementation of rooted paths in the file system shim to be aware of path differences on all platforms.

[baronfel]

The implementation of System.IO.Path.IsPathRooted varies across Windows and Unix, and so cannot be used in a consistent way for FCS in my opinion:

windows: https://github.com/dotnet/runtime/blob/master/src/libraries/System.Private.CoreLib/src/System/IO/Path.Windows.cs#L195-L200
unix: https://github.com/dotnet/runtime/blob/master/src/libraries/System.Private.CoreLib/src/System/IO/Path.Unix.cs#L114-L117

[baronfel]

Along these lines we should consider usage of Path.Combine as highly suspect, it too does some normalization of paths with OS-specific assumptions in mind.

[NinoFloris]

Oh boy, Path and Uri is such a mess in .NET.
I've opened quite a few bugs on these (including via the microsoft security center)

Yeah this is going to be painful work -- pushing the ecosystem to publish only deterministic /_/ base dirs in pdbs will also help a lot.

[baronfel]

Similarly, GetFullPath has platform-specific behavior that makes this harder to fix. I made an attempt to fix IsPathRooted and now initial range construction (which apparently normalizes paths) is doing a delightful thing where it's prefixing all windows-style paths with my local running directory :)

[baronfel]

Because these issues are present(, and overridable!) in the DefaultFileSystem implementation, we are able to work around them in FSAC (eg here: https://github.com/baronfel/FsAutoComplete/blob/sourcelink-check/src/FsAutoComplete.Core/CompilerServiceInterface.fs#L38-L45) in order to verify the assertions. And true enough, when I implemented custom, cross-platform path aware members for these two calls I got back ranges that I expected, and I was able to light up sourcelink-powered source downloads.

[baronfel]

Well, sort of. I keep finding that ranges read from external assemblies (eg Giraffe.dll from the nuget package cache) get my working directory prepended to them. @dsyme sorry to ping you directly, but do you have any info/pointers on where I can look to see how/where ranges get their filenames set? I thought I found it in range.fs, in normalizeFilePath: string -> string, so I've overridden IFileSystem.IsPathRootedShim and IFileSystem.GetFullPathShim with custom implementations. However, logging shows that every call to IFileSystem.IsPathRootedShim (to check if the paths need to be expanded) already contains my PWD prepended. Here's a brief log slice to iillustrate, note that these are the first times I'm loading the desired Giraffe assembly:

[16:48:18.051 DBG] [FileSystem] /Users/chethusk/.nuget/packages/giraffe/4.1.0/lib/netcoreapp3.0/Giraffe.dll expanded to /Users/chethusk/.nuget/packages/giraffe/4.1.0/lib/netcoreapp3.0/Giraffe.dll
[16:48:18.057 DBG] [FileSystem] Is /Users/chethusk/oss/scratch/scratch/C:\projects\giraffe\src\Giraffe\Middleware.fs rooted? True
[16:48:18.057 DBG] [FileSystem] /Users/chethusk/oss/scratch/scratch/C:\projects\giraffe\src\Giraffe\Middleware.fs expanded to /Users/chethusk/oss/scratch/scratch/C:/projects/giraffe/src/Giraffe/Middleware.fs
[16:48:18.058 DBG] [FileSystem] Is /Users/chethusk/oss/scratch/scratch/C:\projects\giraffe\src\Giraffe\HttpStatusCodeHandlers.fs rooted? True
[16:48:18.058 DBG] [FileSystem] /Users/chethusk/oss/scratch/scratch/C:\projects\giraffe\src\Giraffe\HttpStatusCodeHandlers.fs expanded to /Users/chethusk/oss/scratch/scratch/C:/projects/giraffe/src/Giraffe/HttpStatusCodeHandlers.fs
[16:48:18.058 DBG] [FileSystem] Is /Users/chethusk/oss/scratch/scratch/C:\projects\giraffe\src\Giraffe\Negotiation.fs rooted? True
[16:48:18.058 DBG] [FileSystem] /Users/chethusk/oss/scratch/scratch/C:\projects\giraffe\src\Giraffe\Negotiation.fs expanded to /Users/chethusk/oss/scratch/scratch/C:/projects/giraffe/src/Giraffe/Negotiation.fs
[16:48:18.058 DBG] [FileSystem] Is /Users/chethusk/oss/scratch/scratch/C:\projects\giraffe\src\Giraffe\Streaming.fs rooted? True
[16:48:18.058 DBG] [FileSystem] /Users/chethusk/oss/scratch/scratch/C:\projects\giraffe\src\Giraffe\Streaming.fs expanded to /Users/chethusk/oss/scratch/scratch/C:/projects/giraffe/src/Giraffe/Streaming.fs

[baronfel]

The above was due to a bug in FSAC. The current, fixed, and working implementation is available at https://github.com/fsharp/FsAutoComplete/blob/master/src/FsAutoComplete.Core/FileSystem.fs#L25, and I think we should seriously consider upstreaming it, or something very much like it, to fix the underlying path-dependency issues in the DefaultFileSystem.