Handle T? constraint by cston · Pull Request #27525 · dotnet/roslyn

cston · 2018-06-06T17:51:27Z

Binding of constraints is now split into two steps: early and late.

In the early step, constraint types are bound but with no additional checks. In the late step, constraint types are checked for validity and duplicates, and type parameter "bounds" (EffectiveBase and EffectiveInterface) are calculated.

Most TypeParameter properties simply ensure both steps have completed (as before), but IsValueType and IsReferenceType now only ensure the early step has completed.

Fixes #27289.

cston · 2018-06-07T22:24:22Z

@dotnet/roslyn-compiler please review. #Resolved

…s' into 27289

cston · 2018-06-12T15:46:58Z

@dotnet/roslyn-compiler please review. #Resolved

AlekseyTs · 2018-06-12T17:32:28Z

@cston It looks like there are legitimate test failures #Resolved

…s' into 27289

cston · 2018-06-13T16:56:05Z

@dotnet/roslyn-compiler please review, thanks.

jcouv · 2018-06-14T17:47:05Z

docs/features/nullable-reference-types.md

 _Describe the set of tests that affect flow state._

+## `default`
+`default(T)` is `T?` if `T` is a reference type. _Should `default(T?)` be an error?_


reference type [](start = 33, length = 14)

It may be good to explain the meaning of T?, especially for unconstrained T.
Also, the phrasing may need to be adjusted "is a reference or unconstrained type".

Tangent: the null-state of null and the null-state of an instantiation (new) are trivial.

This spec change is unrelated to the PR. It may be better to have a separate PR for the spec change. #Closed

Added a question about unconstrained type parameters.

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

jcouv · 2018-06-14T18:27:10Z

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


-        internal static ImmutableArray<TypeParameterConstraintClause> MakeTypeParameterConstraints(
-            this SourceMethodSymbol containingSymbol,
+        internal static ImmutableArray<TypeParameterConstraintClause> MakeTypeParameterConstraintsEarly(


MakeTypeParameterConstraintsEarly [](start = 70, length = 33)

Please add comments to clarify the purpose and rules around "early" vs. "late". #Closed

jcouv · 2018-06-14T19:07:00Z

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

        /// If a type parameter does not have constraints, the corresponding entry in the array is null.
        /// </summary>
-        public abstract ImmutableArray<TypeParameterConstraintClause> TypeParameterConstraintClauses { get; }
+        public abstract ImmutableArray<TypeParameterConstraintClause> GetTypeParameterConstraintClauses(bool early);


GetTypeParameterConstraintClauses [](start = 70, length = 33)

Please comment on early parameter #Closed

jcouv · 2018-06-14T19:14:34Z

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

+        internal ImmutableArray<TypeSymbolWithAnnotations> GetConstraintTypesNoUseSiteDiagnostics(ConsList<TypeParameterSymbol> inProgress, bool early)
        {
-            get
+            this.EnsureAllConstraintsAreResolved(early: true);


EnsureAllConstraintsAreResolved [](start = 17, length = 31)

Is there a difference with just calling EnsureAllConstraintsAreResolved(early)? I would have assumed that calling with early : false would just do a superset of the work that calling with early : true does. #Closed

The work for early: false is a superset of the early: true, but the early work should be completed for all type parameters before the late work is started for any type parameter. This split (two separate calls), needs to be done somewhere, and it was simpler to do here, in the caller of EnsuresAllConstraintsAreResolved rather than in several implementations of that abstract method. (This should be the only caller of that method that support early: false.)

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

Thanks. Probably worth a comment #Closed

jcouv · 2018-06-14T19:15:59Z

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

+            return this.GetConstraintTypes(inProgress, early);
        }

+        internal ImmutableArray<TypeSymbolWithAnnotations> ConstraintTypesNoUseSiteDiagnostics => GetConstraintTypesNoUseSiteDiagnostics(ConsList<TypeParameterSymbol>.Empty, early: false);


=> [](start = 94, length = 3)

Nit: consider breaking the line before => #Closed

jcouv · 2018-06-14T19:16:29Z

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

        /// order the callers query individual type parameters.
        /// </summary>
-        internal abstract void EnsureAllConstraintsAreResolved();
+        internal abstract void EnsureAllConstraintsAreResolved(bool early);


Please document early parameter #Closed

jcouv · 2018-06-14T19:24:31Z

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

+            {
+                return true;
+            }
+            return IsReferenceTypeFromConstraintTypes(this.GetConstraintTypesNoUseSiteDiagnostics(inProgress, early: true), inProgress);


early [](start = 110, length = 5)

Maybe early should be renamed to minValidation or limitedChecks? #Closed

Perhaps, although early is the term used to differentiate phases of attribute binding so I re-used the term here.

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

cston · 2018-06-25T15:24:41Z

@dotnet/roslyn-compiler please review.

jcouv · 2018-06-25T20:04:46Z

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

+                var constraintTypes = currentBounds.ConstraintTypes;
+                if (!ContainingModule.UtilizesNullableReferenceTypes)
+                {
+                    constraintTypes = constraintTypes.SelectAsArray(t => t.SetUnknownNullabilityForReferenceTypes());


SetUnknownNullabilityForReferenceTypes [](start = 75, length = 38)

Can you add a PROTOTYPE marker: I suspect this will need to be revised in the presence of [NonNullTypes] in relevant scope. #Resolved

jcouv · 2018-06-25T20:09:12Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_Nullable.cs

+        new C<A>();
+    }
+}";
+            var comp2 = CreateCompilation(source2, parseOptions: TestOptions.Regular8, references: new[] { comp.EmitToImageReference() });


CreateCompilation [](start = 24, length = 17)

Could you also verify diagnostics when compiling soure+source2? This also applies to other tests below. #Resolved

jcouv · 2018-06-25T20:10:14Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_Nullable.cs

+{
+    static void Main()
+    {
+        new B<A?>();


B<A?> [](start = 12, length = 5)

I'd be tempted to also test permutations with a type derived from A, such as new B<AChild?>();, and so on. #Resolved

jcouv · 2018-06-25T20:12:49Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_Nullable.cs

+        }
+
+        [Fact]
+        public void EmitAttribute_Constraint_Oblivious()


EmitAttribute_Constraint_Oblivious [](start = 20, length = 34)

Please add a PROTOTYPE marker to test this with [NonNullTypes]. #Resolved

jcouv · 2018-06-25T20:19:21Z

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

+        }
+
+        [Fact]
+        public void ConstraintErrorMultiplePartialDeclarations_01()


ConstraintErrorMultiplePartialDeclarations_01 [](start = 20, length = 45)

Maybe test partial declarations with differences in ?. For instance, one with an I constraint, and the other with an I? constraint. #Resolved

See StaticNullChecking.PartialClassConstraintMismatch.

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

jcouv · 2018-06-25T20:29:14Z

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

+                // (5,9): error CS0454: Circular constraint dependency involving 'T' and 'T'
+                // class E<T> where T : A, T? { }
+                Diagnostic(ErrorCode.ERR_CircularConstraint, "T").WithArguments("T", "T").WithLocation(5, 9));
+        }


} [](start = 8, length = 1)

Is there any interesting scenario with nullability and possible unification on type arguments?
For instance, in where T : I<int>, I<U> we'll complain that U might unify with int.
But in where T : I<int>, I<U?> or where T : I<int?>, I<U?>, I think they cannot unify. #Resolved

Added tests.

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

jcouv · 2018-06-25T20:48:19Z

src/Compilers/Test/Utilities/CSharp/SymbolUtilities.cs

        }

-        public static string ToTestDisplayString(this SymbolWithAnnotations symbol)
+        public static string ToTestDisplayString(this SymbolWithAnnotations symbol, bool includeNonNullable = false)


ToTestDisplayString [](start = 29, length = 19)

Should this new extension method be moved to the "right" place (see comment on method above)? #ByDesign

It looks like there are more callers of the extension method above than the extension method referenced in the comment. Perhaps the extension method in the comment should be replaced instead.

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

jcouv · 2018-06-25T20:52:33Z

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

+class E<T> where T : A, T? { }";
+            var comp = CreateCompilation(source, parseOptions: TestOptions.Regular8);
+            comp.VerifyDiagnostics(
+                // (4,30): error CS0701: 'T?' is not a valid constraint. A type used as a constraint must be an interface, a non-sealed class or a type parameter.


error CS0701: 'T?' is not a valid constraint. A type used as a constraint must be an interface, a non-sealed class or a type parameter. [](start = 27, length = 135)

Should this diagnostic message be updated? #ByDesign

jcouv · 2018-06-25T21:11:03Z

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

+class C<V> where V : V?, V? { }";
+            var comp = CreateCompilation(source, parseOptions: TestOptions.Regular8);
+            comp.VerifyDiagnostics(
+                // (3,26): error CS0405: Duplicate constraint 'V' for type parameter 'V'


V [](start = 63, length = 1)

Should we say "Duplicate constraint 'V?'..." instead? #Pending

Yes, top-level nullability should be included in the message. This requires allowing TypeSymbolWithAnnotations as a Diagnostic argument. I have that change in the next PR.

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

jcouv · 2018-06-25T21:12:09Z

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

+                // (2,9): error CS0454: Circular constraint dependency involving 'U' and 'U'
+                // class B<U> where U : U?, U { }
+                Diagnostic(ErrorCode.ERR_CircularConstraint, "U").WithArguments("U", "U").WithLocation(2, 9));
+        }


} [](start = 8, length = 1)

I don't remember the LDM decision on whether top-level nullability constraints need to agree. For instance, where T : I?, U where U : class. #Resolved

We're not checking nullability of constraints currently, but according to the notes from 4/25, nullability of constraints should agree. nullable-reference-types.md contains a reference to those notes for now.

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

jcouv · 2018-06-25T21:15:57Z

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

+                // (8,40): error CS0701: 'T?' is not a valid constraint. A type used as a constraint must be an interface, a non-sealed class or a type parameter.
+                //         void F3<T>() where T : struct, T? { }
+                Diagnostic(ErrorCode.ERR_BadBoundType, "T?").WithArguments("T?").WithLocation(8, 40),
+                // (9,32): error CS0246: The type or namespace name 'A' could not be found (are you missing a using directive or an assembly reference?)


The type or namespace name 'A' [](start = 41, length = 30)

Is the error on missing type A intentional? #Resolved

No, not intentional.

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

jcouv · 2018-06-25T21:17:15Z

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

+        void F3<T>() where T : struct, T? { }
+        void F4<T>() where T : A, T? { }
+        void F5<T, U>() where U : T? { }
+        void F6<T, U>() where T : class where U : T? { }


F6 [](start = 13, length = 2)

It would be good to test the instantiations on F6 and F7 if they aren't already covered elsewhere.
Same in some tests below (like NullableTInConstraint_06).

Update: I think you're intentionally keeping that for a follow-up PR, which makes sense. #Resolved

jcouv · 2018-06-25T21:29:21Z

docs/features/nullable-reference-types.md

 ### Null tests
 _Describe the set of tests that affect flow state._

+## `default`


Please update the feature doc to explain T? constraints. #Resolved

jcouv

LGTM with some test suggestions (iteration 17).

…s' into 27289

333fred · 2018-06-27T23:10:09Z

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

+                    _syntax.ConstraintClauses,
+                    _syntax.Identifier.GetLocation(),
+                    diagnostics);
+                lock (_declarationDiagnostics)


Consider another check to _lazyTypeParameterConstraints.IsDefault, as MakeTypeParameterConstraintsEarly is not a small function.

If _lazyTypeParameterConstraints.IsDefault fails, another thread has already updated the field so there shouldn't be any lock contention from with that thread.

Will leave as is unless there are other concerns.

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

333fred · 2018-06-27T23:15:02Z

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

+            {
+                // Late step.
+                var diagnostics = DiagnosticBag.GetInstance();
+                var constraints = this.MakeTypeParameterConstraintsLate(TypeParameters, clauses, diagnostics);


Same suggestion on the check to _lazyTypeParameterConstraints.IsEarly() to avoid unnecessary lock contention.

333fred · 2018-06-27T23:24:58Z

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


-            var clauses = _lazyTypeParameterConstraints;
-            return (clauses.Length > 0) ? clauses[ordinal] : null;
+            return (clauses.Length > 0) ? clauses[ordinal] : TypeParameterConstraintClause.Empty;


We should probably add a comment to _lazyTypeParameterConstraints that multiple accesses like this are only safe if you copy it to a local variable first, as it's now a multi-stage initialization that could change between the first and second accesses. Same goes for the rest of the multi-stage initialization changes in this PR. #Resolved

333fred

LGTM (commit 21), with some suggestions for followup. I am slightly concerned that future changes might introduce subtle threading issues with the double initialization strategy we're using, so I'd like us to at least comment about accessing those double-initialized fields now.

cston requested a review from a team as a code owner June 6, 2018 17:51

Handle T? constraint

6360312

cston force-pushed the 27289 branch from 08e3679 to 6360312 Compare June 7, 2018 22:21

jcouv added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Jun 7, 2018

jcouv added this to the 16.0 milestone Jun 7, 2018

cston added 4 commits June 7, 2018 21:47

Fix tests

0607946

Revert change

48fae1e

Merge remote-tracking branch 'upstream/features/NullableReferenceType…

6307ce8

…s' into 27289

Fix tests

410ff5b

cston added 9 commits June 12, 2018 11:43

Merge remote-tracking branch 'upstream/features/NullableReferenceType…

a97e1eb

…s' into 27289

Fix test

3caa1e9

Merge remote-tracking branch 'upstream/features/NullableReferenceType…

20f1f16

…s' into 27289

Fix merge

69b589c

Misc.

2222212

Round-trip top-level nullability of constraint types

8b2ea02

Merge remote-tracking branch 'upstream/features/NullableReferenceType…

4280e1c

…s' into 27289

Add default

6213466

Merge remote-tracking branch 'upstream/features/NullableReferenceType…

5e0c0b9

…s' into 27289

jcouv reviewed Jun 14, 2018

View reviewed changes

Another cycle

eea8c78

jcouv reviewed Jun 25, 2018

View reviewed changes

jcouv approved these changes Jun 25, 2018

View reviewed changes

jcouv self-assigned this Jun 25, 2018

cston added 4 commits June 25, 2018 16:35

PR feedback

2979be0

Update doc

c1e0257

Update doc

1113239

Merge remote-tracking branch 'upstream/features/NullableReferenceType…

2f139fa

…s' into 27289

333fred reviewed Jun 27, 2018

View reviewed changes

333fred approved these changes Jun 27, 2018

View reviewed changes

PR feedback

7a5f47d

cston merged commit a87a613 into dotnet:features/NullableReferenceTypes Jun 28, 2018

cston deleted the 27289 branch June 28, 2018 05:49

cston mentioned this pull request Jul 25, 2018

Revert two step initialization of constraints and base type #28834

Closed

AlekseyTs mentioned this pull request May 2, 2019

Revert two step initialization of constraints #35375

Merged

Conversation

cston commented Jun 6, 2018

Uh oh!

cston commented Jun 7, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston commented Jun 12, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

cston commented Jun 13, 2018

Uh oh!

jcouv Jun 14, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 14, 2018 • 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 14, 2018 • 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 14, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 14, 2018 • 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 14, 2018 • 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 14, 2018 • 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 14, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

cston commented Jun 25, 2018

Uh oh!

jcouv Jun 25, 2018 • edited by cston 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 25, 2018 • edited by cston 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 25, 2018 • edited by cston 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 25, 2018 • edited by cston 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 25, 2018 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 25, 2018 • edited by cston 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 commented Jun 7, 2018 •

edited by jcouv

Loading

cston commented Jun 12, 2018 •

edited by jcouv

Loading

AlekseyTs commented Jun 12, 2018 •

edited by jcouv

Loading

jcouv Jun 14, 2018 •

edited

Loading

jcouv Jun 14, 2018 •

edited

Loading

jcouv Jun 14, 2018 •

edited

Loading

jcouv Jun 14, 2018 •

edited

Loading

jcouv Jun 14, 2018 •

edited

Loading

jcouv Jun 14, 2018 •

edited

Loading

jcouv Jun 14, 2018 •

edited

Loading

jcouv Jun 14, 2018 •

edited

Loading

jcouv Jun 25, 2018 •

edited by cston

Loading

jcouv Jun 25, 2018 •

edited by cston

Loading

jcouv Jun 25, 2018 •

edited by cston

Loading

jcouv Jun 25, 2018 •

edited by cston

Loading

jcouv Jun 25, 2018 •

edited by cston

Loading

jcouv Jun 25, 2018 •

edited by cston

Loading

jcouv Jun 25, 2018 •

edited by cston

Loading

jcouv Jun 25, 2018 •

edited by cston

Loading

jcouv Jun 25, 2018 •

edited by cston

Loading

jcouv Jun 25, 2018 •

edited by cston

Loading

jcouv Jun 25, 2018 •

edited by cston

Loading

jcouv Jun 25, 2018 •

edited

Loading

jcouv Jun 25, 2018 •

edited by cston

Loading

jcouv left a comment •

edited

Loading

cston Jun 28, 2018 •

edited

Loading

333fred Jun 27, 2018 •

edited by cston

Loading