Remove support for explicit `object` generic type constraints by AlekseyTs · Pull Request #36269 · dotnet/roslyn

AlekseyTs · 2019-06-10T05:21:41Z

AlekseyTs · 2019-06-10T16:48:05Z

@dotnet/roslyn-compiler, @cston, @jcouv Please review.

jcouv · 2019-06-10T17:42:04Z

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

                        }
+                        else
+                        {
+                            result[mergeWith] = ConstraintsHelper.MergeTopLevelNullabilityForConstraint(result[mergeWith], substituted);


MergeTopLevelNullabilityForConstraint [](start = 66, length = 37)

I didn't understand why we need to merge. The map uses the strictest type and annotation comparison, so if map.TryGetValue found an existing substituted type I would think they are identical already. #Closed

jcouv · 2019-06-10T18:06:34Z

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

+            }
+        }
+
+        internal static bool IsObjectConstraint(TypeWithAnnotations type, ref TypeWithAnnotations possiblySignificantObjectConstraint)


possiblySignificantObjectConstraint [](start = 98, length = 35)

nit: consider naming bestObjectConstraint as in caller. Or maybe mergedObjectConstraint both here and in caller? #Closed

jcouv · 2019-06-10T18:13:02Z

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

                conversions.HasBoxingConversion(deducedBase, effectiveBase, ref useSiteDiagnostics));
        }
+
+        internal static TypeWithAnnotations MergeTopLevelNullabilityForConstraint(TypeWithAnnotations type1, TypeWithAnnotations type2)


Merge [](start = 44, length = 5)

nit: "Merge" did throw me off a bit because it has many connotations (Merge/Join in flow analysis). Maybe StrictestTopLevelNullabilityConstraint? #Closed

jcouv · 2019-06-10T18:14:35Z

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

+
+                    if (bestObjectConstraint.HasType)
+                    {
+                        constraintTypes.Insert(0, bestObjectConstraint);


.Insert(0, [](start = 39, length = 11)

nit: why insert at index 0 rather than just append with .Add? #Closed

why insert at index 0 rather than just append with .Add?

Having significant constraint at the beginning is likely to make following calculations faster - we might get the answer from the first item and stop.

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

jcouv · 2019-06-10T18:19:00Z

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

        }

-        internal static bool? IsNotNullableIfReferenceTypeFromConstraintType(TypeWithAnnotations constraintType)
+        internal static bool? IsNotNullableIfReferenceTypeFromConstraintType(TypeWithAnnotations constraintType, out bool isNonNullableValueType)


bool? [](start = 24, length = 5)

I don't know if it's worth the effort to refactor, but it feels like we're trying to return a Nullable<NullableAnnotation> (3 possible annotations when applicable + 1 case for not applicable) #Closed

I don't know if it's worth the effort to refactor, but it feels like we're trying to return a Nullable (3 possible annotations when applicable + 1 case for not applicable)

The returned value is applicable in all cases. It is convenient to return an extra information that we already calculated here and avoid recalculating it again.

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

jcouv · 2019-06-10T18:36:40Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PETypeParameterSymbol.cs

-                            {
-                                continue;
-                            }
+                            // Droped 'System.ValueType' constraint type when the 'valuetype' constraint was also specified.


Droped [](start = 31, length = 6)

nit: typo "Drop" or "Dropped" #Closed

cston · 2019-06-10T18:37:38Z

docs/features/nullable-reference-types.md

 [4/25/18](https://github.com/dotnet/csharplang/blob/master/meetings/2018/LDM-2018-04-25.md)

-An explicit `object` (or `System.Object`) constraint is allowed, which requires the type to be non-nullable (value or reference type).
+An explicit `object` (or `System.Object`) constraints of any nullability are disallowed. However, type substitution can lead to


An explicit [](start = 0, length = 11)

Consider removing "an" to match "constraints ... are". #Closed

jcouv · 2019-06-10T18:42:49Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PETypeParameterSymbol.cs

+            return true;
+        }
+
+        private bool? CalculateIsNotNullableIfReferenceType(ConsList<PETypeParameterSymbol> inProgress, out bool isNonNullableValueType)


CalculateIsNotNullableIfReferenceType [](start = 22, length = 37)

nit: consider making this a local function (single use in IsNotNullableIfReferenceType property as far as I can tell) #Closed

consider making this a local function (single use in IsNotNullableIfReferenceType property as far as I can tell)

I find it clearer to have it as an instance method because it operates on a different instance than its caller.

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

jcouv · 2019-06-10T18:55:13Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PETypeParameterSymbol.cs

+                return true;
+            }
+
+            bool? fromNonTypeConstraints = CalculateIsNotNullableFromNonTypeConstraints();


fromNonTypeConstraints [](start = 18, length = 22)

I don't know if it's worth optimizing, but if we have a ! from non-type constraints, we should be able to return without looking at type constraints. #Closed

I don't know if it's worth optimizing, but if we have a ! from non-type constraints, we should be able to return without looking at type constraints.

We need to calculate isNonNullableValueType, only then we can return

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

jcouv · 2019-06-10T18:59:43Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PETypeParameterSymbol.cs

+                return true;
+            }
+
+            if (fromTypes == true || fromNonTypeConstraints == false)


if (fromTypes == true || fromNonTypeConstraints == false) [](start = 12, length = 57)

nit: It feels like we're implementing the "strictest top-level nullability from constraints" here again. Might there be a way to factor that out (share with existing implementation)?
Maybe this comes back to whether we're dealing with pairs of bool?, NullableAnnotations or TypeWithAnnotations... Aligning on one (possibly NullableAnnotations) might make things simpler and less custom. #WontFix

jcouv · 2019-06-10T19:00:59Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PETypeParameterSymbol.cs

+                    return true;
+                }
+
+                if (fromType == true)


if (fromType == true) [](start = 16, length = 21)

nit: same comment here (having a method to pick strictest from two bool? or maybe dealing with NullableAnnotation) #WontFix

jcouv

LGTM Thanks (iteration 2)

cston · 2019-06-10T19:24:03Z

            // - IsUnmanagedAttribute is allowed iif there is an unmanaged pattern

"iff"? #WontFix

Refers to: src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PETypeParameterSymbol.cs:223 in f960134. [](commit_id = f960134, deletion_comment = False)

AlekseyTs · 2019-06-10T19:35:15Z

            // - IsUnmanagedAttribute is allowed iif there is an unmanaged pattern

"iff"?

This comment is not part of the changes.

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

Refers to: src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PETypeParameterSymbol.cs:223 in f960134. [](commit_id = f960134, deletion_comment = False)

jcouv · 2019-06-10T20:03:59Z

            // - IsUnmanagedAttribute is allowed iif there is an unmanaged pattern

means "if and only if"

In reply to: 500563221 [](ancestors = 500563221,500559705)

Refers to: src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PETypeParameterSymbol.cs:223 in f960134. [](commit_id = f960134, deletion_comment = False)

cston · 2019-06-10T20:13:17Z

    private static SymbolDisplayFormat TestFormatWithConstraintsAndNonNullableTypeModifier =>

Consider changing SymbolDisplayFormat.TestFormatWithConstraints. (Are there any uses of that format other than here?) #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:61215 in f960134. [](commit_id = f960134, deletion_comment = False)

jcouv · 2019-06-10T23:23:56Z

We'll hold onto this PR until we get a greenlight from Stephen (ie. after they adopt the compiler that introduced notnull and made object constraint a warning).

jcouv · 2019-06-13T17:02:19Z

Santiago confirmed that all instances of the object constraint have been removed from the BCL. We can go ahead and merge this PR.
Removed the "blocked" label.

Remove support for explicit object generic type constraints

636a721

AlekseyTs added Language-C# Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Jun 10, 2019

AlekseyTs requested a review from a team as a code owner June 10, 2019 05:21

jcouv reviewed Jun 10, 2019

View reviewed changes

cston reviewed Jun 10, 2019

View reviewed changes

jcouv reviewed Jun 10, 2019

View reviewed changes

PR feedback

f960134

jcouv approved these changes Jun 10, 2019

View reviewed changes

jcouv modified the milestones: 16.3, 16.2 Jun 10, 2019

cston approved these changes Jun 10, 2019

View reviewed changes

AlekseyTs added 2 commits June 10, 2019 13:46

PR feedback

e46e1e0

PR feedback

0b794e9

jcouv self-assigned this Jun 10, 2019

jcouv added the Blocked label Jun 10, 2019

AlekseyTs added 2 commits June 10, 2019 22:20

Add tests

44c98e3

Merge remote-tracking branch 'dotnet/master' into NotNull_02

02180ea

AlekseyTs mentioned this pull request Jun 12, 2019

Replace dynamic with object when substituting constraints. #36379

Merged

jcouv removed the Blocked label Jun 13, 2019

AlekseyTs merged commit f54c086 into dotnet:master Jun 13, 2019

jaredpar mentioned this pull request Jun 13, 2019

Nullable Reference Type Changes #35816

Closed

23 tasks

Conversation

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

Uh oh!

Uh oh!

AlekseyTs commented Jun 10, 2019

Uh oh!

jcouv Jun 10, 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!

jcouv Jun 10, 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!

jcouv Jun 10, 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!

jcouv Jun 10, 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 Jun 10, 2019

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 10, 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 Jun 10, 2019

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 10, 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!

cston Jun 10, 2019 • edited by jcouv 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 Jun 10, 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 Jun 10, 2019

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 10, 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 Jun 10, 2019

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 10, 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 Jun 10, 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 left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

AlekseyTs commented Jun 10, 2019

Uh oh!

jcouv commented Jun 10, 2019

Uh oh!

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

Uh oh!

Uh oh!

jcouv commented Jun 10, 2019

Uh oh!

jcouv commented Jun 13, 2019

Uh oh!

Reviewers

AlekseyTs commented Jun 10, 2019 •

edited by jcouv

Loading

jcouv Jun 10, 2019 •

edited

Loading

jcouv Jun 10, 2019 •

edited

Loading

jcouv Jun 10, 2019 •

edited

Loading

jcouv Jun 10, 2019 •

edited

Loading

jcouv Jun 10, 2019 •

edited

Loading

jcouv Jun 10, 2019 •

edited

Loading

cston Jun 10, 2019 •

edited by jcouv

Loading

jcouv Jun 10, 2019 •

edited

Loading

jcouv Jun 10, 2019 •

edited

Loading

jcouv Jun 10, 2019 •

edited by AlekseyTs

Loading

jcouv Jun 10, 2019 •

edited by AlekseyTs

Loading

cston commented Jun 10, 2019 •

edited by AlekseyTs

Loading

cston commented Jun 10, 2019 •

edited by AlekseyTs

Loading