Support nullable annotations on unconstrained type parameters by cston · Pull Request #45993 · dotnet/roslyn

cston · 2020-07-15T01:20:35Z

Support ? annotations on unconstrained type parameter references:

? annotation is allowed for type parameters that are not constrained to reference types or value types
? annotation is an error when used on unconstrained type parameter with /langversion:8

Support where T : default constraint:

default constraint is allowed for method type parameters in overridden or explicitly implemented methods to disambiguate between overloads defined in the base or interface
default constraint is an error with /langversion:8
default constraint is an error when used outside of method overrides or explicit implementations
default constraint is an error when the type parameter is constrained

Fixes #29146

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

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

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

jaredpar · 2020-07-16T16:22:46Z

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

        /// T where T : IComparable? => true
        /// </summary>
-        public static bool IsTypeParameterDisallowingAnnotation(this TypeSymbol type)
+        public static bool IsTypeParameterDisallowingAnnotation(this TypeSymbol type) // The method name is misleading since annotations are allowed always.


Think we should move this comment to a <remarks> section. #Resolved

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

jaredpar · 2020-07-16T16:25:49Z

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

+        /// If this is a lazy nullable type pending resolution, forces this to be resolved.
        /// </summary>
-        public void TryForceResolveAsNullableValueType()
+        public void TryForceResolve(bool asValueTypeNotReferenceType)


Consider asValueType. #Resolved

jaredpar · 2020-07-16T16:32:48Z

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

-                ? NullableAnnotation.NotAnnotated : NullableAnnotation.Annotated;
+            if (Type is null)
+            {
+                return TypeWithAnnotations.Create(Type, NullableAnnotation.Annotated);


Suggested change

return TypeWithAnnotations.Create(Type, NullableAnnotation.Annotated);

return TypeWithAnnotations.Create(null, NullableAnnotation.Annotated);

Why do we unconditionally use NullableAnnotation.Annotated here for null types? #Resolved

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

jaredpar · 2020-07-16T16:38:17Z

src/Compilers/CSharp/Test/Symbol/SymbolDisplay/SymbolDisplayTests.cs

+            var formatWithNullableModifier = formatWithoutModifiers.AddMiscellaneousOptions(SymbolDisplayMiscellaneousOptions.IncludeNullableReferenceTypeModifier);
+            var formatWithBothModifiers = formatWithNullableModifier.AddMiscellaneousOptions(SymbolDisplayMiscellaneousOptions.IncludeNotNullableReferenceTypeModifier);
+
+            verify("C.F0", "T F0<T>()", "T F0<T>()", "T F0<T>()");


Why is the final one not T! F0<T>()? #ByDesign

The behavior of SymbolDisplayMiscellaneousOptions.IncludeNotNullableReferenceTypeModifier is unchanged. The type could be reported as T! here, and in T F8<T>() where T : notnull, independent of this PR.

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

jaredpar · 2020-07-16T21:12:47Z

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

+
+            if (Type is null)
+            {
+                return NullableAnnotation.NotAnnotated;


Why do we prefer NotAnnotated here over Oblivious? #Resolved

I've removed the explicit Type is null case so the behavior should match the current behavior which returns this.NullableAnnotation. That said, I suspect NullableAnnotation is typically NotAnnotated when Type is null since that is the default value for the struct.

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

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

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

jaredpar · 2020-07-16T21:30:33Z

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

+        [InlineData(false)]
+        [InlineData(true)]


Suggested change

[InlineData(false)]

[InlineData(true)]

[CombinatorialData]

``` #Resolved

AlekseyTs · 2020-07-17T18:16:18Z

src/Compilers/CSharp/Portable/CSharpResources.resx

  </data>
  <data name="ERR_NullableUnconstrainedTypeParameter" xml:space="preserve">
-    <value>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.</value>
+    <value>A nullable type parameter must be known to be a value type or non-nullable reference type. Please use language version '{0}' or greater, or consider adding a 'class', 'struct', or type constraint.</value>


Please use language version '{0}' [](start = 102, length = 34)

I think we should simply use regular language version message with specific feature name, etc. This way IDE will be able to suggest the change language version fix. #Closed

This message has helpful suggestions for using T? in C#8 which the general language version message would not. We can update the IDE to recognize this ErrorCode if necessary.

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

We can update the IDE to recognize this ErrorCode if necessary.

Let's do that then. I think we already have a mechanism to communicate to IDE that it should offer an upgrade to specific language version. CC @jcouv

In reply to: 456613552 [](ancestors = 456613552,456600909)

AlekseyTs · 2020-07-17T18:20:03Z

src/Compilers/CSharp/Portable/CSharpResources.resx

    <value>null pointer constant pattern</value>
  </data>
+  <data name="IDS_FeatureDefaultTypeParameterConstraint" xml:space="preserve">
+    <value>default type parameter constraints</value>


default type parameter constraints [](start = 11, length = 34)

It feels like the name of the feature isn't very clear, especially that default is used in not very common cases. For example, a user typed T? and compiler reports that "default type parameter constraints" aren't supported by the language version. What constraints the message is talking about? This isn't a constraint. #Closed

Perhaps we need another feature name for T?

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

This message is only reported for uses of where T : default. For uses of T?, we continue to report ErrorCode.ERR_NullableUnconstrainedTypeParameter.

In reply to: 456603253 [](ancestors = 456603253,456602711)

AlekseyTs · 2020-07-17T20:28:28Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

                    return _syntaxFactory.ClassOrStructConstraint(SyntaxKind.ClassConstraint, classToken, questionToken);
+                case SyntaxKind.DefaultKeyword:
+                    var defaultToken = this.EatToken();
+                    return _syntaxFactory.DefaultConstraint(defaultToken);


_syntaxFactory.DefaultConstraint(defaultToken) [](start = 27, length = 46)

Should we check language version here? #Closed

The version check is in BindTypeParameterConstraints().

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

The version check is in BindTypeParameterConstraints().

I realized that. The question is why? Usually, for simple cases like that we perform language version checks in the parser.

In reply to: 456693574 [](ancestors = 456693574,456659327)

AlekseyTs · 2020-07-17T20:29:25Z

src/Compilers/CSharp/Portable/Errors/MessageID.cs

                case MessageID.IDS_FeatureRecords:
                case MessageID.IDS_FeatureStaticAnonymousFunction: // syntax check
                case MessageID.IDS_FeatureModuleInitializers: // semantic check on method attribute
+                case MessageID.IDS_FeatureDefaultTypeParameterConstraint:


case MessageID.IDS_FeatureDefaultTypeParameterConstraint: [](start = 16, length = 57)

Consider adding a comment if this is not a parsing time check #Closed

AlekseyTs · 2020-07-17T21:15:27Z

src/Compilers/CSharp/Portable/Binder/Binder_Symbols.cs

+        {
+            if (type.Type.IsTypeParameterDisallowingAnnotation())
+            {
+                var requiredVersion = MessageID.IDS_FeatureDefaultTypeParameterConstraint.RequiredVersion();


IDS_FeatureDefaultTypeParameterConstraint [](start = 48, length = 41)

This is somewhat confusing because default constraint isn't involved. #Closed

Added a comment.

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

AlekseyTs · 2020-07-17T22:14:50Z

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs

+        private static bool IsPossiblyNullableTypeTypeParameter(in TypeWithAnnotations typeWithAnnotations)
+        {
+            var type = typeWithAnnotations.Type;
+            return type is object &&


return type is object && [](start = 12, length = 24)

It looks like we dropped IsNotAnnotated check. Is this intentional? Could you please elaborate? #Closed

It looks like we dropped IsNotAnnotated check. Is this intentional?

Yes. Callers above are explicitly checking NullableAnnotation, and requiring NullableAnnotation == NullableAnnotation.NotAnnotated here would mean T and T? would be considered identical in HasTopLevelNullabilityIdentityConversion(). For instance, that would allow assigning IEnumerable<T?> to IEnumerable<T> in UnconstrainedTypeParameter_21().

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

jaredpar · 2020-07-23T18:04:27Z

The spanish failure is in UpdaterService which is flaky. Will filean issue shortly. Merging.

…#45993)" This reverts commit cbb5cf1.

…-pointers * upstream/master: (207 commits) Update argument state when parameter has not-null type (dotnet#46072) Fix TypeWithAnnotations.ToTypeWithState() for (untyped) null literal (dotnet#46344) Update README (dotnet#46136) Revert "Revert "Support nullable annotations on unconstrained type parameters"" Revert "Support nullable annotations on unconstrained type parameters (dotnet#45993)" Fix type in publish data Update VSIXExpInstaller version to one available on ADO Update publish data for 16.8 Update version of RichCodeNav.EnvVarDump A fixed initializer must be bound to its natural type (dotnet#46293) Update features merged into 16.7p4 (dotnet#46229) Async-streams: disposal should continue without jump within a finally (dotnet#46188) Recommend default in type constraint, but not record (dotnet#46311) Add use site diagnostics to IsUnmanaged (dotnet#46114) Add another flaky test. Ensure NuGet connections use TLS 1.2 Update to Microsoft.CodeAnalysis.Testing 1.0.1-beta1.20374.2 Skip flaky test. Fix build break. (dotnet#46303) Skip a flaky test Relates to dotnet#46304 ...

…to function-pointer-type-lookup * upstream/features/function-pointers: (212 commits) Correct public API number. Update argument state when parameter has not-null type (dotnet#46072) Fix TypeWithAnnotations.ToTypeWithState() for (untyped) null literal (dotnet#46344) Update README (dotnet#46136) Revert "Revert "Support nullable annotations on unconstrained type parameters"" Revert "Support nullable annotations on unconstrained type parameters (dotnet#45993)" Fix type in publish data Update VSIXExpInstaller version to one available on ADO Update publish data for 16.8 Update version of RichCodeNav.EnvVarDump A fixed initializer must be bound to its natural type (dotnet#46293) Update features merged into 16.7p4 (dotnet#46229) Async-streams: disposal should continue without jump within a finally (dotnet#46188) Recommend default in type constraint, but not record (dotnet#46311) Add use site diagnostics to IsUnmanaged (dotnet#46114) Add another flaky test. Ensure NuGet connections use TLS 1.2 Update to Microsoft.CodeAnalysis.Testing 1.0.1-beta1.20374.2 Skip flaky test. Fix build break. (dotnet#46303) ...

mysteryx93 · 2021-04-19T15:50:50Z

Why was T? on unconstrained types implemented as meaning ValueOrDefault instead of ValueOrNull?

For ValueOrNull, we're still back to the old method of duplicating classes with struct/class constraints.

RikkiGibson · 2021-04-19T17:35:29Z

Could you please share some specific scenarios that are negatively affected by this?

mysteryx93 · 2021-04-19T18:29:55Z

The following code compiles but doesn't behave as one would expect. Previously it would fail to compile, now it just doesn't do what I'd expect it to.

public enum LogLevel { None, Debug, Info }

public interface IFormatter<T>
{
    T Parse(string? value);
    string? Format(T value);
}

public class PropertyBase<T> : IFormatter<T?>
{
    public virtual T? Parse(string? value) => default!;
    public virtual string? Format(T? value) => null;
}

public class PropertyRead<T> : PropertyBase<T>
{ }

public class PropertyReadWrite<T> : PropertyRead<T>
{ }

public class PropertyEnum<T> : PropertyRead<T>
    where T : struct, Enum
{ }

public class MyClass
{
    public void DoIt()
    {
        var f = new PropertyEnum<LogLevel>();
        var value = f.Parse(null); // non-nullable!
    }
}

The solution is to write it like this

public enum LogLevel { None, Debug, Info }

public interface IFormatter<T>
{
    T Parse(string? value);
    string? Format(T value);
}

public class PropertyBase<TNull> : IFormatter<TNull>
{
    public virtual TNull Parse(string? value) => default!;
    public virtual string? Format(TNull value) => null;
}

public class PropertyRead<T> : PropertyBase<T?>
    where T : struct
{ }

public class PropertyReadRef<T> : PropertyBase<T?>
    where T : class
{ }

public class PropertyReadWrite<T> : PropertyRead<T>
    where T : struct
{ }

public class PropertyReadWriteRef<T> : PropertyReadRef<T>
    where T : class
{ }

public class PropertyEnum<T> : PropertyRead<T>
    where T : struct, Enum
{ }

public class MyClass
{
    public void DoIt()
    {
        var f = new PropertyEnum<LogLevel>();
        var value = f.Parse(null); // nullable!
    }
}

This solution however requires a lot of code copy/paste, and inheritance references can get complicated, especially when the classes contain considerable code.

And the syntax is weird for string: PropertyReadRef<string>, which leads me to creating a shortcut class PropertyReadString

cston force-pushed the default-constraint branch 4 times, most recently from c22cd86 to eb3bf46 Compare July 16, 2020 00:22

Support nullable annotations on unconstrained type parameters

378b787

cston force-pushed the default-constraint branch from 4b2e7a2 to 378b787 Compare July 16, 2020 05:41

cston marked this pull request as ready for review July 16, 2020 05:43

cston requested a review from a team as a code owner July 16, 2020 05:43

Fix test

0c6ee3e

sharwell reviewed Jul 16, 2020

View reviewed changes

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_Nullable.cs Show resolved Hide resolved

sharwell reviewed Jul 16, 2020

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/UninitializedNonNullableFieldTests.cs Show resolved Hide resolved

jaredpar reviewed Jul 16, 2020

View reviewed changes

cston added 4 commits July 16, 2020 10:21

PR feedback

0b226df

Misc.

f90b6dc

Misc.

fdb9761

Merge remote-tracking branch 'upstream/master' into default-constraint

75ad5d2

jaredpar reviewed Jul 16, 2020

View reviewed changes

Misc.

45f25fa

cston requested a review from a team July 17, 2020 16:02

cston added the Area-Compilers label Jul 17, 2020

AlekseyTs reviewed Jul 17, 2020

View reviewed changes

cston added 2 commits July 17, 2020 11:26

Merge remote-tracking branch 'upstream/master' into default-constraint

4335656

[MaybeNull] tests

0d99acd

AlekseyTs reviewed Jul 17, 2020

View reviewed changes

Update error message

5bf7059

AlekseyTs reviewed Jul 17, 2020

View reviewed changes

Misc.

114b1f1

AlekseyTs reviewed Jul 17, 2020

View reviewed changes

Formatting

6fff28c

cston force-pushed the default-constraint branch from b9d17f3 to 6fff28c Compare July 23, 2020 06:18

jaredpar approved these changes Jul 23, 2020

View reviewed changes

Fix merge conflict

280bdf4

cston force-pushed the default-constraint branch 2 times, most recently from 440cb3b to 280bdf4 Compare July 23, 2020 07:15

Fix merge conflict

ef0908c

jaredpar merged commit cbb5cf1 into dotnet:release/dev16.8-preview1 Jul 23, 2020

jaredpar mentioned this pull request Jul 23, 2020

The UpdaterService test is flaky #46255

Closed

cston mentioned this pull request Jul 23, 2020

Mechanism to annotate unconstrained type parameters #29146

Closed

cston deleted the default-constraint branch July 23, 2020 18:09

cston mentioned this pull request Jul 23, 2020

Add IDS_FeatureDefaultTypeParameterConstraint #46247

Closed

stephentoub mentioned this pull request Jul 24, 2020

Rollout T? on unconstrained type parameters dotnet/runtime#39877

Closed

JamesNK mentioned this pull request Jul 26, 2020

Add nullable annotations to Authentication.Core & Authentication.Cookies dotnet/aspnetcore#24307

Merged

This was referenced Jul 27, 2020

"CS8603: Possible null reference return" reported for null from lambda that returns T? where T : class #46342

Closed

Fix TypeWithAnnotations.ToTypeWithState() for (untyped) null literal #46344

Merged

RikkiGibson added a commit that referenced this pull request Jul 27, 2020

Revert "Support nullable annotations on unconstrained type parameters (…

0aa5e14

…#45993)" This reverts commit cbb5cf1.

This was referenced Jul 27, 2020

Revert "Support nullable annotations on unconstrained type parameters" #46358

Merged

Revert 46358 revert 45993 default constraint #46360

Merged

kevinchalet mentioned this pull request Jul 28, 2020

Use T? on unconstrained type parameters when migrating to C# 9 openiddict/openiddict-core#1036

Closed

cston mentioned this pull request Jul 29, 2020

Type argument nullability warning no longer reported for type parameter constrained to 'int?' #46410

Closed

This was referenced Jul 31, 2020

Annotate EF Core for nullable reference types dotnet/efcore#19007

Closed

Return values of SingleOrDefaultAsync incorrect when nullable reference types are enabled dotnet/efcore#21793

Closed

cston mentioned this pull request Sep 28, 2020

Unconstrained type parameter annotation proposal dotnet/csharplang#3035

Closed

	return TypeWithAnnotations.Create(Type, NullableAnnotation.Annotated);
	return TypeWithAnnotations.Create(null, NullableAnnotation.Annotated);

Conversation

cston commented Jul 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jaredpar Jul 16, 2020 • 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!

Uh oh!

jaredpar Jul 16, 2020 • 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!

jaredpar Jul 16, 2020 • 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!

Uh oh!

jaredpar Jul 16, 2020 • 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!

jaredpar Jul 16, 2020 • 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!

Uh oh!

Uh oh!

jaredpar Jul 16, 2020 • 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!

AlekseyTs Jul 17, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jul 17, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jul 17, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jul 17, 2020 • 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 Jul 17, 2020 • 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!

AlekseyTs Jul 17, 2020 • 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!

jaredpar commented Jul 23, 2020

cston commented Jul 15, 2020 •

edited

Loading

jaredpar Jul 16, 2020 •

edited by cston

Loading

jaredpar Jul 16, 2020 •

edited by cston

Loading

jaredpar Jul 16, 2020 •

edited by cston

Loading

jaredpar Jul 16, 2020 •

edited by cston

Loading

jaredpar Jul 16, 2020 •

edited by cston

Loading

jaredpar Jul 16, 2020 •

edited by cston

Loading

AlekseyTs Jul 17, 2020 •

edited

Loading

AlekseyTs Jul 17, 2020 •

edited

Loading

AlekseyTs Jul 17, 2020 •

edited

Loading

AlekseyTs Jul 17, 2020 •

edited

Loading

AlekseyTs Jul 17, 2020 •

edited

Loading

AlekseyTs Jul 17, 2020 •

edited

Loading

mysteryx93 commented Apr 19, 2021 •

edited

Loading