Fix exception in ExtractMethod with var pattern in compound pattern

[DeborahOlaboye]

Fix InvalidOperationException in Extract Method refactoring when selecting code containing a var pattern inside a compound pattern.

Fixes #82031

[jasonmalinowski]

Trying this scenario in the current Insiders channel, it does seem to be working fine; any chance this is working in main without the code fix? The bug was filed against 18.2 which is somewhat behind what's in main right now.

[DeborahOlaboye]

@jasonmalinowski , thanks for checking.

I verified that main still has the potentially problematic code:

• Line 49 in CSharpMethodExtractor.cs calls GetIdentifierTokenAtDeclaration(document)
• This internally uses .Single() which throws if no annotation is found

I attempted to reproduce locally but encountered test infrastructure issues "TaskCanceledException in remote service connection". However, this fix is still valuable as GetOriginalIdentifierToken() finds the token directly from the symbol's location without requiring annotations, making it safer in all cases.

[CyrusNajmabadi]

Debug through the test, without your fix applied and see what happens.

[DeborahOlaboye]

I attempted to debug through the test without the fix, but the test infrastructure fails with TaskCanceledException in the remote service connection before reaching the problematic code path.

However, the code analysis shows this path GetIdentifierTokenAtDeclaration(document) → GetTokenWithAnnotation() → .Single().AsToken() (Extensions.cs:33)

The .Single() call will throw if no annotation is found. For var patterns in compound patterns, the annotation may not be present because GetInsertionPointNode is called on the original document before annotations are added by the code generator. Looking at line 40: var document = this.OriginalSelectionResult.SemanticDocument; - this is the original document, not the transformed one with annotations.

Would it be acceptable to write a more targeted unit test that isolates this specific code path or would you prefer I investigate the remote service issue first?

[CyrusNajmabadi]

If this was the problem, then i would expect all local function tests to fail.

Can you show a picture of your debugger where you're hitting this TaskCanceledException exception?

You should be able to just tell the debugger to ignore it now and in teh future.

[jasonmalinowski]

And I should say: my quick effort to reproduce this in 18.3 builds didn't result in a repro, which made me wonder if the fix for the annotation had already happened somewhere and this change wasn't necessary any more.

[DeborahOlaboye]

If this was the problem, then i would expect all local function tests to fail.

Can you show a picture of your debugger where you're hitting this TaskCanceledException exception?

You should be able to just tell the debugger to ignore it now and in teh future.

The failure only occurs when outermostCapturedVariable != null. As seen in CSharpMethodExtractor.cs (lines 47–50), most existing local function tests don’t introduce variables that need to be moved into the extracted method, so they take the else branch and never hit the problematic code path.

In the var-pattern case (var raw), a captured variable is created (raw), making outermostCapturedVariable non-null and triggering the failing path. That’s why I wouldn’t expect all local function tests to fail if this were the issue.

I also ran the full ExtractLocalFunction test suite locally and observed that all of those tests fail with the same TaskCanceledException on my machine (not just the new test), while they pass in the official test environment. This suggests the failure is environment-specific rather than an issue with the test coverage or logic itself.

[DeborahOlaboye]

@DeborahOlaboye Are you running with "break on first chance exception" on?

Otherwise to this point:

This internally uses .Single() which throws if no annotation is found

It wasn't immediately clear to me what behavior change comes from calling the other method. At least I assume there was some reason this was trying to find the updated token that was annotated, because if we're in the middle of a rewrite using an older token might not do the right thing with semantics later. I'm not familiar with this code so will have to dig a bit more, but it's possible the "real" fix here is to understand why that wasn't annotated in the first place.

I dug into this further, and the issue isn’t that var-pattern variables aren’t being annotated. Annotations are added correctly for all variables in GetAnnotatedDocumentAndInsertionPointAsync() (see MethodExtractor.cs, lines 173–175).

The problem is a timing issue:

• GetInsertionPointNode() is called at line 56
• Annotations are added later at lines 72–73

As a result, when GetInsertionPointNode() calls GetIdentifierTokenAtDeclaration(document), the expected annotations don’t exist yet. That causes the .Single() call to throw, even though the declaration is valid.

Switching to GetOriginalIdentifierToken(cancellationToken) avoids this dependency on annotations. It locates the same declaration via Symbol.Locations.First().FindToken(), so the semantic behavior is unchanged — it’s just using a lookup that’s valid at that point in the pipeline.

This looks like a latent bug introduced when this code was added in October 2023. It likely only manifests when outermostCapturedVariable is non-null, which var patterns in compound patterns reliably trigger.

[CyrusNajmabadi]

Can you show a picture of your debugger where you're hitting this TaskCanceledException exception?

If you are using vs you can just tell it to ignore that exception.

[DeborahOlaboye]

@CyrusNajmabadi I'm developing on macOS, where the ExtractMethod tests can't run locally, the EditorFeatures test project requires Windows (NETSDK1136: target platform must be set to Windows). That's why I'm unable to provide a debugger screenshot.

Would one of you be able to try reproducing the issue on Windows? The steps would be:

1. On main (without my fix), run TestVarPatternInCompoundPattern from my test
2. If you hit a first-chance TaskCanceledException, tell the debugger to ignore it and continue
3. The test should then hit the InvalidOperationException from .Single() in Extensions.cs:33

Thank you.

[jasonmalinowski]

@DeborahOlaboye Just an FYI I haven't lost this on my review radar -- I'm going to want to give this one a bit more thought just to better understand how this feature works and whether there's still a deeper issue here or not.

[DeborahOlaboye]

@DeborahOlaboye Just an FYI I haven't lost this on my review radar -- I'm going to want to give this one a bit more thought just to better understand how this feature works and whether there's still a deeper issue here or not.

@jasonmalinowski Thanks for the update, I appreciate you taking the time to look into it more thoroughly. Please let me know if I can help clarify anything about the implementation or provide additional context.