Avoid cycles while binding type parameter constraints #49261

AlekseyTs · 2020-11-10T15:52:59Z

This change consolidates two different techniques to avoid cycles into one. It is using special restricted mode of binding constraint types (proposed in #48270) but avoids the need to capture "partially" bound constraint types in symbols and the risk to use them when not appropriate. Instead, the information required to calculate IsValueType/IsReferenceType is immediately calculated from "partially" bound constraint types and preserved as special TypeParameterConstraintKind flags.

jcouv · 2020-11-10T18:11:44Z

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

        /// <returns>True if the types are equivalent.</returns>
-        internal virtual bool Equals(TypeSymbol t2, TypeCompareKind compareKind, IReadOnlyDictionary<TypeParameterSymbol, bool> isValueTypeOverrideOpt = null)
+        internal virtual bool Equals(TypeSymbol t2, TypeCompareKind compareKind)


Thanks for catching that. It's a nice cleanup.

jcouv

LGTM Thanks (iteration 3)

AlekseyTs · 2020-11-10T18:17:34Z

@cston Please take a look.

cston · 2020-11-11T19:22:58Z

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

+
+            if (clauses.All(clause => clause.ConstraintTypes.IsEmpty))
+            {
+                clauses = ImmutableArray<TypeParameterConstraintClause>.Empty;


clauses = ImmutableArray.Empty [](start = 16, length = 61)

return ImmutableArray<TypeWithAnnotations>.Empty; #Resolved

cston · 2020-11-11T19:23:27Z

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

-            binder = binder.WithAdditionalFlags(BinderFlags.GenericConstraintsClause | BinderFlags.SuppressConstraintChecks);
+            if (clauses.All(clause => clause.Constraints == TypeParameterConstraintKind.None))
+            {
+                clauses = ImmutableArray<TypeParameterConstraintClause>.Empty;


clauses = ImmutableArray.Empty; [](start = 16, length = 62)

return ImmutableArray<TypeParameterConstraintKind>.Empty; #Resolved

cston · 2020-11-11T19:59:04Z

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

-    internal static class TypeParameterConstraintClauseExtensions
-    {
-        internal static bool HasValue(this ImmutableArray<TypeParameterConstraintClause> constraintClauses, bool canIgnoreNullableContext)
+        internal static SmallDictionary<TypeParameterSymbol, bool> BuildIsReferenceTypeFromConstraintTypesMap(Symbol container, ImmutableArray<TypeParameterSymbol> typeParameters,


FromConstraintTypesMap [](start = 87, length = 22)

Is there a reason this method name contains "FromConstraintTypes" and the previous method name does not? #Resolved

Is there a reason this method name contains "FromConstraintTypes" and the previous method name does not?

Yes, this method only takes constraint types into account and the previous method does not.

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

cston · 2020-11-11T20:04:59Z

src/Compilers/CSharp/Portable/Symbols/Wrapped/WrappedTypeParameterSymbol.cs

+        {
+            get
+            {
+                return _underlyingTypeParameter.IsReferenceTypeFromConstraintTypes || CalculateIsReferenceTypeFromConstraintTypes(ConstraintTypesNoUseSiteDiagnostics);


|| CalculateIsReferenceTypeFromConstraintTypes(ConstraintTypesNoUseSiteDiagnostics) [](start = 83, length = 83)

Is this necessary? #Resolved

Is this necessary?

Yes, due to possible type substitution involved new types can appear in the constraint types

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

cston · 2020-11-11T20:05:09Z

src/Compilers/CSharp/Portable/Symbols/Wrapped/WrappedTypeParameterSymbol.cs

+        {
+            get
+            {
+                return _underlyingTypeParameter.IsValueTypeFromConstraintTypes || CalculateIsValueTypeFromConstraintTypes(ConstraintTypesNoUseSiteDiagnostics);


|| CalculateIsValueTypeFromConstraintTypes(ConstraintTypesNoUseSiteDiagnostics) [](start = 79, length = 79)

Is this necessary? #Resolved

Is this necessary?

Yes, due to possible type substitution involved new types can appear in the constraint types

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

cston · 2020-11-11T20:07:08Z

src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/Symbols/EETypeParameterSymbol.cs

+        {
+            get
+            {
+                return _sourceTypeParameter.IsReferenceTypeFromConstraintTypes || CalculateIsReferenceTypeFromConstraintTypes(ConstraintTypesNoUseSiteDiagnostics);


|| CalculateIsReferenceTypeFromConstraintTypes(ConstraintTypesNoUseSiteDiagnostics) [](start = 79, length = 83)

Is this necessary? #Resolved

Is this necessary?

Yes, due to possible type substitution involved new types can appear in the constraint types

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

cston · 2020-11-11T20:07:18Z

src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/Symbols/EETypeParameterSymbol.cs

+        {
+            get
+            {
+                return _sourceTypeParameter.IsValueTypeFromConstraintTypes || CalculateIsValueTypeFromConstraintTypes(ConstraintTypesNoUseSiteDiagnostics);


|| CalculateIsValueTypeFromConstraintTypes(ConstraintTypesNoUseSiteDiagnostics) [](start = 75, length = 79)

Is this necessary? #Resolved

Is this necessary?

Yes, due to possible type substitution involved new types can appear in the constraint types

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

Revert 1f53c66 (dotnet@1f53c66)

ghost · 2020-11-12T15:51:02Z

Hello @AlekseyTs!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost

Auto-approval

AlekseyTs added the Area-Compilers label Nov 10, 2020

AlekseyTs requested review from cston, jcouv and a team November 10, 2020 15:53

jcouv reviewed Nov 10, 2020

View reviewed changes

jcouv approved these changes Nov 10, 2020

View reviewed changes

jcouv self-assigned this Nov 10, 2020

jcouv mentioned this pull request Nov 11, 2020

Use lightweight type constraint binding for IsValueType #48270

Closed

cston reviewed Nov 11, 2020

View reviewed changes

cston approved these changes Nov 11, 2020

View reviewed changes

AlekseyTs and others added 2 commits November 12, 2020 07:34

Revert dotnet#47968

50de29a

Revert 1f53c66 (dotnet@1f53c66)

Avoid cycles while binding type parameter constraints

a122504

AlekseyTs force-pushed the ConstraintCycles_02 branch from c1886d2 to a122504 Compare November 12, 2020 15:49

AlekseyTs added the auto-merge label Nov 12, 2020

ghost approved these changes Nov 12, 2020

View reviewed changes

ghost merged commit 98cbb71 into dotnet:master Nov 12, 2020

ghost added this to the Next milestone Nov 12, 2020

allisonchou modified the milestones: Next, 16.9.P2 Nov 24, 2020

AlekseyTs mentioned this pull request Dec 1, 2020

Prevent potential cycles binding constraint types #48154

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid cycles while binding type parameter constraints #49261

Avoid cycles while binding type parameter constraints #49261

AlekseyTs commented Nov 10, 2020

jcouv Nov 10, 2020

jcouv left a comment

AlekseyTs commented Nov 10, 2020

cston Nov 11, 2020 •

edited by AlekseyTs

Loading

cston Nov 11, 2020 •

edited by AlekseyTs

Loading

cston Nov 11, 2020 •

edited by AlekseyTs

Loading

AlekseyTs Nov 11, 2020

cston Nov 11, 2020 •

edited by AlekseyTs

Loading

AlekseyTs Nov 11, 2020 •

edited

Loading

cston Nov 11, 2020 •

edited by AlekseyTs

Loading

AlekseyTs Nov 11, 2020

cston Nov 11, 2020 •

edited by AlekseyTs

Loading

AlekseyTs Nov 11, 2020

cston Nov 11, 2020 •

edited by AlekseyTs

Loading

AlekseyTs Nov 11, 2020

ghost commented Nov 12, 2020

ghost left a comment

Avoid cycles while binding type parameter constraints #49261

Avoid cycles while binding type parameter constraints #49261

Conversation

AlekseyTs commented Nov 10, 2020

jcouv Nov 10, 2020

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

AlekseyTs commented Nov 10, 2020

cston Nov 11, 2020 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

cston Nov 11, 2020 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

cston Nov 11, 2020 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Nov 11, 2020

Choose a reason for hiding this comment

cston Nov 11, 2020 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Nov 11, 2020 • edited Loading

Choose a reason for hiding this comment

cston Nov 11, 2020 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Nov 11, 2020

Choose a reason for hiding this comment

cston Nov 11, 2020 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Nov 11, 2020

Choose a reason for hiding this comment

cston Nov 11, 2020 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Nov 11, 2020

Choose a reason for hiding this comment

ghost commented Nov 12, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost left a comment

Choose a reason for hiding this comment

cston Nov 11, 2020 •

edited by AlekseyTs

Loading

cston Nov 11, 2020 •

edited by AlekseyTs

Loading

cston Nov 11, 2020 •

edited by AlekseyTs

Loading

cston Nov 11, 2020 •

edited by AlekseyTs

Loading

AlekseyTs Nov 11, 2020 •

edited

Loading

cston Nov 11, 2020 •

edited by AlekseyTs

Loading

cston Nov 11, 2020 •

edited by AlekseyTs

Loading

cston Nov 11, 2020 •

edited by AlekseyTs

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.