dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.96k stars 4.02k forks source link

Flow capture reference with id that was never captured #60757

Open sbomer opened 2 years ago

sbomer commented 2 years ago

Version Used: 4.3.0-1.22213.10

This didn't repro in 4.0.1 - leave out the /p:MicrosoftCodeAnalysisVersion=4.3.0-1.22213.10 in the repro script below to see the testcase pass on earlier version 4.0.1 of Microsoft.CodeAnalysis.

Steps to Reproduce:

  1. git clone https://github.com/sbomer/linker -b captureRepro --recursive
  2. cd linker
  3. .\repro.ps1

This script just runs a testcase that compiles some code using the specified version of roslyn and runs it through an analyzer:

.\eng\dotnet.ps1 test test\ILLink.RoslynAnalyzer.Tests\ILLink.RoslynAnalyzer.Tests.csproj `
    --filter Repro `
    /p:MicrosoftCodeAnalysisVersion=4.3.0-1.22213.10

Test code which triggers analyzer assert:

        static void Test(object? arg)
        {
            Debug.Assert (true, $"Format string {arg}");
        }

The following assert in the analyzer is hit:

        public static void ValidateFlowCaptures (ControlFlowGraph cfg) {
            var captureIds = cfg.DescendantOperations<IFlowCaptureOperation> (OperationKind.FlowCapture).Select(capture => capture.Id).ToImmutableHashSet ();
            var referencedCaptureIds = cfg.DescendantOperations<IFlowCaptureReferenceOperation> (OperationKind.FlowCaptureReference).Select (captureRef => captureRef.Id).ToImmutableHashSet ();
            Debug.Assert (referencedCaptureIds.IsSubsetOf(captureIds));
        }

Expected Behavior: I would have expected that the Id of an IFlowCaptureReferenceOperation in the CFG would have been captured by an IFlowCaptureOperation in the same CFG. I am hardly familiar with these APIs, so maybe there is some gap in understanding on my part - but this seemed to be the case in older versions from my limited experience.

Actual Behavior: There's a flow capture reference to something that was never captured. The CFG has changed between versions of Roslyn - the assert doesn't hit with 4.0.1.

cc @agocke

jcouv commented 2 years ago

The Debug.Assert is using an interpolation handler: image

Assigning to @333fred for triage and investigation. Sounds like a potential CFG issue with interpolation handlers.

As a side note, it may be useful to have an easy-to-copy-and-paste dumper for CFG to assist in such troubleshooting.

333fred commented 2 years ago

That assert is incorrect. Note that IFlowCaptureReference has a IsInitialization field. If that is true, the assert will fail. Please adjust the assert and let us know if you have any other issues.

sbomer commented 2 years ago

Thanks a lot, that's what I was missing! There may still be an issue here:

The repro was simplified down from an assert I initially hit, which was asserting that a written FlowCaptureReference was an l-value flow capture. It seems like the logic in https://github.com/dotnet/roslyn/blob/main/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/FlowAnalysis/LValueFlowCaptureProvider.cs needs to be adjusted to deal with the CFG in this issue - currently it reports that the FlowCaptureReference which initializes the AssertInterpolatedStringHandle is an r-value flow capture. (Related: https://github.com/dotnet/roslyn/issues/31007)

I've pushed an updated repro to the same branch. Once again, this may just be me missing something - I am working on an analyzer that needs to deal with flow capture references, and copied the relevant logic from Roslyn, but I could be doing something wrong.

333fred commented 2 years ago

Very possibly. @mavasani, can you please take a look at that?