#1097 incoming call hierarchy #1164

TheAngryByrd · 2023-09-14T03:05:49Z

Definitely need a parsing/typed expert here.

Outgoing calls
- I was able to use references and top level navigation items, but that comes with missing fidelity into nested expressions.
Incoming calls
- Ranges aren't right but I wanted to get something up
- Needed the type tree here to get function/method calls but that causes keepAssemblyContents to need to be set to true in the FSharpChecker (losing enablePartialTypeChecking in the process since they're mutually exclusive).

WHAT

`🤖 Generated by Copilot at c68553f`

This pull request enhances the F# language server by adding the call hierarchy feature and by refactoring the file management logic. It modifies the AdaptiveFSharpLspServer type to keep track of both open and closed files and to use this information for various language features. It also updates the Common.fs file to enable the CallHierarchyProvider capability.

`🤖 Generated by Copilot at c68553f`

CallHierarchyProvider
Enhances language server
Autumn of refactor

📞♻️🔎

WHY

HOW

`🤖 Generated by Copilot at c68553f`

Refactor the AdaptiveFSharpLspServer type to store and update information about both open and closed F# files in the workspace, and to simplify the logic of getting the project options, parsed results, and declarations for any file path. (link, link, link, link, link, link, link)
Implement the CallHierarchyIncomingCalls and TextDocumentPrepareCallHierarchy methods in the AdaptiveFSharpLspServer type to provide the functionality of finding the callers and callees of a given symbol in the workspace, using the existing parsing, type checking, and symbol use logic. (link)
Modify the workspaceSymbol function in the AdaptiveFSharpLspServer type to use the new function getAllDeclarations instead of the old function getAllOpenDeclarations, to improve the accuracy of the workspace symbol search feature. (link)
Add the CallHierarchyProvider capability to the Helpers module in the Common.fs file, to enable the language server to advertise its support for the call hierarchy feature to the LSP client. (link)
Remove an empty line from the code in the AdaptiveFSharpLspServer type, likely as a formatting or style adjustment. (link)

…chyIncomingCalls

baronfel

I played around with this and it already feels really great to use, but I confess that I have a hard time reasoning about the 'directions' (incoming/outgoing) and matching them to the FCS calls going on here. I'm also having a hard time seeing where the limitations/incorrect ranges are.

The feature itself is very exciting - a was using it in my 'normal' dev workflow for about an hour, so I can see it quickly becoming a go-to to help investigate deep chains.

baronfel · 2023-10-08T21:34:54Z

src/FsAutoComplete/LspServers/AdaptiveFSharpLspServer.fs

+                || x.IsPropertyGetterMethod
+                || x.IsPropertySetterMethod
+              // TODO get bodyRange and check if it is in the range
+              range.Start.Line >= pos.Line && lookingFor


Are you trying to emulate visibility here? So checking for other items in this file that are after the trigger position that are one of these types? I'm having a hard time wrapping my head around the 'direction' of the incoming/outgoing part so I'm very slow here :)

As it is, it kinda feels like this filter could be pushed down into the traversal function you have, so that we don't create/allocate so much.

Anything in the Outgoing implementation was some fever dream coding at like 11pm so I don’t know.

TheAngryByrd · 2023-10-08T22:33:41Z

I confess that I have a hard time reasoning about the 'directions' (incoming/outgoing) and matching them to the FCS calls going on here.

Incoming is like walking the call tree back to its roots. (So a recursive fine all references essentially).

Outgoing is like walking the call tree to its leaves. (So a recursive find all functions called).

I might actually punt on the Outgoing for the moment because it isn’t really possible without the typed tree (as far as I can tell), As that disables partial type checking.

TheAngryByrd · 2023-10-09T02:41:20Z

IncomingCalls is pretty much completed. I could add more tests here.
I removed OutgoingCalls because of what I outlined previously. Unless there's something I've missed, I don't think it's an easy fix.
I was bad and did some extra work here (can move to own branch if necessary)
- I cleaned up AdaptiveExtensions a bit
- I added a Version to DiagnosticCollection. We occasionally get TextSync issues, while I think there's something else going on, I think stopping it at the DiagnosticCollection will stop it from being a problem.

baronfel · 2023-10-09T17:43:08Z

src/FsAutoComplete.Core/AdaptiveExtensions.fs

+        if ct.IsCancellationRequested then
+          Task.FromCanceled<_>(ct)
+        else
+          Task.FromResult(mapping a ct))


oooohhhh, this is subtle. good spot.

TheAngryByrd · 2023-10-09T17:45:26Z

src/FsAutoComplete/LspHelpers.fs

+// type LspPos = Ionide.LanguageServerProtocol.Types.Position
+// type LspRange = Ionide.LanguageServerProtocol.Types.Range
+
+module Lsp = Ionide.LanguageServerProtocol.Types
+
+module LspRange =
+  // static member Create (start: LspPos) (end_: LspPos) : LspRange =
+  //   { Start = start; End = end_ }
+  let Zero: Lsp.Range =
+    { Start = { Line = 0; Character = 0 }
+      End = { Line = 0; Character = 0 } }


I think this should be removed. I was trying to figure out which things were 0 and 1 based.

TheAngryByrd · 2023-10-09T17:47:45Z

src/FsAutoComplete/LspServers/CallHierarchy.fs

+      | FSharpImplementationFileDeclaration.InitAction(e) -> visitExpr f e
+
+
+    fileDecls |> Seq.iter (visitDeclaration memberCallHandler)


Could delete this also since I punted on Outgoing for now.

baronfel · 2023-10-09T17:47:47Z

src/FsAutoComplete.Core/Commands.fs


 [<RequireQualifiedAccess>]
 type NotificationEvent =
-  | ParseError of errors: FSharpDiagnostic[] * file: string<LocalPath>
+  | ParseError of errors: FSharpDiagnostic[] * file: string<LocalPath> * version: int


I really wonder if we need some kind of 'file@version' abstraction here. Not now, but just immediately comes to mind.

TheAngryByrd · 2023-10-09T17:48:31Z

test/FsAutoComplete.Tests.Lsp/paket.references

@@ -6,7 +6,7 @@ Expecto
 Expecto.Diff
 Microsoft.NET.Test.Sdk
 YoloDev.Expecto.TestSdk
-AltCover
+# AltCover


Was getting annoyed at test times taking a long time due to this coverage hooking. Need to revert.

TheAngryByrd · 2023-10-09T17:49:38Z

src/FsAutoComplete/LspServers/Common.fs

+              | Add(source, version, diags) ->
+                match Map.tryFind source state with
+                | Some(oldVersion, _) when oldVersion > version -> return! loop state
+                | _ ->
+                  let newState = state |> Map.add source (version, diags)
+                  do! send uri newState
+                  return! loop newState


Added to prevent out of date diagnostics possibly updating a file.

* WIP Incoming Hierarchy * Call Hierarchy Incoming calls * outgoing call poc * Using TryRangeOfNameOfNearestOuterBindingContainingPos for CallHierarchyIncomingCalls * Fixing incoming call hierarchy plus tests * Fixing incoming call typechecks * formatting * Fixing up Adaptive Cancellation * Adding Versioning to DiagnosticCollection * Fixing tests * cleanup * cleanup * revert removing altcover --------- Co-authored-by: Chet Husk <chusk3@gmail.com>

TheAngryByrd changed the title ~~1097 call hierarchy 1~~ #1097 call hierarchy Sep 14, 2023

TheAngryByrd added 4 commits October 2, 2023 21:57

WIP Incoming Hierarchy

4a027cf

Call Hierarchy Incoming calls

e2cd39c

outgoing call poc

d39f9e9

Using TryRangeOfNameOfNearestOuterBindingContainingPos for CallHierar…

7a04311

…chyIncomingCalls

TheAngryByrd force-pushed the 1097-call-hierarchy-1 branch from 1855199 to 7a04311 Compare October 3, 2023 04:45

Fixing incoming call hierarchy plus tests

43f3fce

TheAngryByrd force-pushed the 1097-call-hierarchy-1 branch from 8333dab to 43f3fce Compare October 4, 2023 03:44

TheAngryByrd and others added 2 commits October 4, 2023 00:28

Fixing incoming call typechecks

0d51159

formatting

20c5fcd

baronfel reviewed Oct 8, 2023

View reviewed changes

TheAngryByrd added 2 commits October 8, 2023 21:57

Fixing up Adaptive Cancellation

bd04f10

Adding Versioning to DiagnosticCollection

7d998c7

TheAngryByrd force-pushed the 1097-call-hierarchy-1 branch 2 times, most recently from 1ea071b to 5c3953e Compare October 9, 2023 02:19

Fixing tests

8f35d64

TheAngryByrd force-pushed the 1097-call-hierarchy-1 branch from 5c3953e to 8f35d64 Compare October 9, 2023 02:21

TheAngryByrd marked this pull request as ready for review October 9, 2023 02:41

baronfel changed the title ~~#1097 call hierarchy~~ #1097 incoming call hierarchy Oct 9, 2023

baronfel approved these changes Oct 9, 2023

View reviewed changes

TheAngryByrd commented Oct 9, 2023

View reviewed changes

baronfel reviewed Oct 9, 2023

View reviewed changes

TheAngryByrd commented Oct 9, 2023

View reviewed changes

TheAngryByrd added 3 commits October 9, 2023 18:23

cleanup

30f546f

cleanup

6d2b1e4

revert removing altcover

7c430d1

TheAngryByrd merged commit f328907 into ionide:main Oct 10, 2023
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#1097 incoming call hierarchy #1164

#1097 incoming call hierarchy #1164

TheAngryByrd commented Sep 14, 2023 •

edited

Loading

baronfel left a comment

baronfel Oct 8, 2023

TheAngryByrd Oct 8, 2023

TheAngryByrd commented Oct 8, 2023

TheAngryByrd commented Oct 9, 2023

baronfel Oct 9, 2023

TheAngryByrd Oct 9, 2023

TheAngryByrd Oct 9, 2023

baronfel Oct 9, 2023

TheAngryByrd Oct 9, 2023

TheAngryByrd Oct 9, 2023

		\| FSharpImplementationFileDeclaration.InitAction(e) -> visitExpr f e


		fileDecls \|> Seq.iter (visitDeclaration memberCallHandler)

#1097 incoming call hierarchy #1164

#1097 incoming call hierarchy #1164

Conversation

TheAngryByrd commented Sep 14, 2023 • edited Loading

WHAT

🤖 Generated by Copilot at c68553f

🤖 Generated by Copilot at c68553f

WHY

HOW

🤖 Generated by Copilot at c68553f

baronfel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheAngryByrd commented Oct 8, 2023

TheAngryByrd commented Oct 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheAngryByrd commented Sep 14, 2023 •

edited

Loading

`🤖 Generated by Copilot at c68553f`

`🤖 Generated by Copilot at c68553f`

`🤖 Generated by Copilot at c68553f`