Avoid cancellableTask CE in DocumentCache to preserve static-state-machine optimization

[xperiandri]

Summary

Fixes#20268 by avoiding the cancellableTask computation expression in DocumentCache, where it cannot compile to a static state machine in the hot path. The cache helpers now use direct CancellationToken-aware task wrappers instead of a closure-based cancellableTask wrapper.

Changes

• Switched TryGetValueAsync and SetAsync in vsintegration/src/FSharp.Editor/Common/DocumentCache.fs to explicit fun ct -> task { ... } functions.
• Kept the public CancellableTask<'T> API shape while removing the non-static resumable wrapper from the performance-sensitive code path.
• Added the VS release note entry in docs/release-notes/.VisualStudio/18.vNext.md for issue DocumentCache.fs cannot use cancellableTask CE because it does not compile to a static state machine #20268.

[github-actions]

❗ Release notes required

You can open this PR in browser to add release notes: open in github.dev

---

✅ Found changes and release notes in following paths:

Warning

No PR link found in some release notes, please consider adding it.

Change path	Release notes path	Description
`vsintegration/src`	docs/release-notes/.VisualStudio/18.vNext.md	No current pull request URL (#20273) found, please consider adding it

[T-Gro]

Thanks for this — the direction is reasonable and the diff is small and readable. A few notes, mostly about the rationale rather than the mechanics.

1. The justification is imprecise — the CE does statically compile

cancellableTask.Run is built on __stateMachine / __useResumableCode, so the resumable body is compiled to a static state machine today. What cancellableTask { … } additionally does is:

• allocate the AfterCode closure (fun ct -> …) that captures the struct state machine, and
• because cancellableTask = CancellableTaskBuilder(true) (the background builder), inject a Task.Run offload when not already on the thread pool.

Your replacement fun ct -> tryGetCachedValueAsync (doc, cache, ct)still allocates a closure (it captures doc and cache), and the inner task { … } still boxes its state machine on the first suspension. So the path is not "allocation-free" — it's "one smaller closure, no background Task.Run, no chance of a dynamic fallback."

Suggest tightening the release note wording accordingly, e.g. "avoids the background Task.Run offload and a larger wrapper closure from the cancellableTask builder" rather than "allocation-free." The precise wording matters here because the note is the durable explanation for why this pattern exists.

2. Behavioral change: background offload is dropped (Low — looks intentional/safe)

cancellableTask is the background builder, so the old helpers were offloaded via Task.Run when invoked from a sync context; the new task { … } runs inline on the caller until the first real await. Both current callers (ClassificationService.AddSemanticClassificationsAsync and HintService.getHintsForDocument) already wrap the work in a background cancellableTask, so the inner offload was redundant and removing it is fine — arguably a small win. Worth a one-line note that this is intentional.

3. Behavioral change: eager cancellation check is lost (Low)

The CE Run entry checks ct.IsCancellationRequested and returns Task.FromCanceled<_>(ct) before running any code. The manual helpers skip that and call doc.GetTextVersionAsync ct directly. Roslyn's GetTextVersionAsync honors the token, so this is practically equivalent, but you no longer guarantee a canceled Task on an already-canceled token. If you want to preserve exact semantics, add a guard:

static lettryGetCachedValueAsync(doc:Document,cache:MemoryCache,ct:CancellationToken)=if ct.IsCancellationRequested then Task.FromCanceled<_ voption>(ct)elsetask{ … }

Otherwise fine to leave as-is.

4. Nits / positives

• Making the helpers static let so they don't capture this is a nice touch. 👍
• An alternative that keeps CE ergonomics while dropping the background offload is foregroundCancellableTask { … } — no need to change, just noting the CE didn't have to be abandoned to get the perf shape you want.
• Please confirm CI is green: the secondary new(name, slidingExpirationSeconds) constructor now sits after the static let bindings. That ordering is valid F#, but worth a glance.

Net: I'd approve after the release-note wording is adjusted (item 1) — the code change itself is sound.

[T-Gro]

🤖🕵️ LGTM ✅

The two conditions from the earlier review are met on the current head: the release-note wording was softened, and commit 84a158c restored the eager Task.FromCanceled cancellation guard. The refactor is behavior-preserving (both call sites already run under a background cancellableTask, so no UI-thread resume is introduced) and the manual fun ct -> task { ... } helpers honor the ambient token. Please verify independently.