Nullable conversion of collection expressions #69852

jcouv · 2023-09-07T21:32:09Z

Closes #69447
Relates to test plan #66418

CyrusNajmabadi · 2023-09-08T01:29:47Z

src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs

+{
+    static MyCollection<int>? M()
+    {
+        return (MyCollection<int>?)[1, 2, 3];


can you have a test with (MyCollection<byte>?)[1, 2, 3]; #Resolved

cston · 2023-09-08T16:27:03Z

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

@@ -541,6 +543,14 @@ void checkConstraintLanguageVersionAndRuntimeSupportForConversion(SyntaxNode syn
            Conversion conversion,
            BindingDiagnosticBag diagnostics)
        {
+            if (targetType.IsNullableType())
+            {
+                Debug.Assert(conversion.IsNullable);


Debug.Assert(conversion.IsNullable);

Is this true for int? x = [1, 2, 3];? #Resolved

Good catch. Thanks

cston · 2023-09-08T17:06:17Z

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

+        private static bool IsSystemNullable(TypeSymbol type, [NotNullWhen(true)] out TypeSymbol? underlyingType)
+        {
+            if (type is NamedTypeSymbol nt
+                && nt.OriginalDefinition.GetSpecialTypeSafe() == SpecialType.System_Nullable_T)


It looks like we can use the SpecialType property directly. Consider combining all and using a pattern:

type is NamedTypeSymbol { OriginalDefinition.SpecialType: SpecialType.System_Nullable_T, TypeArgumentsWithAnnotationsNoUseSiteDiagnostics: [var typeArg] } ``` #Resolved

Actually, given that this method is used for several existing callers that were using GetSpecialTypeSafe(), perhaps it makes sense to keep as GetSpecialTypeSafe() to reduce risk.

cston · 2023-09-08T17:40:55Z

src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs

@@ -4705,10 +4790,27 @@ class Program
                }


}

Consider updating the test to execute these methods, and perhaps additional methods with values, similar to TypeParameter_01. It will require adding a List<T> _list; field to S<T>.

static T Create3<T, U>(U x, U y) where T : struct, I<U> => [x, y]; static T? Create4<T, U>(U x, U y) where T : struct, I<U> => [x, y]; ``` #Resolved

jcouv · 2023-09-08T17:41:52Z

src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs

+
+            var verifier = CompileAndVerify(comp, expectedOutput: IncludeExpectedOutput("[1, 2, 3],"), verify: Verification.Fails);
+
+            // ILVerify:


@cston I found this surprising. Let's discuss offline #Resolved

Consider removing or slimming down this comment.

Filed #69860 to track refreshing ILVerify bits and filing follow-up runtime repo issue if needed.

jcouv · 2023-09-08T18:07:31Z

@333fred @RikkiGibson for another review. Thanks

333fred

Done review pass

333fred · 2023-09-08T18:47:16Z

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

+                var underlyingConversion = GetCollectionExpressionConversion(collectionExpression, underlyingDestination, ref useSiteInfo);
+                if (underlyingConversion.Exists)
+                {
+                    return new Conversion(ConversionKind.ImplicitNullable, ImmutableArray.Create(underlyingConversion));


I think we need to update the speclet to more correctly convey the logic here. It currently states that a collection expression conversion exists from a collection expression to T?, where there exists a collection expression conversion from a collection expression to the underlying type T if T? is a nullable value type. We instead want the speclet to convey that some types of implicit conversions are allowed on top of collection expression conversions, such as implicit nullable conversions.

I'll leave that up to Chuck who made the spec update

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

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/MethodTypeInference.cs

333fred · 2023-09-08T20:06:31Z

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

@@ -233,7 +233,9 @@ static bool filterConversion(Conversion conversion)

                if (source.Kind == BoundKind.UnconvertedCollectionExpression)
                {
-                    Debug.Assert(conversion.IsCollectionExpression || !conversion.Exists);
+                    Debug.Assert(conversion.IsCollectionExpression
+                        || (conversion.IsNullable && conversion.UnderlyingConversions[0].IsCollectionExpression)


Nit:

Suggested change

|| (conversion.IsNullable && conversion.UnderlyingConversions[0].IsCollectionExpression)

|| (conversion is { IsNullable: true, UnderlyingConversions: [{ IsCollectionExpression: true }] })

``` #ByDesign

333fred · 2023-09-08T20:08:27Z

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs

+                    }
+                    else if (boundNodeForSyntacticParent is BoundConversion parentConversion)
+                    {
+                        // There was an explicit cast on top of the collection expression.


In this case, I would not expect ConvertedType to contain something different than the current type. ConvertedType is explicitly documented as implicit conversions only. #Resolved

I didn't know that. Thanks for pointing it out!

jcouv · 2023-09-08T21:49:03Z

src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs

+
+            var typeInfo = model.GetTypeInfo(value);
+            Assert.Null(typeInfo.Type);
+            Assert.Equal("MyCollection<System.Int32>", typeInfo.ConvertedType.ToTestDisplayString());


Tagging @AlekseyTs to confirm this is indeed the behavior we want for ConvertedType in cast scenario. #Resolved

This is not what I would expect. I would expect the type and the converted type to be null. There is an explicit nullable conversion on top of this, with an underlying implicit collection conversion on the input to it.

I see. Let's chat with Aleksey on Monday to confirm. If that's the case, then the previous behavior (absent nullable scenarios) was incorrect.

Confirmed the behavior of ConvertedType with some other typeless expression. I'll go with that.

[Fact] public void TODO2() { string src = """ class Program { static int? M() { return (int?)null; } } """; var comp = CreateCompilation(src); comp.VerifyDiagnostics(); var tree = comp.SyntaxTrees.First(); var model = comp.GetSemanticModel(tree); var value = tree.GetRoot().DescendantNodes().OfType<CastExpressionSyntax>().Last().Expression; var typeInfo = model.GetTypeInfo(value); Assert.Null(typeInfo.Type); Assert.Null(typeInfo.ConvertedType); }

333fred · 2023-09-08T22:16:01Z

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs

@@ -2201,7 +2201,7 @@ private static bool IsUserDefinedTrueOrFalse(BoundUnaryOperator @operator)
                    else if (boundNodeForSyntacticParent is BoundConversion parentConversion)
                    {
                        // There was an explicit cast on top of the collection expression.
-                        convertedType = parentConversion.Type;
+                        convertedType = highestBoundExpr.Type;
                        convertedNullability = nullability;
                        conversion = parentConversion.Conversion;


This is also incorrect. #Resolved

jcouv · 2023-09-12T20:58:42Z

@cston @333fred Please take another look.
The fixes to the semantic model affect one IDE test (broken) which Cyrus is looking at (we'll either fix or skip that test with a tracking issue). #Closed

… to collection types in code

…into nullable-collections

CyrusNajmabadi · 2023-09-12T21:16:40Z

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

+        public static bool IsSystemNullable(this TypeSymbol? type, [NotNullWhen(true)] out TypeSymbol? underlyingType)
+        {
+            if (type is NamedTypeSymbol nt
+                && nt.OriginalDefinition.GetSpecialTypeSafe() == SpecialType.System_Nullable_T)


what's the difference between this and IsNullableType above? specifically, why GetSpecialTypeSafe() versus .SpecialType? #Resolved

should IsNullableType defer to IsSystemNullable so everything is consistent?

why do you need to check if its' a NamedType if you're already going use the SpecialTYpe?

what's the difference between this and IsNullableType above?

The difference is that we're returning the underlying type.

specifically, why GetSpecialTypeSafe() versus .SpecialType?

This is a refactoring of existing code. GetSpecialTypeSafe is just SpecialType plus a null check. That said, we don't need GetSpecialTypeSafe() here since OriginalDefinition cannot be null.

should IsNullableType defer to IsSystemNullable so everything is consistent?

I don't see a need.

why do you need to check if its' a NamedType if you're already going use the SpecialTYpe?

We can only access TypeArgumentsWithAnnotationsNoUseSiteDiagnostics on a NamedTypeSymbol.

My basic point is that the these functions are virtually the same, except for the convenience of getting the type argument. As such, keeping them as similar as possible (naming, impl, etc.) seems like the best thing to do. Having them be subtly different just seems like a way for bugs to creep in.

Wrt the named type check. It seems better to do the special type check first, then just do a cast afterwards.

cston · 2023-09-13T17:51:27Z

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

@@ -1129,6 +1129,32 @@ private Conversion ClassifyImplicitBuiltInConversionFromExpression(BoundExpressi
            return Conversion.NoConversion;
        }

+#nullable enable
+        private Conversion GetImplicitCollectionExpressionConversion(BoundUnconvertedCollectionExpression collectionExpression, TypeSymbol destination, CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)


CompoundUseSiteInfo

Should this be a ref parameter? #Closed

cston · 2023-09-13T17:59:00Z

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs

-                        conversion = convertedCollection.CollectionTypeKind == CollectionExpressionTypeKind.None
-                            ? Conversion.NoConversion
-                            : Conversion.CollectionExpression;
+                        // Explicit cast or error scenario like `object x = [];`


Explicit cast

Does this case still cover explicit casts? (If so, it's surprising the converted type is null and the conversion is Identity.) If not, consider updating the comment. #Resolved

Yes, this case still covers explicit casts (test NullableValueType_ExplicitCast for instance).

The behavior for ConvertedType and the conversion matches what we have for other typeless expressions with an explicit cast:

[Fact] public void TODO2() { string src = """ partial class Program { static object M() { return (object)null; } } """; var comp = CreateCompilation(src, targetFramework: TargetFramework.Net80); comp.VerifyDiagnostics(); var tree = comp.SyntaxTrees.First(); var model = comp.GetSemanticModel(tree); var value = tree.GetRoot().DescendantNodes().OfType<CastExpressionSyntax>().Last().Expression; Assert.Equal("null", value.ToFullString()); var conversion = model.GetConversion(value); Assert.True(conversion.IsIdentity); var typeInfo = model.GetTypeInfo(value); Assert.Null(typeInfo.Type); Assert.Null(typeInfo.ConvertedType); }

cston · 2023-09-13T18:00:56Z

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

@@ -146,6 +146,19 @@ public static TypeSymbol GetNullableUnderlyingType(this TypeSymbol type)
            return type.GetNullableUnderlyingTypeWithAnnotations().Type;
        }

+        public static bool IsSystemNullable(this TypeSymbol? type, [NotNullWhen(true)] out TypeSymbol? underlyingType)


IsSystemNullable

Perhaps rename to IsNullableType to match the methods above. #Closed

cston · 2023-09-13T18:10:37Z

src/Analyzers/CSharp/Analyzers/UseCollectionExpression/UseCollectionExpressionHelpers.cs

-
-        if (speculationAnalyzer.ReplacementChangesSemantics())
-            return false;
+        // HACK: Workaround lack of compiler information for collection expression conversions with casts.


HACK

Are we expecting to fix this in #68826? If so, let's reference the issue here instead.

I'll let @CyrusNajmabadi answer that one, since this is part of his change.

333fred

LGTM. Just a couple of nits

333fred · 2023-09-13T19:00:13Z

src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs

@@ -2851,6 +2851,91 @@ static void Main()
            comp.VerifyEmitDiagnostics();
        }

+        [Fact]
+        public void TypeInference_Nullable()


Nullable is a bit of an ambiguous term now, let's clarify what nullable we mean :).

Suggested change

public void TypeInference_Nullable()

public void TypeInference_NullableValueType()

``` #Resolved

333fred · 2023-09-13T19:00:23Z

src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs

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


Suggested change

public void TypeInference_Nullable_ExtensionMethod()

public void TypeInference_NullableValueType_ExtensionMethod()

``` #Resolved

src/Analyzers/CSharp/Analyzers/UseCollectionExpression/UseCollectionExpressionHelpers.cs

…ectionExpressionHelpers.cs

jcouv self-assigned this Sep 7, 2023

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Sep 7, 2023

jcouv added the New Feature - Collection Expressions label Sep 7, 2023

jcouv force-pushed the nullable-collections branch 2 times, most recently from 262af09 to e92a30f Compare September 7, 2023 21:47

jcouv added this to the 17.8 milestone Sep 7, 2023

Initialization of nullable collections

25a1a98

jcouv force-pushed the nullable-collections branch from e92a30f to 25a1a98 Compare September 7, 2023 23:41

jcouv marked this pull request as ready for review September 7, 2023 23:42

jcouv requested a review from a team as a code owner September 7, 2023 23:42

CyrusNajmabadi reviewed Sep 8, 2023

View reviewed changes

Another test

5e0dc83

cston reviewed Sep 8, 2023

View reviewed changes

Address feedback

e5d30e5

cston reviewed Sep 8, 2023

View reviewed changes

jcouv commented Sep 8, 2023

View reviewed changes

cston approved these changes Sep 8, 2023

View reviewed changes

Address feedback

0a8954c

cston approved these changes Sep 8, 2023

View reviewed changes

333fred reviewed Sep 8, 2023

View reviewed changes

jcouv added 2 commits September 8, 2023 13:35

Address feedback

7775b3b

Tweak ConvertedType

2e7d22e

jcouv commented Sep 8, 2023

View reviewed changes

jcouv requested a review from 333fred September 8, 2023 22:05

333fred reviewed Sep 8, 2023

View reviewed changes

Null ConvertedType for cast

db85e4e

jcouv requested a review from 333fred September 9, 2023 00:27

fixup

3004a74

jcouv marked this pull request as ready for review September 12, 2023 20:57

jcouv requested a review from cston September 12, 2023 20:58

CyrusNajmabadi added 2 commits September 12, 2023 14:09

Add workaround for lack of conversion information with explicit casts…

a02b03a

… to collection types in code

Merge branch 'nullable-collections' of https://github.com/jcouv/roslyn …

543e88e

…into nullable-collections

CyrusNajmabadi requested a review from a team as a code owner September 12, 2023 21:09

CyrusNajmabadi reviewed Sep 12, 2023

View reviewed changes

CyrusNajmabadi and others added 3 commits September 12, 2023 14:18

HACK

feac1e8

Simplify

53f73be

Remove todo

c466914

cston reviewed Sep 13, 2023

View reviewed changes

333fred approved these changes Sep 13, 2023

View reviewed changes

cston mentioned this pull request Sep 13, 2023

Collection expressions: include additional implicit conversions dotnet/csharplang#7533

Merged

jcouv added 2 commits September 13, 2023 14:18

Address feedback

0259176

rename one more test

b710a67

cston approved these changes Sep 13, 2023

View reviewed changes

jcouv commented Sep 13, 2023

View reviewed changes

src/Analyzers/CSharp/Analyzers/UseCollectionExpression/UseCollectionExpressionHelpers.cs Show resolved Hide resolved

Update src/Analyzers/CSharp/Analyzers/UseCollectionExpression/UseColl…

7d539d4

…ectionExpressionHelpers.cs

jcouv enabled auto-merge (squash) September 13, 2023 22:38

jcouv merged commit 7808feb into dotnet:main Sep 14, 2023
27 checks passed

ghost modified the milestones: 17.8, Next Sep 14, 2023

allisonchou mentioned this pull request Sep 18, 2023

[Automated] PRs inserted in VS build main-34118.122 #69996

Closed

jcouv mentioned this pull request Sep 25, 2023

GetTypeInfo returns TypeInfo with null as Type for ImplicitObjectCreationExpression inside CollectionExpression #70041

Closed

Cosifne modified the milestones: Next, 17.8 P3 Sep 25, 2023


		var verifier = CompileAndVerify(comp, expectedOutput: IncludeExpectedOutput("[1, 2, 3],"), verify: Verification.Fails);

		// ILVerify:

	\|\| (conversion.IsNullable && conversion.UnderlyingConversions[0].IsCollectionExpression)
	\|\| (conversion is { IsNullable: true, UnderlyingConversions: [{ IsCollectionExpression: true }] })
	``` #ByDesign

	public void TypeInference_Nullable()
	public void TypeInference_NullableValueType()
	``` #Resolved

	public void TypeInference_Nullable_ExtensionMethod()
	public void TypeInference_NullableValueType_ExtensionMethod()
	``` #Resolved

Nullable conversion of collection expressions #69852

Nullable conversion of collection expressions #69852

Conversation

jcouv commented Sep 7, 2023

CyrusNajmabadi Sep 8, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Sep 8, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Sep 8, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Sep 8, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Sep 8, 2023

333fred left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred Sep 8, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

333fred Sep 8, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

333fred Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred Sep 8, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv commented Sep 12, 2023 • edited Loading

CyrusNajmabadi Sep 12, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

cston Sep 13, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred left a comment

Choose a reason for hiding this comment

333fred Sep 13, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

333fred Sep 13, 2023 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 8, 2023 •

edited by jcouv

Loading

cston Sep 8, 2023 •

edited by jcouv

Loading

cston Sep 8, 2023 •

edited by jcouv

Loading

cston Sep 8, 2023 •

edited by jcouv

Loading

jcouv Sep 8, 2023 •

edited

Loading

333fred Sep 8, 2023 •

edited by jcouv

Loading

333fred Sep 8, 2023 •

edited by jcouv

Loading

jcouv Sep 8, 2023 •

edited

Loading

333fred Sep 8, 2023 •

edited

Loading

333fred Sep 8, 2023 •

edited by jcouv

Loading

jcouv commented Sep 12, 2023 •

edited

Loading

CyrusNajmabadi Sep 12, 2023 •

edited by jcouv

Loading

cston Sep 13, 2023 •

edited

Loading

cston Sep 13, 2023 •

edited by jcouv

Loading

cston Sep 13, 2023 •

edited

Loading

333fred Sep 13, 2023 •

edited by jcouv

Loading

333fred Sep 13, 2023 •

edited by jcouv

Loading