Roslyn: Changing constraint from "struct" to "unmanaged" causes stack overflow and VS crash.

[Korporal]

I'm working on some code in VS 2017 (v 15.9.2) using v 7.3 of C#. This is a small console app but does some low level memory manipulation. I was refactoring the code earlier and decided that a constraint could be adjusted to unmanagedrather than structto support some richer operations.

Before this change the code compiles and runs exactly as expected. The code here is basically a generic structwhere the Tis itself constrained to be struct- changing this constraint to unmanagedis what leads to the failure.

I then decided to edit Program.cs in notepad just to change the constraint and then re-opened the solution in VS. I opened the solution (but not Program.cs) and ran a build, the build soon failed with stack overflow.

I then opened Program.csagain (in VS 2017) and VS simply freezes, stops interacting and responding to mouse etc. Left for long enough it abruptly dies and restarts.

I did explore the unmanagedconstraint recently but in a simple test app and probably used an earlier version of C# than 7.3 - I had no issues. So I simply opened the solution and changed each project to C# 7.2 but again as soon as I opened Program.cs VS freezes, same thing happens when I try C# 7.1 and C# 7.0 too.

NOTE: This is not a runtime issue, so don't suspect that the logic itself is involved, this is an IDE/compiler issue.

Thoughts?

[tannergooding]

Do you have a simple repro application that we can use to validate the issue locally?

[Korporal]

@tannergooding - That's a pretty fair question, the code however is proprietary and I'm unable to share it. I will however endeavor to create a simple repro, I'm sure I can incrementally copy bits n piece from the original until it happens. The "good" thing here is that it's not intermittent, very consistent.

I'm thinking the stack overflow must be a clue to what's happening - by the way, are there any special diagnostic flags I can add to the build or anything that could generate a log for you guys?

The "pattern" (if I can say that) is a generic struct X<T> where Tis itself constrained to struct(making it unmanagedis where it breaks). Then there is another struct Z which contains a private member field of type X<Z>. In this particular code (a proof of concept) both these structs are in Program.cs
It's possible this is the root and the other code in there is not a factor, I'll try to look at this later but it may be possible for someone to repro it using what I describe above.

[jmarolf]

@Korporal You can use the Windows Event Viewer to get the callstack of the crash

[Korporal]

@jmarolf - Ahh OK cool, if I get some time today I'll definitely dig this out and a possible simple repro...

[jmarolf]

@Korporal event past events should still be there so if you still have the machine it happened on you can get the original crash info

[jcouv deleted image as it confused multiple folks ;-)
https://user-images.githubusercontent.com/9797472/49233789-9d5ee800-f3ab-11e8-957f-1cbe8bcfbc72.png
]

[tannergooding]

Seems pretty easy to repro. The following hangs sharplab and crashes VS https://sharplab.io/#v2:EYLgHgbALAPgAgBgARwIwG4CwAoHcDMSAzgC4BOArgMYlIAaAPACoB8OSHSA7gBYCmZPkiZIQSCgDsAtgEMJMgOZ8AJjgDeOAL448hUpRpIAWuvacADmQCWANxkkhjIyyQAzK3wA2yrNm3YgA===

Using where T : struct works fine

```C#
using System;

public struct X
where T : unmanaged
{
}

public struct Z
{
private X field;
}
```

[Korporal]

@tannergooding - Attaboy, you got it - thanks!

[sharwell]

@jcouv Is this related to the signature help changes we worked on recently?

[jcouv]

@sharwell Yes, it is a regression caused by my recent change to signature help.
Gen fixed this issue in dev16 preview2 (PR https://github.com/dotnet/roslyn/pull/30997).
I'll add a test with Tanner's repro code to confirm.

[Korporal]

Hi, pleased to see this is readily addressed, very good. May I ask approx when would an update containing this fix expect to be released to the public? Or if this may be several months, is there a workaround by any chance?

[jcouv]

I'll first add a test to confirm this is fixed (stack trace looks like previous issue, but still good to validate with a test). I'll do that today.
Then I'll get back to you on availability or mitigation.

[jcouv]

Correction. This is not fixed and it is not related to the signature help work I had done recently.
Apologies for not reading the thread carefully enough. I got confused by Jon's screenshot which showed a stacktrace for signature help :-S

The issue at hand is a compiler stack overflow with unmanaged constraint.

@jaredpar Do you know how to query Watson to see the hit count on this?

```C#
[Fact, WorkItem(31439, "https://github.com/dotnet/roslyn/issues/31439")]
public void Test()
{
string source = @"
using System;

public struct X
where T : unmanaged
{
}

public struct Z
{
private X field;
}";
var comp = CreateCompilation(source);
comp.VerifyDiagnostics(); // stack overflow
}

Here's the repeating portion of the stacktrace:

FieldSymbol.Type.get()
BaseTypeAnalysis.DependsOnDefinitelyManagedType(CSharp.Symbols.NamedTypeSymbol type, HashSet partialClosure)
BaseTypeAnalysis.IsManagedType(CSharp.Symbols.NamedTypeSymbol type)
NamedTypeSymbol.IsManagedType.get()
SourceMemberContainerTypeSymbol.IsManagedType.get()
TypeSymbolWithAnnotations.IsManagedType.get()
ConstraintsHelper.CheckConstraints(CSharp.Symbol containingSymbol, CSharp.ConversionsBase conversions, CSharp.Symbols.TypeMap substitution, CSharp.Symbols.TypeParameterSymbol typeParameter, CSharp.Symbols.TypeSymbolWithAnnotations typeArgument, Compilation currentCompilation, PooledObjects.ArrayBuilder diagnosticsBuilder, PooledObjects.ArrayBuilder warningsBuilderOpt, ref PooledObjects.ArrayBuilder useSiteDiagnosticsBuilder, HashSet ignoreTypeConstraintsDependentOnTypeParametersOpt)
ConstraintsHelper.CheckConstraints(CSharp.Symbol containingSymbol, CSharp.ConversionsBase conversions, CSharp.Symbols.TypeMap substitution, System.Collections.Immutable.ImmutableArray typeParameters, System.Collections.Immutable.ImmutableArray typeArguments, Compilation currentCompilation, PooledObjects.ArrayBuilder diagnosticsBuilder, PooledObjects.ArrayBuilder warningsBuilderOpt, ref PooledObjects.ArrayBuilder useSiteDiagnosticsBuilder, BitVector skipParameters, HashSet ignoreTypeConstraintsDependentOnTypeParametersOpt)
ConstraintsHelper.CheckTypeConstraints(CSharp.Symbols.NamedTypeSymbol type, CSharp.ConversionsBase conversions, Compilation currentCompilation, PooledObjects.ArrayBuilder diagnosticsBuilder, PooledObjects.ArrayBuilder warningsBuilderOpt, ref PooledObjects.ArrayBuilder useSiteDiagnosticsBuilder)
ConstraintsHelper.CheckConstraintsForNonTuple(CSharp.Symbols.NamedTypeSymbol type, CSharp.ConversionsBase conversions, SyntaxNode typeSyntax, SeparatedSyntaxList typeArgumentsSyntax, Compilation currentCompilation, Roslyn.Utilities.ConsList basesBeingResolved, DiagnosticBag diagnostics)
CSharp.Binder.ConstructNamedType(CSharp.Symbols.NamedTypeSymbol type, SyntaxNode typeSyntax, SeparatedSyntaxList typeArgumentsSyntax, System.Collections.Immutable.ImmutableArray typeArguments, Roslyn.Utilities.ConsList basesBeingResolved, DiagnosticBag diagnostics)
CSharp.Binder.BindGenericSimpleNamespaceOrTypeOrAliasSymbol(CSharp.Syntax.GenericNameSyntax node, DiagnosticBag diagnostics, Roslyn.Utilities.ConsList basesBeingResolved, CSharp.Symbols.NamespaceOrTypeSymbol qualifierOpt)
CSharp.Binder.BindNamespaceOrTypeOrAliasSymbol(CSharp.Syntax.ExpressionSyntax syntax, DiagnosticBag diagnostics, Roslyn.Utilities.ConsList basesBeingResolved, bool suppressUseSiteDiagnostics)
CSharp.Binder.BindTypeOrAlias(CSharp.Syntax.ExpressionSyntax syntax, DiagnosticBag diagnostics, Roslyn.Utilities.ConsList basesBeingResolved)
CSharp.Binder.BindType(CSharp.Syntax.ExpressionSyntax syntax, DiagnosticBag diagnostics, Roslyn.Utilities.ConsList basesBeingResolved)
SourceMemberFieldSymbolFromDeclarator.GetFieldType(Roslyn.Utilities.ConsList fieldsBeingBound)
FieldSymbol.Type.get()
BaseTypeAnalysis.DependsOnDefinitelyManagedType(CSharp.Symbols.NamedTypeSymbol type, HashSet partialClosure)
```

[jaredpar]

@jcouv

Do you know how to query Watson to see the hit count on this?

If we can get a bucket ID of the crash it should be possible to query this.

[sharwell]

/cc @ivanbasov for help with reliability data

[ivanbasov]

1. I hope we already fixed it with https://github.com/dotnet/roslyn/pull/30997
   @genlu please let us know if my guess is wrong.
2. There is a watson issue: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/718094
   The issue was fixed in Dev 16 preview 1.
3. Based on:
   a. number of hits in the watson issue
   b. version number of the PR (16 preview 1)
   I'm resolving this issue and marking it with Dev 16 Preview 1

[jcouv]

@ivanbasov There was some confusion (to which I contributed) in the thread. This issue is not about an index-out-of-range in signature helper, it is a stack overflow related to unmanaged. Do you see that in Watson data?

Here's the repeating stacktrace in the overflow:

FieldSymbol.Type.get()
BaseTypeAnalysis.DependsOnDefinitelyManagedType(CSharp.Symbols.NamedTypeSymbol type, HashSet<CSharp.Symbol> partialClosure)
BaseTypeAnalysis.IsManagedType(CSharp.Symbols.NamedTypeSymbol type)
NamedTypeSymbol.IsManagedType.get()
SourceMemberContainerTypeSymbol.IsManagedType.get()
TypeSymbolWithAnnotations.IsManagedType.get()
ConstraintsHelper.CheckConstraints(CSharp.Symbol containingSymbol, CSharp.ConversionsBase conversions, CSharp.Symbols.TypeMap substitution, CSharp.Symbols.TypeParameterSymbol typeParameter, CSharp.Symbols.TypeSymbolWithAnnotations typeArgument, Compilation currentCompilation, PooledObjects.ArrayBuilder<CSharp.Symbols.TypeParameterDiagnosticInfo> diagnosticsBuilder, PooledObjects.ArrayBuilder<CSharp.Symbols.TypeParameterDiagnosticInfo> warningsBuilderOpt, ref PooledObjects.ArrayBuilder<CSharp.Symbols.TypeParameterDiagnosticInfo> useSiteDiagnosticsBuilder, HashSet<CSharp.Symbols.TypeParameterSymbol> ignoreTypeConstraintsDependentOnTypeParametersOpt)
ConstraintsHelper.CheckConstraints(CSharp.Symbol containingSymbol, CSharp.ConversionsBase conversions, CSharp.Symbols.TypeMap substitution, System.Collections.Immutable.ImmutableArray<CSharp.Symbols.TypeParameterSymbol> typeParameters, System.Collections.Immutable.ImmutableArray<CSharp.Symbols.TypeSymbolWithAnnotations> typeArguments, Compilation currentCompilation, PooledObjects.ArrayBuilder<CSharp.Symbols.TypeParameterDiagnosticInfo> diagnosticsBuilder, PooledObjects.ArrayBuilder<CSharp.Symbols.TypeParameterDiagnosticInfo> warningsBuilderOpt, ref PooledObjects.ArrayBuilder<CSharp.Symbols.TypeParameterDiagnosticInfo> useSiteDiagnosticsBuilder, BitVector skipParameters, HashSet<CSharp.Symbols.TypeParameterSymbol> ignoreTypeConstraintsDependentOnTypeParametersOpt)
ConstraintsHelper.CheckTypeConstraints(CSharp.Symbols.NamedTypeSymbol type, CSharp.ConversionsBase conversions, Compilation currentCompilation, PooledObjects.ArrayBuilder<CSharp.Symbols.TypeParameterDiagnosticInfo> diagnosticsBuilder, PooledObjects.ArrayBuilder<CSharp.Symbols.TypeParameterDiagnosticInfo> warningsBuilderOpt, ref PooledObjects.ArrayBuilder<CSharp.Symbols.TypeParameterDiagnosticInfo> useSiteDiagnosticsBuilder)
ConstraintsHelper.CheckConstraintsForNonTuple(CSharp.Symbols.NamedTypeSymbol type, CSharp.ConversionsBase conversions, SyntaxNode typeSyntax, SeparatedSyntaxList<CSharp.Syntax.TypeSyntax> typeArgumentsSyntax, Compilation currentCompilation, Roslyn.Utilities.ConsList<CSharp.Symbol> basesBeingResolved, DiagnosticBag diagnostics)
CSharp.Binder.ConstructNamedType(CSharp.Symbols.NamedTypeSymbol type, SyntaxNode typeSyntax, SeparatedSyntaxList<CSharp.Syntax.TypeSyntax> typeArgumentsSyntax, System.Collections.Immutable.ImmutableArray<CSharp.Symbols.TypeSymbolWithAnnotations> typeArguments, Roslyn.Utilities.ConsList<CSharp.Symbol> basesBeingResolved, DiagnosticBag diagnostics)
CSharp.Binder.BindGenericSimpleNamespaceOrTypeOrAliasSymbol(CSharp.Syntax.GenericNameSyntax node, DiagnosticBag diagnostics, Roslyn.Utilities.ConsList<CSharp.Symbol> basesBeingResolved, CSharp.Symbols.NamespaceOrTypeSymbol qualifierOpt)
CSharp.Binder.BindNamespaceOrTypeOrAliasSymbol(CSharp.Syntax.ExpressionSyntax syntax, DiagnosticBag diagnostics, Roslyn.Utilities.ConsList<CSharp.Symbol> basesBeingResolved, bool suppressUseSiteDiagnostics)
CSharp.Binder.BindTypeOrAlias(CSharp.Syntax.ExpressionSyntax syntax, DiagnosticBag diagnostics, Roslyn.Utilities.ConsList<CSharp.Symbol> basesBeingResolved)
CSharp.Binder.BindType(CSharp.Syntax.ExpressionSyntax syntax, DiagnosticBag diagnostics, Roslyn.Utilities.ConsList<CSharp.Symbol> basesBeingResolved)
SourceMemberFieldSymbolFromDeclarator.GetFieldType(Roslyn.Utilities.ConsList<CSharp.Symbols.FieldSymbol> fieldsBeingBound)
FieldSymbol.Type.get()
BaseTypeAnalysis.DependsOnDefinitelyManagedType(CSharp.Symbols.NamedTypeSymbol type, HashSet<CSharp.Symbol> partialClosure)

[tannergooding]

Is this something that might be handled by @RikkiGibson's changes in https://github.com/dotnet/roslyn/pull/31148?

[ivanbasov]

Thank you, @jcouv !

1. I was able to find crashes for corresponding to the crash on the screenshot you deleted.
2. I could not find any Watson crashes for the stacktrace provided in text.

[RikkiGibson]

I will try and add this test in my feature branch. I suspect constraint checks on X<Z> are causing Z to be recursively bound. I changed it to delay constraint checking until after field binding so I'm optimistic the test will just work.

[jcouv]

@RikkiGibson I assigned the issue to you since you're working in that area, and the issue is not caused by my change to SignatureHelp after all. Thanks

[Korporal]

@RikkiGibson - Hi, just curious how close we are to a release for this fix? Thx.

[jcouv]

@Korporal If all goes well, VS 2019 preview2 (no dates announced).

[Korporal]

@jcouv - Hi and thanks. So (just curious) why can't this simply be an update to the compiler and decouple it from a Visual Studio release? I frequntly install updates as we all do and I was under the impression that the compiler is no longer tied to VS releases, indeed we can select the compiler version easily for any project. This is also a rather fundamentql bug, complete failure to process valid C# source.

Since VS 2019 appears to expected in mid 2019 this implies the bug won't have a production ready fix for us to use for another 6 months.

[jcouv]

In a project, you don't select a "compiler version", you only select a "language version". There is only one version of the compiler installed with VS.

That said, Roslyn and the compiler are indeed well componentized inside VS, and it is actually possible to install Roslyn in VS as an extension.
But for long complicated reasons it is trouble to expose to end-users, because of an occasional bug in un-install scenario. We don't have enough adoption of Roslyn dailies to justify an onerous fix of that bug, and vice-versa.
The second reason is that VS and Roslyn interact via many APIs, and sometimes there are some lock-step changes required (can't use new version of Roslyn on old version of VS, or vice-versa).

This deployment/adoption/dogfooding issue is tracked by https://github.com/dotnet/roslyn/issues/18783

[Korporal]

@jcouv Thanks for explaining this but I must admit I'm still confused! Is this bug special in some way? in the sense that a fix cannot be made and released before VS 2019? I can see many bugs (most being quite obscure) listed here but some of these will presumably be fixed in VS 2017, does my question make sense? Once again, thanks, I'm just trying to understand the situation.

[jcouv]

Every bug is triaged and prioritized based on impact (how common is it, how bad is the experience), risk of change, and some other factors (regression, compatibility, etc). The bar for fixing issues in VS 2017 is very high at this point, and this bug does not meet that bar.

[Korporal]

Thanks, that explains things, much appreciated.

[jcouv]

Moved back to preview3. Check with Jared and move out again if ok. Thanks

[RikkiGibson]

@jcouv @jaredpar do you want this fix to go in at the same time as unmanaged constructed types, or to extract the specific change that fixes this scenario to its own PR for preview3?

[Korporal]

@RikkiGibson - Hi, is this fixed in the C# 8.0 beta that's part of VS 2019 16.0.0 preview 4.4? Attempting to compile this generates a compilation error (the crash has gone but it seems there are still issues):

namespace ConsoleApp3
{
    class Program
    {
        static void Main(string[] args)
        {
        }
    }

    public struct X<T> where T : unmanaged
    {

    }

    public struct Z
    {
        private X<Z> field;
    }
}

[jmarolf]

@Korporal this appears to work in master

[Korporal]

@jmarolf - Hi, I'm not familair with sharplab, looks pretty useful but each time I visit I see a strange error messages at the top of the page:

[connection lost, reconnecting…]

and I have no idea what that means, the left hand side edit window becomes greyed out too.

[RikkiGibson]

It can be finicky occasionally.. I suggest just refreshing, maybe clearing caches and stuff, and report it in https://github.com/ashmind/sharplab if it keeps happening.

[Korporal]

@RikkiGibson @jmarolf @jcouv - Hi. There's is considerable confusion here - I cannot get an answer from anyone in the csharplang repo as to whether the code fragment _should_ actually compile (is legal C#) or if it should fail to compile and if it should fail then the error message:

The type 'Z' must be a non-nullable value type, along with all fields at any level of nesting, in order to use it as parameter 'T' in the generic type or method 'X'

is an appropriate error message.

See this issue which is a recent history of the discussion with the csharplang techies.

Is there _anyone_ who knows enough about the language and its formal specification who can give a definitive yes/no answer? My own understanding is that the code should compile when the unmanagedconstraint is used just as it does when the structconstraint is used.

Surely this is a simple matter of examining the formal language specification? after all a fix was made by @RikkiGibson (as stated in this issue thread) so that fix must have been to make the compiler align with said specification, yes?

Thanks!

[RikkiGibson]

Apologies for the delay.

I suspect the hang you're experiencing in SharpLab is due to using the 2.9.0 compiler instead of the master compiler (select 'master' from the dropdown list). Sometimes when the compiler stack overflows on the server side, SharpLab will just hang.

Now, regarding the sample:

public struct X<T> where T : unmanaged
{
}

public struct Z
{
    private X<Z> field; // error in 7.3: feature 'unmanaged constructed types' requires C# 8.0
}

This is not valid in C# 7.3 and earlier. The reason is that Z is considered 'managed' by that language version due to containing a field of a constructed type (namely X<Z>.)

However, this is valid in C# 8. The unmanaged constructed types feature (see dotnet/csharplang#1744) relaxes the rules to allow types such as Z to be considered unmanaged as long as the types of all fields are unmanaged.

[tannergooding]

It is valid in the current master bits and can be tested by pulling down a nightly build of the compiler. For example: https://sharplab.io/#v2:EYLgtghgzgLgpgJwD4AEDMACWCCuBjGDADQB4AVAPgwHcALRODMjEDHAO0nYgHM4ATALAAoAN4iAviJHosMXAQwAtEeOEYNGAA4IAlgDcI8YiSVUAZrrgAbfgG4MAekcZECAPYIMu9hgDsAHRorOZwRjgIjADkHFy8Ahh47uzY+PD8GDAAnlpwUFEYkQCOOLqRUBgAwgDEGAAcAQAMkkA===

C# 8 is not RTM and the compiler that shipped in VS2019 only provides C# 8 in a preview form (LangVersion=latest still returns 7.3; and you have to explicit opt into C# 8 using LangVersion=8 or LangVersion=preview).