Add HashDirectiveInfo module to FCS. #4122

smoothdeveloper · 2017-12-16T00:20:55Z

Add HashDirectiveInfo module to FCS, this is used to implement "go to definition" over #load directives in script.

This is the editor agnostic code I originally made in VFPT, I'm not sure if I'll have time to integrate it in VS, but it will be useful for other editors.

Comes with a simple unit test.

… definition" over #load directives in script. This is the editor agnostic code I originally made in VFPT, I'm not sure if I'll have time to integrate it in VS, but it will be useful for other editors. Comes with a simple unit test.

cartermp · 2017-12-16T22:44:06Z

src/fsharp/service/HashDirectiveInfo.fsi

+
+namespace Microsoft.FSharp.Compiler.SourceCodeServices
+
+#if COMPILER_PUBLIC_API


Let's not worry about this given #4120 was merged

cartermp · 2017-12-18T21:28:47Z

I the intention here that I can have the following script:

#load "MyMath.fs"

open MyMath

square 12

And f12 into the "MyMath.fs" part of the script to take me to that file, once VS integration (and other integration) is added?

smoothdeveloper · 2017-12-18T22:53:53Z

@cartermp yes, the current code has two functions:

list all #I and #load directives, with logic to resolve the absolute path
check if such directives are at a specific position

I'm looking at the VS integration, and might have that in a separate PR eventually, but the logic could be useful already if say Ionide would like to implement the navigation feature.

cartermp · 2017-12-18T23:00:20Z

How do you feel about this versus the existing FileSystemCompletion logic that @vasily-kirichenko ported over from Roslyn?

I'm all for exposing this functionality in FCS so other editors can benefit, but we'll want to make sure we don't have two implementations here.

smoothdeveloper · 2017-12-18T23:09:37Z

I haven't looked into the implementation much but it is not based on the AST but rather it uses the roslyn toolset.

I think it makes sense to have the HashDirectiveModule (or equivalent) as part of FCS.

We can look if there is use to change FileSystemCompletion to use HashDirectiveModule but I think the implementation choice was made this way to keep it close to C# version as well as performance reasons.

cartermp · 2017-12-18T23:23:08Z

Right, the actual searching logic is done in Roslyn, but there are still some differences here (e.g., handling network paths). Using the AST vs. a regex (what we have currently) to snag directives seems like it should really just be a matter of efficiency. Whichever is faster should be what's taken and placed in FCS.

In terms of the actual searching logic, how does it stack up when compared with what Roslyn does?

I'd be in favor of exposing this in FCS and consuming it so long as it's similar in performance and handles the same number of cases. Otherwise, VS will be "special" when compared with other editors.

smoothdeveloper · 2017-12-19T00:09:41Z

I'm leaning toward this approach:

get the feature implemented in VS (and others), the same way it was working with VFPT
let the roslyn/C# guys catch up on the feature gap (they don't have the goto definition on the #load directives)
based on that, revise implementation to draw inspiration from what happened on C# side and eventually optimize FCS (albeit this code path won't be a critical one) and refactor the FileSystemCompletion, note that the code I'm providing is not tangled with editor logic, it would take some efforts to factor out something reusable from FileSystemCompletion in a logic only module

I don't feel great about using regexp where something is provided in a safer manner in the AST, unless we definitely hit a performance issue.

We can leave this PR open for more reviews until I eventually have the VS implementation working (so we can review the whole) or someone else is looking to use this in FCS.

If I have chance, I'll look more into FileSystemCompletion but I believe we should touch it only if the C# side is doing more work on it (or bugfixes).

smoothdeveloper · 2018-03-27T19:04:58Z

@cartermp / @KevinRansom I think this is ready, somehow forgot to ping about the requested change to remove COMPILER_PUBLIC_API which is now done.

Is there anyone which can point me how to open arbitrary file in Visual Studio editor, I'll want to integrate it in https://github.com/Microsoft/visualfsharp/blob/5a12cde14c18646380a05371297e38e74e96bc18/vsintegration/src/FSharp.Editor/Navigation/GoToDefinitionService.fs but I couldn't figure out appropriate way to open editor given arbitrary file path from there last time I had a look.

majocha · 2018-03-27T21:28:04Z

@smoothdeveloper VsShellUtilities.OpenDocument maybe this will do?

forki · 2018-04-17T14:27:44Z

ping

smoothdeveloper · 2018-04-17T14:37:14Z

@forki I had issues building the repository: #4317 (comment) I'll give it another try (thanks @majocha going to try this soon).

The FCS part is ready for merge though, I'll do the VS tooling part in a separate PR.

KevinRansom · 2018-09-26T18:13:57Z

@cartermp,

can you work with @smoothdeveloper to see if you still need changes, thanks.

Kevin

smoothdeveloper · 2018-10-03T10:08:34Z

@KevinRansom thanks for the nudge, there are two points I wanted to make:

other editors (ionide & rider AFAIR) seem to have implemented their own "go to definition" on #load directives, without use of this code, VS2017 is the only one not having it
I've learned about #load for multiples files in a single directive recently and this code (which comes from VFPT) doesn't work properly with it, I think this is minor issue for now

I'm currently not able to dig back into the VS integration bits I experimented, but it would be great if someone at MS could give a shot at wiring the logic in this PR to the goto definition service using Roslyn APIs.

I currently can't commit the time fiddling with it but I'm sure that for someone who is sufficently exposed to APIs, it won't be too much trouble, the code here handles you an absolute path to open an editor for and the goto definition service isn't too big.

KevinRansom · 2018-10-03T14:28:08Z

@smoothdeveloper thanks for the heads up, and for the PR. I will chat with a few folks to see how to take this forward.

Kevin

cartermp

Looking this over again and I think it makes sense. We should take this without the VS integration, which can be done separately. I don't think it should be too difficult to wire up though.

cartermp · 2018-10-16T17:58:14Z

src/fsharp/service/HashDirectiveInfo.fs

+        // (this behaviour is undocumented in F# but it seems to be how it works).
+
+        // list of #I directives so far (populated while encountering those in order)
+        // TODO: replace by List.fold if possible


Do you intend on replacing this with List.fold?

dsyme · 2018-10-18T11:57:02Z

fcs/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.Tests.fsproj

+    <Compile Include="$(FSharpSourcesRoot)\..\tests\service\HashDirectiveInfoTests.fs">
+      <Link>HashDirectiveInfoTests.fs</Link>
+    </Compile>
+    <None Include="$(FSharpSourcesRoot)\..\packages\FSharp.Compiler.Tools.4.1.27\tools\FSharp.Core.optdata">


These should not be needed

dsyme · 2018-10-18T11:57:55Z

src/fsharp/service/HashDirectiveInfo.fs

+        let pushInclude = includesSoFar.Add
+
+        // those might need to be abstracted away from real filesystem operations
+        let fileExists = File.Exists


Please remove all these abbreviations

You should also use the FileSystem abstraction for all of these

dsyme · 2018-10-18T12:04:35Z

src/fsharp/service/HashDirectiveInfo.fsi

+    /// a LoadDirective (#load) either points to an existing file, or an unresolveable location (along with all locations searched)
+    type LoadDirective =
+        | ExistingFile of string
+        | UnresolvableFile of string * previousIncludes : string array


Please use string[] rather than string array in the compiler

dsyme · 2018-10-18T12:08:29Z

This looks like a partial reimplementation of aspects of the resolution logic done by the main F# compiler code.

The normal approach to this would be to record the resolutions in from the type-checking/analysis phase and report those resolutions here, rather than reimplementing the resolution logic.

In this case it's not a big problem. But there's a bit of trend toward reimplementing core compiler logic under src/fsharp/service and of course in the long term that will be a maintenance problem.

smoothdeveloper · 2019-03-29T16:16:05Z

I'm resuming this in a new branch (https://github.com/smoothdeveloper/visualfsharp/tree/hashdirective-redux) and will open a separate PR.

Taking care of some reviewed aspects:

@dsyme

Ok I found the FileSystem shims and extending them, removing the custom path stuff; is it ok to use System.IO.Path.Combine in compiler or I should add FileSystem.CombineShim as well?

This looks like a partial reimplementation of aspects of the resolution logic done by the main F# compiler code.

The normal approach to this would be to record the resolutions in from the type-checking/analysis phase and report those resolutions here, rather than reimplementing the resolution logic.

Do you know the location of that resolution in the code? I'd be happy to take a further look at some point.

I'll add some more tests and figure out handling of multi file load directives, I'm not sure I can work yet on the VS bits in my environment.

dsyme · 2019-03-29T17:41:22Z

Ok I found the FileSystem shims and extending them, removing the custom path stuff; is it ok to use System.IO.Path.Combine in compiler or I should add FileSystem.CombineShim as well?

It's ok to use SYstem.IO.Path.Combine

cartermp suggested changes Dec 16, 2017

View reviewed changes

remove COMPILER_PUBLIC_API usage

6465ab0

Merge branch 'master' into fcs-source-code-services-HashDirectiveInfo

5ed63ef

smoothdeveloper mentioned this pull request Oct 5, 2018

[WIP] Using MiscellaneousFilesWorkspace to handle single files #5733

Closed

cartermp approved these changes Oct 16, 2018

View reviewed changes

dsyme reviewed Oct 18, 2018

View reviewed changes

KevinRansom closed this Mar 9, 2019

KevinRansom reopened this Mar 9, 2019

smoothdeveloper added a commit to smoothdeveloper/visualfsharp that referenced this pull request Mar 29, 2019

restart work on dotnet#4122

92617bd

smoothdeveloper closed this Mar 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add HashDirectiveInfo module to FCS. #4122

Add HashDirectiveInfo module to FCS. #4122

smoothdeveloper commented Dec 16, 2017

cartermp Dec 16, 2017

cartermp commented Dec 18, 2017

smoothdeveloper commented Dec 18, 2017

cartermp commented Dec 18, 2017

smoothdeveloper commented Dec 18, 2017

cartermp commented Dec 18, 2017 •

edited

Loading

smoothdeveloper commented Dec 19, 2017

smoothdeveloper commented Mar 27, 2018

majocha commented Mar 27, 2018

forki commented Apr 17, 2018

smoothdeveloper commented Apr 17, 2018

KevinRansom commented Sep 26, 2018

smoothdeveloper commented Oct 3, 2018 •

edited

Loading

KevinRansom commented Oct 3, 2018

cartermp left a comment

cartermp Oct 16, 2018

dsyme Oct 18, 2018

dsyme Oct 18, 2018

dsyme Oct 18, 2018

dsyme Oct 18, 2018

dsyme commented Oct 18, 2018

smoothdeveloper commented Mar 29, 2019

dsyme commented Mar 29, 2019


		namespace Microsoft.FSharp.Compiler.SourceCodeServices

		#if COMPILER_PUBLIC_API

Add HashDirectiveInfo module to FCS. #4122

Add HashDirectiveInfo module to FCS. #4122

Conversation

smoothdeveloper commented Dec 16, 2017

cartermp Dec 16, 2017

Choose a reason for hiding this comment

cartermp commented Dec 18, 2017

smoothdeveloper commented Dec 18, 2017

cartermp commented Dec 18, 2017

smoothdeveloper commented Dec 18, 2017

cartermp commented Dec 18, 2017 • edited Loading

smoothdeveloper commented Dec 19, 2017

smoothdeveloper commented Mar 27, 2018

majocha commented Mar 27, 2018

forki commented Apr 17, 2018

smoothdeveloper commented Apr 17, 2018

KevinRansom commented Sep 26, 2018

smoothdeveloper commented Oct 3, 2018 • edited Loading

KevinRansom commented Oct 3, 2018

cartermp left a comment

Choose a reason for hiding this comment

cartermp Oct 16, 2018

Choose a reason for hiding this comment

dsyme Oct 18, 2018

Choose a reason for hiding this comment

dsyme Oct 18, 2018

Choose a reason for hiding this comment

dsyme Oct 18, 2018

Choose a reason for hiding this comment

dsyme Oct 18, 2018

Choose a reason for hiding this comment

dsyme commented Oct 18, 2018

smoothdeveloper commented Mar 29, 2019

dsyme commented Mar 29, 2019

cartermp commented Dec 18, 2017 •

edited

Loading

smoothdeveloper commented Oct 3, 2018 •

edited

Loading