Add actual nullable analysis support for `with(...)` elements. by CyrusNajmabadi · Pull Request #81978 · dotnet/roslyn

CyrusNajmabadi · 2026-01-12T22:56:24Z

Relates to test plan #80613

…onExprArgTests

CyrusNajmabadi · 2026-01-14T13:42:12Z

                """;

-            // We're missing a diagnostic for the `where T : notnull` constraint on the Create method
-            // Tracked by https://github.com/dotnet/roslyn/issues/68786


yaay. one bug down.

CyrusNajmabadi · 2026-01-14T13:42:33Z

                """;

-            // We're missing a diagnostic for the `where T : notnull` constraint on the Create method
-            // Tracked by https://github.com/dotnet/roslyn/issues/68786


CyrusNajmabadi · 2026-01-14T13:43:10Z

+            expectedOutput: IncludeExpectedOutput("""
+            42
+            0, 1
+            """)).VerifyDiagnostics().VerifyIL("Program.<Main>d__0.System.Runtime.CompilerServices.IAsyncStateMachine.MoveNext()", """


i can remove the IL if desired. but i thought it usefful to show that everything is happening correctly.

Let's assert runtime async code instead. Much shorter and easier to verify.

CyrusNajmabadi · 2026-01-14T13:43:24Z

    public void ConstructorNonNullParameterPassedNull_Inference1()
    {
-        // PROTOTYPE: Currently, inference on Goo is producing `Goo<string>` even though it should be `Goo<string!>`.
-        // This is problematic as it then means T is oblivious and we don't properly report a warning on 'null'.


333fred

Done review pass

333fred · 2026-01-14T23:44:51Z

Think we can add MemberNotNull tests for collection builders; this would be for another static member on the builder type. Should also be able to test DoesNotReturn and DoesNotReturnIf.

DoesNotReturnIf: 3aba20f

DoesNotReturn: 10beb60

MemberNotNull: 8f04bcb

Note: membernotnull doesn't work. i added a prototype comment. However, this is a limtation that existed prior to my change. It's also very esoteric. So i don't think this shuold be blocking, and we can track this with an issue. LMK if that's sufficient for you @333fred .

333fred · 2026-01-14T23:46:13Z

+            expectedOutput: IncludeExpectedOutput("""
+            42
+            0, 1
+            """)).VerifyDiagnostics().VerifyIL("Program.<Main>d__0.System.Runtime.CompilerServices.IAsyncStateMachine.MoveNext()", """


Let's assert runtime async code instead. Much shorter and easier to verify.

333fred · 2026-01-14T23:49:05Z

+                        // take the final collection type and use its type arguments to re-construct the creation method.
+
+                        var allTypeArguments = ((NamedTypeSymbol)collectionFinalType).GetAllTypeArgumentsNoUseSiteDiagnostics();
+                        Debug.Assert(allTypeArguments.Length == call.Method.Arity, "Guaranteed by GetCollectionBuilderMethods");


Are there tests of the form:

// Collection builder method public static MyCollection<TList> Create<TList, TOther>(TOther arg, ReadOnlySpan<TList> elements) { ... } MyCollection<int> m = [with(1), 2];

To verify that such cases don't go through here in error recovery modes?

Added in 186a431

CyrusNajmabadi · 2026-01-15T17:04:40Z

Runtime async done in: c2c780c. Much nicer.

CyrusNajmabadi · 2026-01-15T17:51:42Z

@333fred for another pass.

CyrusNajmabadi · 2026-01-15T17:55:04Z

@RikkiGibson ptal :)

RikkiGibson · 2026-01-15T19:45:39Z

+                    // Walk into the arguments of the object creation, passing in 'delayCompletionForTargetMember: true'
+                    // so that we reprocess the nullability of the arguments when we have the target-type for the
+                    // collection expression.
+                    var (_, argumentResults, _, completion) = this.VisitArguments(


@jcouv I think your change to introduce a named return type for some of these may cause conflicts and/or build failures when merging the branches. I think they will be simple to resolve, just, thought I would give a heads up.

RikkiGibson

LGTM, comments are not blocking.

RikkiGibson · 2026-01-15T20:00:52Z

+                        var allTypeArguments = ((NamedTypeSymbol)collectionFinalType).GetAllTypeArgumentsNoUseSiteDiagnostics();
+                        Debug.Assert(allTypeArguments.Length == call.Method.Arity, "Guaranteed by GetCollectionBuilderMethods");
+
+                        var constructed = call.Method.Arity == 0 ? call.Method : call.Method.ConstructedFrom.Construct(allTypeArguments);


I am wondering if there is an impact to using 'ConstructedFrom' versus 'OriginalDefinition' here.

The difference is in whether the containing type is generic. I'd expect OriginalDefinition to return the original method in the original type, while ConstructedFrom returns the original method in the constructed type.

Correct.

Because the containing type must be a normal-non-generic type, this ends up not mattering (since .OriginalDef and .ConstructedFrom will be the same.

But, conceptually .ConstructedFrom makes the most sense here as we would not be reinferring the type args on the containing type, just reconstructing the method again, off of whatever type it came from.

So going to leave this as is.

RikkiGibson · 2026-01-15T20:05:06Z

+                    // of the arguments passed into them.  The other kinds are:
+                    //
+                    // 1. Arrays/Spans.  These can't have CollectionCreation expressions at all.
+                    // 2. Array interfaces (e.g. IList<T>).  These only have two supported constructors that are called


It might be good to translate this list into assertions, so that when conditions change in the future (new collection kinds, etc.), we could find out sooner that a change is also needed here.

RikkiGibson · 2026-01-15T21:41:28Z

+        {
+            var count = 0;
+            for (var current = symbol; current is not null; current = current.ContainingType)
+                count += current.TypeArgumentsWithAnnotationsNoUseSiteDiagnostics.Length;


nit: Arity? This isn't blocking.

i could switch it to that... but this feels more clearly correct, as it is preallocating the size for exactly what we know we will be adding.

RikkiGibson · 2026-01-15T22:02:23Z

+            using System;
+            using System.Collections.Generic;
+
+            var v = true ? [with(""), null] : new MyCollection<string>("");


Do we have a case like M(true ? [with(null), ""] : [with(""), ""]), with void M<T>(MyCollection<T> arg)? e.g. where nullability is "pushed down" to the other branch and we show there are no warnings (both paths are inferred as nullable). I am assuming we do.

i will add teest in followup. In this case, because the elements are all non-null, i would expect this to be inferred as string and the with(null) wouold warn.

It was part of the design that the with(...) call itself doesn't contribute to inference. but we can def have a few variants on this to show what's going on. will incclude in followup PR.

added in bf9745b

RikkiGibson · 2026-01-15T22:14:35Z

+            }
+            """;
+
+        // PROTOTYPE: MemberNotNull analysis does not flow across calls to collection builders yet. This is probably


It might be good to ensure that a MemberNotNull on a create method, doesn't update a member of the collection type.

It seems also reasonable to test the analogous cases with constructors, if the attribute can be applied to those.

It seems also reasonable to test the analogous cases with constructors, if the attribute can be applied to those.

It can't be applied to constructors. Only methods and properties.

Yup. intentional that this was only for builders, as it's not legal on constructors (unlike many of the other attributes).

jcouv · 2026-01-15T22:40:15Z

    {
-        string sourceA = """
+        var source = """
            #nullable enable


Do we have nullability tests verifying the symbols post nullability analysis?
I'd expect GetSymbolInfo on with to reflect the re-inferred symbols.

I will add in followup PR (i have two coming) :)

added in f96fb48

CyrusNajmabadi added 30 commits January 8, 2026 13:24

Add with(...) nullable tests

1568fa9

Revert

d2ad15c

Add tests

583c748

Add inference tests

f95f305

Add inference tests

3a4cf38

Update tssts

6c8fb67

prototype

02ddfe2

Merge branch 'features/collection-expression-arguments' into collecti…

8f41c7c

…onExprArgTests

Fix tests

5cfa3df

Add attribute tests

3f94372

add some attribute tests

3b2a7e2

Include nullaiblity info

949d273

Validate type

f05d9e4

Validate return type

4325503

Add accessibiility tests

71f7065

Add file test

0ad9f6c

Add dynamic tests

242a502

Ref readonly test

667fa65

arglist

e20abf5

Add interpolated string support

54dd69c

Add orpa test

e83f423

Support GetMemberGroup for collection builder with elements

04dcc51

update tests

a88a6a0

Merge branch 'features/collection-expression-arguments' into collecti…

1782f60

…onExprArgTests

Add tests

7688e8b

Fixup

35c14e7

Add unscopedref tests

48c18d4

Add file local test with file arg type

ba2078a

Add file local test with file arg type

ae535b3

Update test

d6f9b61

CyrusNajmabadi added 3 commits January 14, 2026 14:32

Fix

4f635b7

Add tests

9b6771c

Fix comments

0f077d0

CyrusNajmabadi commented Jan 14, 2026

View reviewed changes

333fred reviewed Jan 14, 2026

View reviewed changes

CyrusNajmabadi added 3 commits January 15, 2026 17:42

Casing

56c57d6

Unify asserts

48cd9ad

Improve il output

c2c780c

CyrusNajmabadi added 5 commits January 15, 2026 18:08

Async il

ad633c1

Add test

186a431

Add DoesNotReturnIf tests

3aba20f

Add DoesNotReturn tests

10beb60

Add MemberNotNull test

8f04bcb

CyrusNajmabadi requested a review from 333fred January 15, 2026 17:51

333fred approved these changes Jan 15, 2026

View reviewed changes

RikkiGibson reviewed Jan 15, 2026

View reviewed changes

CyrusNajmabadi mentioned this pull request Jan 15, 2026

Remove remaining PROTOTYPE pieces of with(...), so it can be merged into main. #82030

Merged

RikkiGibson approved these changes Jan 15, 2026

View reviewed changes

jcouv reviewed Jan 15, 2026

View reviewed changes

Comment thread src/Compilers/CSharp/Portable/Symbols/SymbolExtensions.cs

CyrusNajmabadi merged commit 48270ee into dotnet:features/collection-expression-arguments Jan 15, 2026
24 checks passed

CyrusNajmabadi deleted the collectionExprNullable branch January 15, 2026 23:28

dotnet-maestro Bot mentioned this pull request Jan 22, 2026

[main] Source code updates from dotnet/roslyn dotnet/dotnet#4349

Merged

dotnet-bot mentioned this pull request Jan 24, 2026

[Automated] PRs inserted in VS build main-11423.137 #82138

Closed

Conversation

CyrusNajmabadi commented Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Jan 15, 2026

Uh oh!

CyrusNajmabadi commented Jan 15, 2026

Uh oh!

CyrusNajmabadi commented Jan 15, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson left a comment

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!

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

CyrusNajmabadi commented Jan 12, 2026 •

edited

Loading