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

dotnet build doesn't terminate in 5.0.200, same code compiles in previous version. #4920

Closed
emmisberger opened this issue Mar 3, 2021 · 24 comments
Assignees
Labels
Area-Microsoft.CodeAnalysis.NetAnalyzers Bug The product is not behaving according to its current intended design Urgency-Soon

Comments

@emmisberger
Copy link

Version Used: 5.0.200

Steps to Reproduce:

  1. Create an empty C# project (dotnet new console)
  2. Run dotnet add package Microsoft.Win32.Registry
  3. Replace Program.cs with the following code:
using System.Runtime.InteropServices;
using Microsoft.Win32;

if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && Registry.GetValue("", "", null) != null)
{
    foreach (var x in new string[0])
    {
        if ("".ToString() == "")
        {
            try
            {
            }
            catch
            {
            }
        }
    }
}

Expected Behavior:
dotnet build compiles the project

Actual Behavior:
dotnet build hangs indefinitely.

@jaredpar jaredpar transferred this issue from dotnet/roslyn Mar 3, 2021
@jaredpar
Copy link
Member

jaredpar commented Mar 3, 2021

This looks to be hanging in the platform compatibility analyzer. Confirmed that disabling analyzers causes the build to succeed.

It's possible that there is a bug in the underlying data flow APIs that is the root here. If so please kick back to compilers.

@mavasani, @jmarolf, @sharwell

@sharwell sharwell added the Bug The product is not behaving according to its current intended design label Mar 3, 2021
@sharwell sharwell self-assigned this Mar 3, 2021
@sharwell
Copy link
Member

sharwell commented Mar 3, 2021

What version of the compiler is included with 5.0.200? I'm not able to reproduce this in the Platform Compatibility Analyzer tests with either 3.8.0 or 3.9.0.

@sharwell
Copy link
Member

sharwell commented Mar 3, 2021

Here's the test I wrote:

        [Fact]
        [WorkItem(4920, "https://github.com/dotnet/roslyn-analyzers/issues/4920")]
        public async Task FailureToTerminate()
        {
            var source = @"
using System.Runtime.InteropServices;
using Microsoft.Win32;

if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && Registry.GetValue("""", """", null) != null)
{
    foreach (var x in new string[0])
    {
        if ("""".ToString() == """")
        {
            try
            {
            }
            catch
            {
            }
        }
    }
}";

            await new VerifyCS.Test
            {
                ReferenceAssemblies = ReferenceAssemblies.Net.Net50.AddPackages(
                    ImmutableArray.Create(new PackageIdentity("Microsoft.Win32.Registry", "5.0.0"))),
                LanguageVersion = CSharpLanguageVersion.CSharp9,
                TestState =
                {
                    OutputKind = OutputKind.ConsoleApplication,
                    Sources = { source },
                    AnalyzerConfigFiles = { ("/.editorconfig", s_msBuildPlatforms) },
                }
            }.RunAsync();
        }

@mavasani
Copy link
Contributor

mavasani commented Mar 3, 2021

Tagging @buyaa-n as well in case she is aware of this issue or a fix that was done.

@emmisberger
Copy link
Author

@sharwell dotnet csc.dll -v shows me the following:
Microsoft (R) Visual C# Compiler version 3.9.0-5.21120.8 (accdcb77)

@jaredpar
Copy link
Member

jaredpar commented Mar 3, 2021

Here is the project file that I'm using + the global.json

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net5.0</TargetFramework>
    <RunAnalyzers>true</RunAnalyzers>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.Win32.Registry" Version="5.0.0" />
  </ItemGroup>

</Project>

{
  "sdk": {
    "version": "5.0.200"
  }
}

Confirmed that this does use 5.0.200 on my machine

C:\Users\jaredpar\temp\console> dotnet --version
5.0.200

@buyaa-n
Copy link
Contributor

buyaa-n commented Mar 3, 2021

Hm, I was not aware of this issue, i can repro with a sample console app. There is something related to Registry.GetValue("""", """", null) != null condition, if I use any other condition it builds just fine

@sharwell

This comment has been minimized.

@sharwell
Copy link
Member

sharwell commented Mar 3, 2021

@jaredpar Can you add a copy of the .globalconfig produced during your test build above? It's generated in the obj/Debug directory.

@sharwell
Copy link
Member

sharwell commented Mar 3, 2021

Oh, I was finally able to reproduce this by changing the .globalconfig.

@buyaa-n
Copy link
Contributor

buyaa-n commented Mar 4, 2021

Thanks, @sharwell i was able to repro with a similar unit test, it is a very strange edge case, if remove the guard method it builds just fine

// Builds fine
if (Registry.GetValue("", "", null) != null)
{
    foreach (var x in new string[0])
    {
        if ("""".ToString() == """")
        {
            try
            {
            }
            catch
            {
            }
        }
    }
}

If remove any of the blocks inside it also builds (loop or try catch or if condition)

// Builds fine
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && Console.CapsLock)
{
    foreach (var x in new string[0])
    {
        if ("""".ToString() == """")
        {
        }
    }
}

When debug the flow analysis result is not returning, seems flow analysis bug

@sharwell
Copy link
Member

sharwell commented Mar 4, 2021

@buyaa-n Yeah that comment was outdated. Once I fixed the .globalconfig file I was able to reproduce this and submitted #4923 with the test.

Yes, I also commented today to @mavasani that we need analyzer callbacks for AnalyzerConfig documents in the compiler. Too many cases where lines get silently ignored due problems that could be easily detected and reported by analyzers.

@sharwell
Copy link
Member

sharwell commented Mar 4, 2021

The test is now merged. The issue is not fixed, but writing the test is tricky so this will hopefully same someone time when they go to investigate this.

@buyaa-n
Copy link
Contributor

buyaa-n commented Mar 4, 2021

@sharwell got it, thanks for the tests

@jaredpar
Copy link
Member

jaredpar commented Mar 4, 2021

Seen at least two instances of this bug pretty much immediately after release. This seems very likely to need to go through servicing.

@Youssef1313
Copy link
Member

Youssef1313 commented Mar 4, 2021

In DataFlowOperationVisitor put a breakpoint at VisitInvocation. It's called, and finishes executing, then called again, then called again, etc. (that may not be the root cause)

A little bit more down the stack, the following call is done repeatedly:

(TAnalysisData newSuccessorInput, bool isFeasibleBranch) = OperationVisitor.FlowBranch(block, successorWithBranch, AnalysisDomain.Clone(output));

@Youssef1313
Copy link
Member

And finally, here is the infinite loop:

while (worklist.Count > 0 || pendingBlocksNeedingAtLeastOnePass.Count > 0)

@nickwhaley
Copy link

Worth looking at duplicate dotnet/roslyn#51652 . It fails in older versions also, but with a warning instead of a lockup. In case that helps diagnosis:

Warning AD0001 Analyzer 'Microsoft.NetCore.Analyzers.InteropServices.PlatformCompatibilityAnalyzer' threw an exception of type 'System.ArgumentException' with message 'An element with the same key but a different value already exists. Key: 'Microsoft.CodeAnalysis.Operations.LiteralOperation''.

@mavasani
Copy link
Contributor

mavasani commented Mar 5, 2021

I have a fix for this issue: #4928

@jaredpar @terrajobst @jeffhandley @vatsalyaagrawal would this be a candidate for .NET5 servicing fix?

@jaredpar
Copy link
Member

jaredpar commented Mar 5, 2021

There were two reports within the first few days of the SDK being released which speaks a bit about impact to me.

How corner case is the scenario though? It's hard to tell from the bug. If it's decently mainstream I'd vote for pushing through servicing at this point.

@mavasani
Copy link
Contributor

mavasani commented Mar 5, 2021

Apparently my fix does not fix the second hang: dotnet/roslyn#51652. I'll continue investigating.

How corner case is the scenario though

Both cases are when there are platform guard checks inside/outside a loop with a try/catch inside a loop.

@mavasani
Copy link
Contributor

mavasani commented Mar 6, 2021

Apparently my fix does not fix the second hang: dotnet/roslyn#51652. I'll continue investigating.

Latest commit 44fb5c4 for #4928 fixes dotnet/roslyn#51652.

mavasani added a commit to mavasani/roslyn-analyzers that referenced this issue Mar 9, 2021
@jmarolf jmarolf modified the milestones: .NET 5.x, .NET 6 Preview 4 Apr 15, 2021
@nickwhaley
Copy link

This problem has reappeared in SDK 5.0.403.

Problem does not exist however in SDK 6.0.100.

@buyaa-n
Copy link
Contributor

buyaa-n commented Nov 19, 2021

There is something wrong with the latest SDK 5.0.403, another bug also reported @jmarolf @mavasani PTAL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Microsoft.CodeAnalysis.NetAnalyzers Bug The product is not behaving according to its current intended design Urgency-Soon
Projects
None yet
Development

No branches or pull requests

8 participants