Enhancements for Find All References & Rename #1037
Conversation
| let symbolUseWorkspace | ||
| getDeclarationLocation | ||
| (findReferencesForSymbolInFile: (string * FSharpProjectOptions * FSharpSymbol) -> Async<Range seq>) | ||
| (getDeclarationLocation: FSharpSymbolUse * NamedText -> SymbolDeclarationLocation option) | ||
| (findReferencesForSymbolInFile: (string<LocalPath> * FSharpProjectOptions * FSharpSymbol) -> Async<Range seq>) | ||
| (tryGetFileSource: string<LocalPath> -> ResultOrString<NamedText>) | ||
| getProjectOptionsForFsproj | ||
| (tryGetProjectOptionsForFsproj: string<LocalPath> -> FSharpProjectOptions option) | ||
| (getAllProjectOptions: unit -> FSharpProjectOptions seq) | ||
| (includeDeclarations: bool) | ||
| (includeBackticks: bool) | ||
| (errorOnFailureToFixRange: bool) | ||
| pos | ||
| lineStr | ||
| (text: NamedText) | ||
| (tyRes: ParseAndCheckResults) | ||
| = | ||
| : Async<Result<(FSharpSymbol * IDictionary<string<LocalPath>, Range[]>), string>> = |
There was a problem hiding this comment.
Prev this function returned declarations & usages separately (one Dictionary for decls, one for usages).
I changed this to return all together in a single Dictionary: Everywhere the results of symbolUseWorkspace are used, these two are immediately thrown together again. This way symbolUseWorkspace as well as using its results is easier.
If you don't need decls there's a parameter for that.
And in case you want the results partitioned: Use Symbol.partitionIntoDeclarationsAndUsages
|
Why does this need module MyModule =
let (|Even|Odd|) v = if v % 2 = 0 then Even else Odd
let _ = (|Even|Odd|) 42
// backticks
let _ = (|``Even``|Odd|) 42
let _ = (|``Even``|``Odd``|) 42
let _ = (``|Even|Odd|``) 42
// spaces
let _ = (| Even | Odd |) 42
let _ = (| Even|Odd |) 42
let _ = (|Even | Odd|) 42
// linebreaks
let _ = (|Even|
Odd|) 42
let _ = (
|Even|Odd|) 42
let _ = (
|Even|
Odd|
) 42
let _ = MyModule.(|Even|Odd|) 42
// backticks
let _ = MyModule.(|``Even``|Odd|) 42
let _ = MyModule.(|``Even``|``Odd``|) 42
// Invalid:
// let _ = MyModule.(``|Even|Odd|``) 42
// spaces
let _ = MyModule.(| Even | Odd |) 42
let _ = MyModule.(| Even|Odd |) 42
let _ = MyModule.(|Even | Odd|) 42
// linebreaks
let _ = MyModule.(|Even|
Odd|) 42
let _ = MyModule.(
|Even|Odd|) 42
let _ = MyModule.(
|Even|
Odd|
) 42
let _ = MyModule.(
|Even|
Odd|
) 42
And why doesn't it need And on other tests it cannot find `Some`:
.... Edit:
The remaining errors seem to be the usual random "no parse/type check results"... |
|
This needs to be rebased after all the latest changes related to Adaptive work. |
Booksbaum
force-pushed
the
FindAllReferences
branch
from
3ae1516 to
2a94c22
Compare
done But I'm not 100% sure everything
(Wasn't introduced with this PR: even current |
|
CC: @TheAngryByrd any idea? |
|
Seems like an environment issue. |
I do have that particular issue fixed in #1053. Incorporating the fixes from that branch seem to fix this. Gif showing rename with the declaration file open, then close the file, reload VSCode to make sure it's cleared. Then rename still works.
|
|
This is super cool 😄
|
in my `dotnet --info`(inside FSAC) Here the complete Error with Stack Trace for Adaptive Server: (Debug Build) Observations:
Seems like AdaptiveServer loads other dotnet dependencies (vs. old/current Server)? |
Incorporated from [fix](https://github.com/fsharp/FsAutoComplete/pull/1053/files#diff-febb2875fef94d7a7c78c5c23943057bc820a1f6b672f7357aee2502b51806e0R1004-R1013) in ionide#1053 (see ionide#1037 (comment)) Co-authored-by: Jimmy Byrd <jimmybyrd@gmail.com>
|
When I compile FSAC with & to dotnet 7 Adaptive Server works. Required steps:
Seems to work perfectly 👍 |
|
If you check the CI pipelines we do shenanigans with the global.json to test specific TFMs, but if you have global.json set to allow 7.0 TFMs the test runners doesn't handle 6.0 testing in a way that's compatible with the way we look up MSBuild and the .net SDK. That's why we have the BuildNet7 shenanigans. I think for the .net 7 update we might end up dropping net6 support (or at least drop testing for it) to make the repo layout less confusing overall. |
Very strange, Both 6.0 or 7.0 seem to work. (my giant work project is |
the project target framework doesn't matter. FSAC target framework does. So Adaptive Server in FSAC net6.0 inside net6.0 project fails, but Adaptive Server in FSAC net7.0 inside net6.0 projects works. in {
"FSharp.enableAdaptiveLspServer": true,
// error
// "FSharp.fsac.netCoreDllPath": "[..]\\FsAutoComplete\\bin\\Debug\\net6.0\\fsautocomplete.dll",
// ok
"FSharp.fsac.netCoreDllPath": "[..]\\FsAutoComplete\\src\\FsAutoComplete\\bin\\Debug\\net7.0\\fsautocomplete.dll",
}( The Published Ionide-Extension seems to use Which would explain why Adaptive Server works with current Ionide. |
I can't reproduce that. "FSharp.fsac.netCoreDllPath": "[..]\\FsAutoComplete\\src\\FsAutoComplete\\bin\\Debug\\net6.0\\fsautocomplete.dll"
"FSharp.enableAdaptiveLspServer": trueis what I have in my work project's Basically gonna need someone else to give this a try. @baronfel :) |
|
While trying to use I'll try to track it down a bit better. |
|
I changed Going further: Tracked the deciding argument down: Diffing builds of
-> |
Adding this to |> Async.Catch
|> Async.map(Result.ofChoice >> Result.mapError string >> Result.bind id) |
Incorporated from [fix](https://github.com/fsharp/FsAutoComplete/pull/1053/files#diff-febb2875fef94d7a7c78c5c23943057bc820a1f6b672f7357aee2502b51806e0R1004-R1013) in ionide#1053 (see ionide#1037 (comment)) Co-authored-by: Jimmy Byrd <jimmybyrd@gmail.com>
Booksbaum
force-pushed
the
FindAllReferences
branch
from
fa2ed65 to
fb26aa2
Compare
src/FsAutoComplete.Core/Commands.fs
Outdated
| let iterProject (project: FSharpProjectOptions) = | ||
| asyncResult { | ||
| //Enhancement: do in parallel? | ||
| for file in project.SourceFiles do | ||
| let file = UMX.tag file | ||
| do! tryFindReferencesInFile (file, project) | ||
| } | ||
|
|
||
| let! _ = getSymbolUsesInProjects (symbol, projects, onFound) | ||
| let iterProjects (projects: FSharpProjectOptions seq) = | ||
| asyncResult { | ||
| for project in projects do | ||
| do! iterProject project | ||
| } |
There was a problem hiding this comment.
Been running this branch a few days on my work project. Yeah parallel helps a lot here with speed. I did something like:
let iterProjects (projects: FSharpProjectOptions seq) =
[
for project in projects do
for file in project.SourceFiles do
let file = UMX.tag file
tryFindReferencesInFile (file, project)
]
|> Async.Parallel
|> Async.Ignore<unit array>and also changed the Dictionary to a ConcurrentDictionary.
There was a problem hiding this comment.
How does it feel from a load perspective? Async.Parallel queues all the items on the threadpool right, so you by default do NUM_CPUS at once? Would it make sense to limit to e.g. NUM_CPUs/2 or something to make sure there's headroom for other operations? What's the delay like in your experience?
QUESTIONS!
There was a problem hiding this comment.
How does it feel from a load perspective? Async.Parallel queues all the items on the threadpool right, so you by default do NUM_CPUS at once?
Mostly the big wait time is the type checker so we're throttled by that until type checks complete for the solution. (mostly 1 core)
Otherwise the main part is looping through a bunch of files and adding them to a dictionary. This part parallelizes extremely well. Looping through a lot of files can be slow comparatively.
There was a problem hiding this comment.
I did something like:
let iterProjects (projects: FSharpProjectOptions seq) = [ for project in projects do for file in project.SourceFiles do let file = UMX.tag file tryFindReferencesInFile (file, project) ] |> Async.Parallel |> Async.Ignore<unit array>
One disadvantage of this: it tryFindReferencesInFile for all files in all projects -- always.
Currently it stops on first error. (usually just for Rename when it cannot correct the range of a symbol). Is there already a Async.Parallel that stops on first error?
Though when deciding between these two: Parallel is probably preferred: It can only fail for Rename, not Find References -- which is most likely more often used. And now Adaptive Server loads all files -> not being able to correct range shouldn't happen that often
There was a problem hiding this comment.
I think the errors I run into are because I'm in a mixed C#/F# project and it tries to do things for cs file files so maybe we need to have more guards around file types.
fixed with ionide#1043 (Upgrade to dotnet 7 & new FCS) (see ionide#1037 (comment) )
So I finally hit this, it seemed to be a |
fixed with ionide#1043 (Upgrade to dotnet 7 & new FCS) (see ionide#1037 (comment) )
Booksbaum
force-pushed
the
FindAllReferences
branch
from
c60a552 to
1c57b52
Compare
|
There was a change in Adaptive Server: How do I get all project options including Scripts? Extract distinct Project Options from (AHHH.... Github doesn't allow comments on unchanged code parts :/ ) |
|
-> same as `dotnet/fsharp` Note: `None` too if project for file not loaded!
`--debug` is already a Expecto flag (for example: prints message for test start & finished). But when `--debug` is specified it * gets reinterpreted by FSAC (-> FSAC debug logging) * gets removed by FSAC (-> not passed on to Expecto) Now: `--debug` gets ignored by FSAC and passed on to Expecto. If you want debug FSAC logging: use `--log=verbose` instead (Note: `verbose`: previously `--debug` was associated with `verbose` logging, NOT `debug` logging)
* Enhancement: Find references of external symbols * Enhancement: better Range fixup * Add `textDocument/prepareRename` * Enhancement: Don't rename when not all exact ranges * Add tests for FindReferences & Rename Note: rename over unloaded files doesn't work in Adaptive LSP Server: Range fixup requires source text, but unlike in normal LSP Server the Adaptive LSP server doesn't autoload all F# files Note: Rename for Active Pattern & Active Pattern Cases is disabled
includes now Prepare support
Co-authored-by: Jimmy Byrd <jimmybyrd@gmail.com>
fixed with ionide#1043 (Upgrade to dotnet 7 & new FCS) (see ionide#1037 (comment) )
Note: Some AdaptiveLspServer Reference tests still fail: `allProjectOptions` doesn't return Project Options for Script files any more (at least untitled Script files) -> `Commands.symbolUseWorkspace` doesn't return usages inside Script file
…rOnFailureToFixRange = false`
…e Server) Note: There's still one bug/change compared to FSharpLspServer: (-> `FSAC.lsp.Ionide WorkspaceLoader.AdaptiveLspServer.Find All References tests.solution.inside B/WorkingModule.fs.List.map` fails) * FSharpLspServer: Caches script files that were once opened -> can find references script files that were once open but aren't any more * AdaptiveLspServer: Only keeps open script files -> cannot find references in script files that were once open but aren't any more
Issue: Unlike `FsharpLspServer`, AdaptiveLspServer doesn't keep closed scripts in cache -> cannot find references inside closed script files see ionide#1037 (comment)
Booksbaum
force-pushed
the
FindAllReferences
branch
from
d154e2e to
de6beb9
Compare
|
Test is adjusted (also: finding references in parallel incorporated) |
|
Excellent work as always @Booksbaum - thank you for these great fixes! |
Find all References: (
textDocument/references)ListorList.map)(FCS returns range with Namespace, Module, ... -> we want just actual identifier)
stateyetincludeDeclarationand return declarations only when setRename: (
textDocument/rename)Add
textDocument/prepareRename:Rename(F2) was available everywhere -- but then just didn't rename anything at invalid locations (like space, keywords, strings, external symbols)-.--> empty default name) or names with spaces (like``foo bar baz``-> cursor onbar-> default name justbar)``my value``-> default name ismy value): Focus on actual name and not if there should be backticks or not (-> gets auto-added by rename operation if necessary)Enhancement/Fix: Rename doesn't remove existing backticks
``value``-> Rename to``my value``->````my value````-> invalidFix: Identifier doesn't get renamed when declaration with backticks, but usage without:
-> Rename to
foo(doesn't matter at declaration or usage):Fix: Renaming operator -> new name gets put into backticks -> invalid
Enhancement: very basic new name validation when operator
@)Issue: For Active Pattern & their cases: doesn't rename all or too much locations
-> Regression/Fix/Workaround/all 3?...: Disable rename for Active Patterns and Active Pattern Cases
|Even|Odd|) are only complete Active Pattern occurrences (|Even|Odd|), but not individual cases (Even,Odd) -> Rename would only rename complete Active Pattern locations and leave single cases aloneEven,Odd) sometimes include the other cases too (usage forEven-> findsOddusages too).That can be handled (it shouldn't be too difficult to exclude all locations not matching source case) -- but I haven't implemented that special case.
FindBackgroundReferencesInFileon Active Pattern Case finds all other Cases too dotnet/fsharp#14206)-> can be enabled again once fixed in FCS
Unchanged Issue with Adaptive Server: Rename fails when declaration or usage in not-loaded files
textDocument/didOpenyet -> file not insidestate-> no range adjustment of occurrences possible -> no exact rename possible because might be incorrect rangestextDocument/didOpen)didOpen-> source now insidestate)Don't know if that's better or worse...
But I took the lazy route and do neither :P