Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix reference count positions #1319

Merged

Conversation

TheAngryByrd
Copy link
Member

Fixes ionide/ionide-vscode-fsharp#2019

Before:

codelens-references-broken.mp4

After:

codelens-references-fixed.mp4

@@ -1437,12 +1452,17 @@ type AdaptiveState
asyncAVal {
let wins =
openFilesToChangesAndProjectOptions
|> AMap.map (fun _k v -> v |> AsyncAVal.mapSync (fun (_, projects) _ -> projects))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main issue here is the calls to parseFile weren't happening after file changes because of how F# Data Adaptive works when caching values. Even if something changes upstream (like file changes), the list of projects was staying the same and causing us to use stale parsechecks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sure to allow the file changes to pass thru so we can use them correctly for updates.

@TheAngryByrd TheAngryByrd force-pushed the 2019-reference-counts-position-incorrect branch from 86aec8a to d204b8e Compare September 2, 2024 18:33
@TheAngryByrd TheAngryByrd force-pushed the 2019-reference-counts-position-incorrect branch from 176e7cc to 4c5a7b6 Compare September 2, 2024 19:52
@Krzysztof-Cieslak
Copy link
Member

Great! Merge and release ASAP?

@baronfel
Copy link
Contributor

baronfel commented Sep 3, 2024

I'm being responsible parent here and asking for a test to prevent regressing this (because @TheAngryByrd thinks a simple refactoring regressed this behavior):

  • check file with function definition
  • assert lens is anchored to the definition's position
  • move definition
  • assert lens is still anchored to the definition's position

}
|> AsyncResult.foldResult id (fun e -> failtest $"{e}")


let projectBasedTests state =
testList "ProjectBased" [
serverTestList ("CodeLensPositionStaysAccurate") state defaultConfigDto (Some CodeLens.CodeLensPositionStaysAccurate.dir) (fun server -> [
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests added.

Fails with only BackgroundCompiler on older version of src/FsAutoComplete/LspServers/AdaptiveServerState.fs.

When we were testing on discord, we discovered it was only failing for background compiler and not transparent (this is because the project workspace handles this file update mechanism differently)

Comment on lines +31 to +45
let getLenses (doc: Document) =
let p: CodeLensParams =
{ TextDocument = doc.TextDocumentIdentifier
PartialResultToken = None
WorkDoneToken = None }

doc.Server.Server.TextDocumentCodeLens p
|> AsyncResult.mapError string
|> AsyncResult.map (fun r -> r |> Option.defaultValue [||])

let getResolvedLenses (doc: Document) lenses =
lenses
|> List.ofArray
|> List.traverseAsyncResultA doc.Server.Server.CodeLensResolve
|> AsyncResult.mapError string
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly refactoring and mechanical changes here

@TheAngryByrd TheAngryByrd force-pushed the 2019-reference-counts-position-incorrect branch from 34cc98a to 72eb03e Compare September 4, 2024 01:07
@baronfel
Copy link
Contributor

baronfel commented Sep 4, 2024

Awesome - merging. Want to try and take this one and 1309 in the next release.

@baronfel baronfel merged commit 3f6d1bd into ionide:main Sep 4, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reference counts appearing in really odd places
3 participants