ExtractMethodCodeRefactoringProvider crashed with InvalidOperationException

[davkean]

VS Build: 26426.1 VSUWPT

Note same code that crashed: #19135.

// Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.

using System;
using Microsoft.VisualStudio.ProjectSystem.VS;

namespace Microsoft.VisualStudio.Shell.Interop
{
    internal static class IVsOutputWindowExtensions
    {
        public static void ActivatePane(this IVsOutputWindow outputWindow, Guid pane)
        {
            Requires.NotNull(outputWindow, nameof(outputWindow));
            Requires.NotEmpty(pane, nameof(pane));

            HResult hr = outputWindow.GetPane(ref $pane$, out IVsOutputWindowPane pane);
            if (!hr.IsOK) // Pane not found
                return;

            hr = pane.Activate();
            if (hr.Failed)
                throw hr.Exception;
        }
    }
}

System.InvalidOperationException : Unexpected false
   at Roslyn.Utilities.Contract.ThrowIfFalse(Boolean condition,String message)
   at Microsoft.CodeAnalysis.ExtractMethod.ExtractMethodMatrix.GetVariableStyle(Boolean captured,Boolean dataFlowIn,Boolean dataFlowOut,Boolean alwaysAssigned,Boolean variableDeclared,Boolean readInside,Boolean writtenInside,Boolean readOutside,Boolean writtenOutside,Boolean unsafeAddressTaken)
   at Microsoft.CodeAnalysis.ExtractMethod.MethodExtractor.Analyzer.GetVariableStyle(Dictionary`2 symbolMap,ISymbol symbol,SemanticModel model,ITypeSymbol type,Boolean captured,Boolean dataFlowIn,Boolean dataFlowOut,Boolean alwaysAssigned,Boolean variableDeclared,Boolean readInside,Boolean writtenInside,Boolean readOutside,Boolean writtenOutside,Boolean unsafeAddressTaken)
   at Microsoft.CodeAnalysis.ExtractMethod.MethodExtractor.Analyzer.GenerateVariableInfoMap(SemanticModel model,DataFlowAnalysis dataFlowAnalysisData,Dictionary`2 symbolMap)
   at async Microsoft.CodeAnalysis.ExtractMethod.MethodExtractor.Analyzer.AnalyzeAsync(<Unknown Parameters>)
   at async Microsoft.CodeAnalysis.ExtractMethod.MethodExtractor.ExtractMethodAsync(<Unknown Parameters>)
   at async Microsoft.CodeAnalysis.ExtractMethod.AbstractExtractMethodService`3.ExtractMethodAsync[TValidator,TExtractor,TResult](<Unknown Parameters>)
   at async Microsoft.CodeAnalysis.CodeRefactorings.ExtractMethod.ExtractMethodCodeRefactoringProvider.GetCodeActionAsync(<Unknown Parameters>)
   at async Microsoft.CodeAnalysis.CodeRefactorings.ExtractMethod.ExtractMethodCodeRefactoringProvider.ComputeRefactoringsAsync(<Unknown Parameters>)
   at async Microsoft.CodeAnalysis.CodeRefactorings.CodeRefactoringService.GetRefactoringFromProviderAsync(<Unknown Parameters>)
   at Microsoft.VisualStudio.Telemetry.WindowsErrorReporting.WatsonReport.GetClrWatsonExceptionInfo(Exception exceptionObject)

[davkean]

Might have something to do with a field and a variable with the same name; "pane".

[Pilchie]

@davkean what did you select?

Looks like a dupe of #17165, but I don't see any local methods in that code...

[davkean]

I had the `pane' in 'ref pane' selected, I've marked it with '$' and '$'.

[sharwell]

So the issue here is the double-declaration of pane. In the semantic model created for the incorrect code, the out variable declaration out IVsOutputWindowPane pane is not treated as a new variable in the scope. Therefore, the statement pane.Activate() is bound to the parameter pane instead of the out variable.

When this code runs through the method extraction analyzer (modified to avoid the crash), the data flow analysis determines that the out variable is declared but never used (because references later bind to the parameter), and thus can be removed by the method extraction. If this were allowed to proceed, it would totally break the code, producing the following:

public static void ActivatePane(this IVsOutputWindow outputWindow, Guid pane)
{
    Requires.NotNull(outputWindow, nameof(outputWindow));
    Requires.NotEmpty(pane, nameof(pane));

    HResult hr = GetHr(outputWindow, ref pane);
    if (!hr.IsOK) // Pane not found
        return;

    hr = pane.Activate();
    if (hr.Failed)
        throw hr.Exception;
}

private static bool GetHr(IVsOutputWindow outputWindow, ref IVsOutputWindowPane pane)
{
    return outputWindow.GetPane(ref pane, out IVsOutputWindowPane pane);
}

[sharwell]

For now I think I'm just going to disable the refactoring for this case, and we can work on improving (expanding) its functionality for a future release.

[CyrusNajmabadi]

What's causign the InvalidOperationException?

[sharwell]

@CyrusNajmabadi Variable matrix doesn't know what to do with a variable which is both passed into the method and also defined by the method.