Handle cycles in IsValueType and IsReferenceType binding constraints #47968

cston · 2020-09-23T05:17:40Z

Ignore #nullable context when evaluating IsValueType and IsReferenceType.

Binding constraints in the repros results in a call to TypeWithAnnotations.SubstituteTypeCore() which calls IsTypeParameterDisallowingAnnotationsInCSharp8() which calls SourceTypeParameterSymbol.IsValueType. The call to IsValueType requires binding constraints. It seems unnecessary to have IsValueType or IsReferenceType depend on nullability though.

AlekseyTs · 2020-09-23T16:44:52Z

src/Compilers/CSharp/Portable/Symbols/Source/TypeParameterConstraintClause.cs

@@ -212,6 +218,15 @@ static void adjustConstraintTypes(Symbol container, ImmutableArray<TypeWithAnnot

    internal static class TypeParameterConstraintClauseExtensions
    {
+        internal static bool IsSet(this ImmutableArray<TypeParameterConstraintClause> constraintClauses, bool canIgnoreNullableContext)


IsSet [](start = 29, length = 5)

HasValue? #Closed

AlekseyTs · 2020-09-23T16:48:05Z

src/Compilers/CSharp/Portable/Symbols/TypeParameterBounds.cs

+        /// Creates a "early" bound instance that has constraint types set
+        /// but no other fields.
+        /// </summary>
+        public TypeParameterBounds(ImmutableArray<TypeWithAnnotations> constraintTypes)


TypeParameterBounds [](start = 15, length = 19)

Would it make sense to keep this constructor private and to add a factory method with very specific name for consumers to use? #Closed

RikkiGibson · 2020-09-23T17:33:09Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceNamedTypeSymbol.cs

                {
-                    this.AddDeclarationDiagnostics(diagnostics);
+                    if  (_lazyTypeParameterConstraints.HasValue(canIgnoreNullableContext: false))


nit: formatting #Resolved

AlekseyTs · 2020-09-23T17:50:44Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceNamedTypeSymbol.cs

            {
                var diagnostics = DiagnosticBag.GetInstance();
-                if (ImmutableInterlocked.InterlockedInitialize(ref _lazyTypeParameterConstraints, MakeTypeParameterConstraints(diagnostics)))
+                if (ImmutableInterlocked.InterlockedCompareExchange(ref _lazyTypeParameterConstraints, MakeTypeParameterConstraints(canIgnoreNullableContext, diagnostics), clauses) == clauses)


InterlockedCompareExchange [](start = 41, length = 26)

Is there a possibility of a race here? If a thread with canIgnoreNullableContext==true completes first, is the thread with canIgnoreNullableContext==false going to return expected value? #Closed

Good catch, thanks.

In reply to: 493779200 [](ancestors = 493779200)

AlekseyTs · 2020-09-23T17:55:17Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceTypeParameterSymbol.cs


-                if (ReferenceEquals(Interlocked.CompareExchange(ref _lazyBounds, bounds, TypeParameterBounds.Unset), TypeParameterBounds.Unset))
+                if (ReferenceEquals(Interlocked.CompareExchange(ref _lazyBounds, bounds, currentBounds), currentBounds))


Interlocked.CompareExchange(ref _lazyBounds, bounds, currentBounds), currentBounds) [](start = 36, length = 83)

Is there a race here? #Closed

AlekseyTs · 2020-09-23T18:27:25Z

    public override ImmutableArray<TypeParameterConstraintClause> GetTypeParameterConstraintClauses()

Should we add bool canIgnoreNullableContext here as well? #Closed

Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs:284 in cbbf3b8. [](commit_id = cbbf3b8, deletion_comment = False)

AlekseyTs · 2020-09-23T18:29:53Z

    public override ImmutableArray<TypeParameterConstraintClause> GetTypeParameterConstraintClauses()

Should we add bool canIgnoreNullableContext here as well? #Closed

Refers to: src/Compilers/CSharp/Portable/Symbols/Source/LocalFunctionSymbol.cs:464 in cbbf3b8. [](commit_id = cbbf3b8, deletion_comment = False)

AlekseyTs · 2020-09-23T23:37:42Z

src/Compilers/CSharp/Portable/Binder/Semantics/AccessCheck.cs

@@ -234,7 +234,7 @@ private static bool IsNamedTypeAccessible(NamedTypeSymbol type, Symbol within, r
                {
                    // type parameters are always accessible, so don't check those (so common it's
                    // worth optimizing this).
-                    if (typeArg.Type.Kind != SymbolKind.TypeParameter && !IsSymbolAccessibleCore(typeArg.Type, within, null, out unused, compilation, ref useSiteDiagnostics, basesBeingResolved))


Type [](start = 32, length = 4)

Why not simply access DefaultType here? #Closed

AlekseyTs · 2020-09-23T23:38:28Z

src/Compilers/CSharp/Portable/Symbols/NamedTypeSymbol.cs

@@ -63,7 +63,7 @@ internal ImmutableArray<TypeWithAnnotations> TypeArgumentsWithDefinitionUseSiteD

            foreach (var typeArgument in result)
            {
-                typeArgument.Type.OriginalDefinition.AddUseSiteDiagnostics(ref useSiteDiagnostics);


Type [](start = 29, length = 4)

Why not simply access DefaultType here? #Closed

It would be good to avoid repeating type.Default.OriginalDefinition.AddUseSiteDiagnostics(). Moved the helper method here.

In reply to: 493955260 [](ancestors = 493955260)

AlekseyTs · 2020-09-23T23:38:41Z

src/Compilers/CSharp/Portable/Symbols/NamedTypeSymbol.cs

@@ -72,7 +72,7 @@ internal ImmutableArray<TypeWithAnnotations> TypeArgumentsWithDefinitionUseSiteD
        internal TypeWithAnnotations TypeArgumentWithDefinitionUseSiteDiagnostics(int index, ref HashSet<DiagnosticInfo> useSiteDiagnostics)
        {
            var result = TypeArgumentsWithAnnotationsNoUseSiteDiagnostics[index];
-            result.Type.OriginalDefinition.AddUseSiteDiagnostics(ref useSiteDiagnostics);


Type [](start = 19, length = 4)

Why not simply access DefaultType here? #Closed

AlekseyTs · 2020-09-23T23:40:07Z

src/Compilers/CSharp/Portable/Symbols/TypeWithAnnotations.cs

@@ -428,6 +430,9 @@ public bool IsAtLeastAsVisibleAs(Symbol sym, ref HashSet<DiagnosticInfo> useSite
            return NullableUnderlyingTypeOrSelf.IsAtLeastAsVisibleAs(sym, ref useSiteDiagnostics);
        }

+        public void AddUseSiteDiagnostics(ref HashSet<DiagnosticInfo> useSiteDiagnostics) =>


AddUseSiteDiagnostics [](start = 20, length = 21)

AddDefinitionUseSiteDiagnostics? #Closed

AlekseyTs · 2020-09-23T23:43:13Z

src/Compilers/CSharp/Portable/Symbols/TypeWithAnnotations.cs

@@ -468,9 +473,18 @@ internal TypeWithAnnotations SubstituteTypeCore(AbstractTypeMap typeMap)

            NullableAnnotation newAnnotation;

-            Debug.Assert(!IsIndexedTypeParameter(newTypeWithModifiers.Type) || newTypeWithModifiers.NullableAnnotation.IsOblivious());
+            //Debug.Assert(!IsIndexedTypeParameter(newTypeWithModifiers.Type) || newTypeWithModifiers.NullableAnnotation.IsOblivious());


//Debug. [](start = 12, length = 8)

Commented out code #Closed

AlekseyTs · 2020-09-24T16:52:00Z

src/Compilers/CSharp/Portable/Symbols/ConstraintsHelper.cs

@@ -295,7 +297,7 @@ internal static class ConstraintsHelper
                return null;
            }

-            var bounds = new TypeParameterBounds(constraintTypes, interfaces, effectiveBaseClass, deducedBaseType);
+            var bounds = new TypeParameterBounds(constraintTypes, interfaces, effectiveBaseClass, deducedBaseType, ignoresNullableContext);


ignoresNullableContext [](start = 115, length = 22)

It doesn't look like this value actually affects other content in the TypeParameterBounds instance. Why not always use false and and getting rid of the parameter. #Closed

AlekseyTs · 2020-09-24T17:01:39Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceTypeParameterSymbol.cs

@@ -680,7 +684,7 @@ private TypeParameterConstraintClause GetTypeParameterConstraintClause()
            return constraintClauses.IsEmpty ? TypeParameterConstraintClause.Empty : constraintClauses[Ordinal];
        }

-        protected override TypeParameterBounds ResolveBounds(ConsList<TypeParameterSymbol> inProgress, DiagnosticBag diagnostics)
+        protected override TypeParameterBounds ResolveBounds(ConsList<TypeParameterSymbol> inProgress, bool canIgnoreNullableContext, DiagnosticBag diagnostics)


canIgnoreNullableContext [](start = 108, length = 24)

It looks like the parameter is no longer used. Is this intentional? #Closed

AlekseyTs · 2020-09-24T20:59:16Z

src/Compilers/CSharp/Portable/Symbols/TypeParameterBounds.cs

+
+        // Returns true if bounds was updated with value.
+        // Returns false if bounds already had a value with expected 'IgnoresNullableContext'
+        // or was updated to a value with the expected 'IgnoresNullableContext' value on another thread.


another [](start = 89, length = 7)

"this"? #Closed

AlekseyTs

LGTM (iteration 11), assuming CI is passing.

RikkiGibson

LGTM, just had some comment nits to pick. No need to address in this PR if it is not practical

RikkiGibson · 2020-09-24T23:46:13Z

src/Compilers/CSharp/Portable/Symbols/TypeParameterBounds.cs

+        }
+
+        // Returns true if bounds was updated with value.
+        // Returns false if bounds already had a value with expected 'IgnoresNullableContext'


nit: consider wording this as "a value with sufficient IgnoresNullableContext". Similar on the next line.

Something to indicate that when the caller requests a bounds that ignores nullable context, the symbol may provide a bounds that doesn't ignore nullable context.

Updated comment in #48041.

RikkiGibson · 2020-09-24T23:54:52Z

src/Compilers/CSharp/Portable/Symbols/Source/TypeParameterConstraintClause.cs

        internal static bool ContainsOnlyEmptyConstraintClauses(this ImmutableArray<TypeParameterConstraintClause> constraintClauses)
        {
            return constraintClauses.All(clause => clause.IsEmpty);
        }
+
+        // Returns true if constraintClauses was updated with value.
+        // Returns false if constraintClauses already had a value with expected 'IgnoresNullableContext'


Same comment here as in TypeParameterBounds.cs.

Revert 1f53c66 (dotnet@1f53c66)

cston requested a review from a team as a code owner September 23, 2020 05:17

Handle cycles in IsValueType and IsReferenceType binding constraints

bc5d41e

cston force-pushed the 47513 branch from 920a5f2 to bc5d41e Compare September 23, 2020 05:41

Misc.

cbbf3b8

RikkiGibson self-assigned this Sep 23, 2020

RikkiGibson added the Area-Compilers label Sep 23, 2020

AlekseyTs reviewed Sep 23, 2020

View reviewed changes

cston added 3 commits September 23, 2020 09:50

Fix tests

3c76be2

PR feedback

417f48b

Misc.

9367f0a

RikkiGibson reviewed Sep 23, 2020

View reviewed changes

AlekseyTs reviewed Sep 23, 2020

View reviewed changes

cston added 3 commits September 23, 2020 11:45

More tests

d88c2fd

Revert change

2558034

More cycles

f9061da

This was referenced Sep 23, 2020

Test failures via timeline in Roslyn jaredpar/runfo#37

Closed

Test failures via timeline in Roslyn jaredpar/runfo#38

Closed

AlekseyTs reviewed Sep 23, 2020

View reviewed changes

Calculate TypeParameterBounds ignoring nullable context if necessary

89eeb4c

AlekseyTs reviewed Sep 24, 2020

View reviewed changes

PR feedback

d1360db

Fix test

6e1fe38

AlekseyTs reviewed Sep 24, 2020

View reviewed changes

AlekseyTs approved these changes Sep 24, 2020

View reviewed changes

RikkiGibson approved these changes Sep 24, 2020

View reviewed changes

cston merged commit 1f53c66 into dotnet:release/dev16.8 Sep 25, 2020

cston deleted the 47513 branch September 25, 2020 00:05

cston added this to the 16.8.P4 milestone Sep 25, 2020

This was referenced Sep 29, 2020

Prevent potential cycles binding constraint types #48154

Closed

Missing warnings on overriding a method with different nullability attributes #41368

Closed

jcouv mentioned this pull request Oct 3, 2020

Use lightweight type constraint binding for IsValueType #48270

Closed

AlekseyTs added a commit to AlekseyTs/roslyn that referenced this pull request Nov 5, 2020

Revert dotnet#47968

7b0c29a

Revert 1f53c66 (dotnet@1f53c66)

AlekseyTs added a commit to AlekseyTs/roslyn that referenced this pull request Nov 12, 2020

Revert dotnet#47968

50de29a

Revert 1f53c66 (dotnet@1f53c66)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle cycles in IsValueType and IsReferenceType binding constraints #47968

Handle cycles in IsValueType and IsReferenceType binding constraints #47968

cston commented Sep 23, 2020 •

edited

Loading

AlekseyTs Sep 23, 2020 •

edited

Loading

AlekseyTs Sep 23, 2020 •

edited

Loading

RikkiGibson Sep 23, 2020 •

edited by cston

Loading

AlekseyTs Sep 23, 2020 •

edited

Loading

cston Sep 24, 2020

AlekseyTs Sep 23, 2020 •

edited

Loading

AlekseyTs commented Sep 23, 2020 •

edited

Loading

AlekseyTs commented Sep 23, 2020 •

edited

Loading

AlekseyTs Sep 23, 2020 •

edited

Loading

AlekseyTs Sep 23, 2020 •

edited

Loading

cston Sep 24, 2020

AlekseyTs Sep 23, 2020 •

edited

Loading

AlekseyTs Sep 23, 2020 •

edited

Loading

AlekseyTs Sep 23, 2020 •

edited

Loading

AlekseyTs Sep 24, 2020 •

edited

Loading

AlekseyTs Sep 24, 2020 •

edited

Loading

AlekseyTs Sep 24, 2020 •

edited

Loading

AlekseyTs left a comment

RikkiGibson left a comment

RikkiGibson Sep 24, 2020

cston Sep 25, 2020 •

edited

Loading

RikkiGibson Sep 24, 2020


		if (ReferenceEquals(Interlocked.CompareExchange(ref _lazyBounds, bounds, TypeParameterBounds.Unset), TypeParameterBounds.Unset))
		if (ReferenceEquals(Interlocked.CompareExchange(ref _lazyBounds, bounds, currentBounds), currentBounds))

Handle cycles in IsValueType and IsReferenceType binding constraints #47968

Handle cycles in IsValueType and IsReferenceType binding constraints #47968

Conversation

cston commented Sep 23, 2020 • edited Loading

AlekseyTs Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

RikkiGibson Sep 23, 2020 • edited by cston Loading

Choose a reason for hiding this comment

AlekseyTs Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

cston Sep 24, 2020

Choose a reason for hiding this comment

AlekseyTs Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Sep 23, 2020 • edited Loading

AlekseyTs commented Sep 23, 2020 • edited Loading

AlekseyTs Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

cston Sep 24, 2020

Choose a reason for hiding this comment

AlekseyTs Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Sep 24, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Sep 24, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Sep 24, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

RikkiGibson left a comment

Choose a reason for hiding this comment

RikkiGibson Sep 24, 2020

Choose a reason for hiding this comment

cston Sep 25, 2020 • edited Loading

Choose a reason for hiding this comment

RikkiGibson Sep 24, 2020

Choose a reason for hiding this comment

cston commented Sep 23, 2020 •

edited

Loading

AlekseyTs Sep 23, 2020 •

edited

Loading

AlekseyTs Sep 23, 2020 •

edited

Loading

RikkiGibson Sep 23, 2020 •

edited by cston

Loading

AlekseyTs Sep 23, 2020 •

edited

Loading

AlekseyTs Sep 23, 2020 •

edited

Loading

AlekseyTs commented Sep 23, 2020 •

edited

Loading

AlekseyTs commented Sep 23, 2020 •

edited

Loading

AlekseyTs Sep 23, 2020 •

edited

Loading

AlekseyTs Sep 23, 2020 •

edited

Loading

AlekseyTs Sep 23, 2020 •

edited

Loading

AlekseyTs Sep 23, 2020 •

edited

Loading

AlekseyTs Sep 23, 2020 •

edited

Loading

AlekseyTs Sep 24, 2020 •

edited

Loading

AlekseyTs Sep 24, 2020 •

edited

Loading

AlekseyTs Sep 24, 2020 •

edited

Loading

cston Sep 25, 2020 •

edited

Loading