Revert two step initialization of constraints by AlekseyTs · Pull Request #35375 · dotnet/roslyn

AlekseyTs · 2019-04-30T20:57:33Z

Closes #28834.
Closes #30061.

Closes dotnet#28834. Closes dotnet#30061.

AlekseyTs · 2019-05-01T16:59:25Z

@cston, @dotnet/roslyn-compiler Please review. #Resolved

AlekseyTs · 2019-05-02T17:08:50Z

@cston, @dotnet/roslyn-compiler Please review. #Resolved

jcouv · 2019-05-02T18:00:30Z

src/Compilers/CSharp/Test/Symbol/Symbols/GenericConstraintTests.cs

                // (6,30): error CS0717: 'S': static classes cannot be used as constraints
                // partial class C<T> where T : S { }
                Diagnostic(ErrorCode.ERR_ConstraintIsStaticClass, "S").WithArguments("S").WithLocation(6, 30),
+                // (7,15): error CS0265: Partial declarations of 'D<T>' have inconsistent constraints for type parameter 'T'


error CS0265: Partial declarations of 'D' have inconsistent constraints for type parameter 'T' [](start = 27, length = 97)

I didn't understand this error. The three partial declarations for D<T> seem to agree: A, I, I, A and I, A. #Closed

I didn't understand this error. The three partial declarations for D seem to agree: A, I, I, A and I, A.

See the error below. Invalid constraint types are removed from the set before partials are matched, therefore other partials do not have 'A' among the constraints. I believe this behavior matches C# 7.3 compiler

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

Thanks

In reply to: 280541572 [](ancestors = 280541572,280533172)

jcouv · 2019-05-02T18:04:32Z

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

            }
            var map = _map.TypeMap;
            Debug.Assert(map != null);
-            // Use typeParameter.ConstraintTypesNoUseSiteDiagnostics rather than


// Use typeParameter.ConstraintTypesNoUseSiteDiagnostics rather than [](start = 12, length = 68)

Feels like this comment is still applicable/useful, no? #Closed

Feels like this comment is still applicable/useful, no?

GetConstraintTypesNoUseSiteDiagnostics is removed, ConstraintTypesNoUseSiteDiagnostics has no alternative

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

333fred · 2019-05-02T18:27:09Z

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

+        }
+
        // From typedesc.cpp :
        // > A recursive helper that helps determine whether this variable is constrained as ObjRef.


Is this comment relevant anymore? #Resolved

Is this comment relevant anymore?

I think the method is still recursive, indirectly. In any case, I guess this comment is on this method from the beginning of times and refers to relevant parts of native compiler implementation.

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

jcouv · 2019-05-02T18:29:16Z

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

@@ -300,7 +297,7 @@ internal bool IsEqualToOrDerivedFrom(TypeSymbol type, TypeCompareKind comparison
        /// You can ignore custom modifiers, ignore the distinction between object and dynamic, or ignore tuple element names differences.
        /// </param>


[](start = 12, length = 8)

Consider adding xml doc for new isValueTypeOverrideOpt parameter as well. #Resolved

cston · 2019-05-02T18:53:20Z

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

+                isValueTypeOverride = new SmallDictionary<TypeParameterSymbol, bool>(ReferenceEqualityComparer.Instance);

-        internal bool IsEmpty => Constraints == TypeParameterConstraintKind.None && ConstraintTypes.IsEmpty && OtherPartialDeclarations.ContainsOnlyEmptyConstraintClauses();
+                for (int i = 0; i < typeParameters.Length; i++)


for (int i = 0; i < typeParameters.Length; i++) [](start = 16, length = 47)

Please consider using foreach. #Resolved

cston · 2019-05-02T18:53:47Z

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

+            }

-        internal bool IsEarly => !TypeConstraintsSyntax.IsDefault || !OtherPartialDeclarations.IsEmpty;
+            for (int i = 0; i < typeParameters.Length; i++)


for (int i = 0; i < typeParameters.Length; i++) [](start = 12, length = 47)

Please consider using foreach. #Resolved

cston · 2019-05-02T19:00:11Z

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

+        internal static bool IsValueTypeFromConstraintTypes(ImmutableArray<TypeWithAnnotations> constraintTypes)
        {
-            if (constraintTypes.IsEmpty)
+            foreach (var constraintType in constraintTypes)


foreach [](start = 12, length = 7)

return constraintTypes.Any(constraintType => constraintType.Type.IsValueType); #WontFix

cston · 2019-05-02T19:00:53Z

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

-                                      });
+        internal static bool IsReferenceTypeFromConstraintTypes(ImmutableArray<TypeWithAnnotations> constraintTypes)
+        {
+            foreach (var constraintType in constraintTypes)


foreach [](start = 12, length = 7)

return constraintTypes.Any(constraintType => ConstraintImpliesReferenceType(constraintType.Type)); #WontFix

cston · 2019-05-02T19:10:14Z

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

+            internal static readonly EqualsComparer ConsiderEverythingComparer = new EqualsComparer(TypeCompareKind.ConsiderEverything, isValueTypeOverrideOpt: null);

            private readonly TypeCompareKind _compareKind;
+            private readonly SmallDictionary<TypeParameterSymbol, bool> _isValueTypeOverrideOpt;


SmallDictionary [](start = 29, length = 15)

It's unfortunate this is a mutable dictionary. Should all of the Equals() infrastructure use ImmutableDictionary<TypeParameterSymbol, bool> instead? #Resolved

jcouv · 2019-05-02T20:24:45Z

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


        internal abstract TypeSymbol GetDeducedBaseType(ConsList<TypeParameterSymbol> inProgress);

+        private static bool ConstraintImpliesReferenceType(TypeSymbol constraint)


ConstraintImpliesReferenceType [](start = 28, length = 30)

nit: I'd be tempted to name this method like the one below, but singular IsReferenceTypeFromConstraintType. I was thrown off by the distinction between "is" and "implies".
Along those lines, this method could be a local function inside of IsReferenceTypeFromConstraintTypes. #WontFix

This method and others in this file are simply restored to their original state prior to #27525.

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

This include names, etc.

In reply to: 280583028 [](ancestors = 280583028,280582299)

jcouv · 2019-05-02T22:07:38Z

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

-        /// Populated from early constraint checking step only.
-        /// </summary>
-        internal readonly ImmutableArray<TypeParameterConstraintClause> OtherPartialDeclarations;
+        internal static void AdjustConstraintTypes(Symbol container, ImmutableArray<TypeParameterSymbol> typeParameters,


AdjustConstraintTypes [](start = 29, length = 21)

nit: this could probably use a comment too. What is "adjusting"? #Resolved

jcouv · 2019-05-02T22:28:18Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

                Diagnostic(ErrorCode.ERR_NullableUnconstrainedTypeParameter, "U?").WithLocation(15, 12),
+                // (17,12): error CS8627: A nullable type parameter must be known to be a value type or non-nullable reference type. Consider adding a 'class', 'struct', or type constraint.
+                //     static U?[] F3<T, U>() where T : struct where U : T => throw null!;
+                Diagnostic(ErrorCode.ERR_NullableUnconstrainedTypeParameter, "U?").WithLocation(17, 12),


Diagnostic(ErrorCode.ERR_NullableUnconstrainedTypeParameter, "U?").WithLocation(17, 12), [](start = 16, length = 88)

nit: this is a cascading diagnostic. The root cause is below ("Type parameter 'T' has the 'struct' constraint so 'T' cannot be used as a constraint for 'U'"). Might there be a simple way to suppress? #Resolved

nit: this is a cascading diagnostic. The root cause is below ("Type parameter 'T' has the 'struct' constraint so 'T' cannot be used as a constraint for 'U'"). Might there be a simple way to suppress?

Unfortunately there is no easy way to do that. We are not tracking what constraints were removed as invalid and what impact they otherwise would have if they were valid. Also, now we have a consistent experience across all usages of U?. For example, in the code below prior to this change the CS8627 would be reported only inside method body, but not inside method signature. With this change, method signature gets the same treatment.

class C1 { #nullable enable static U?[] F3<T, U>() where T : struct where U : T { U? x = default; Console.WriteLine(x); throw null!; } }

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

jcouv

LGTM Thanks (iteration 2)

AlekseyTs added 2 commits April 30, 2019 13:43

Revert two step initialization of constraints

08568e9

Closes dotnet#28834. Closes dotnet#30061.

Follow up on merge

e67532d

AlekseyTs added the Area-Compilers label Apr 30, 2019

AlekseyTs requested a review from cston April 30, 2019 20:57

AlekseyTs requested a review from a team as a code owner April 30, 2019 20:57

AlekseyTs added the Feature - Nullable Reference Types Nullable Reference Types label Apr 30, 2019

jcouv reviewed May 2, 2019

View reviewed changes

333fred reviewed May 2, 2019

View reviewed changes

jcouv reviewed May 2, 2019

View reviewed changes

cston reviewed May 2, 2019

View reviewed changes

cston approved these changes May 2, 2019

View reviewed changes

jcouv reviewed May 2, 2019

View reviewed changes

jcouv approved these changes May 2, 2019

View reviewed changes

PR feedback

d89ef33

jcouv approved these changes May 3, 2019

View reviewed changes

AlekseyTs merged commit 0088a06 into dotnet:master May 3, 2019

		@@ -300,7 +297,7 @@ internal bool IsEqualToOrDerivedFrom(TypeSymbol type, TypeCompareKind comparison
		/// You can ignore custom modifiers, ignore the distinction between object and dynamic, or ignore tuple element names differences.
		/// </param>


		internal abstract TypeSymbol GetDeducedBaseType(ConsList<TypeParameterSymbol> inProgress);

		private static bool ConstraintImpliesReferenceType(TypeSymbol constraint)

Conversation

AlekseyTs commented Apr 30, 2019

Uh oh!

AlekseyTs commented May 1, 2019 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented May 2, 2019 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 2, 2019

Choose a reason for hiding this comment

Uh oh!

jcouv May 2, 2019

Choose a reason for hiding this comment

Uh oh!

jcouv May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 2, 2019

Choose a reason for hiding this comment

Uh oh!

333fred May 2, 2019 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 2, 2019

Choose a reason for hiding this comment

Uh oh!

jcouv May 2, 2019 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 2, 2019 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 2, 2019 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 2, 2019 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 2, 2019 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 2, 2019 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv May 2, 2019 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 2, 2019

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 2, 2019

Choose a reason for hiding this comment

Uh oh!

jcouv May 2, 2019 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv May 2, 2019 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 2, 2019

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

AlekseyTs commented May 1, 2019 •

edited by jcouv

Loading

AlekseyTs commented May 2, 2019 •

edited by jcouv

Loading

jcouv May 2, 2019 •

edited

Loading

jcouv May 2, 2019 •

edited

Loading

333fred May 2, 2019 •

edited by AlekseyTs

Loading

jcouv May 2, 2019 •

edited by AlekseyTs

Loading

cston May 2, 2019 •

edited by AlekseyTs

Loading

cston May 2, 2019 •

edited by AlekseyTs

Loading

cston May 2, 2019 •

edited by AlekseyTs

Loading

cston May 2, 2019 •

edited by AlekseyTs

Loading

cston May 2, 2019 •

edited by AlekseyTs

Loading

jcouv May 2, 2019 •

edited by AlekseyTs

Loading

jcouv May 2, 2019 •

edited by AlekseyTs

Loading

jcouv May 2, 2019 •

edited by AlekseyTs

Loading