Add test-plan tests for with(...) elements.#81922
Conversation
333fred
left a comment
There was a problem hiding this comment.
As mentioned offline, need nullable attribute tests as well.
| Goo([with(null), ""]); | ||
| } | ||
|
|
||
| static void Goo<T>(MyList<T> list) |
There was a problem hiding this comment.
Let's have this return a T and validate what the nullability of that T is at the call-site above. Similar for the other inference cases.
|
@333fred ptal :) This is really a tiny code change to collection-exprs, and then a bunch of tests to help go through our test plan backlog. would like to make good forward progress on this on CEST hours. Thanks :) |
There was a problem hiding this comment.
I don't see any tests for AllowNull/DisallowNull/NotNullIfNotNull.
There was a problem hiding this comment.
i would prefer to hold off on more nullable tests and have those be added to #81978 thanks. We can then extensively test nullable in that pr.
| var root = syntaxTree.GetRoot(); | ||
| var withElement = root.DescendantNodes().OfType<WithElementSyntax>().Single(); | ||
|
|
||
| // It is expected that we get 0 here. GetMemberGroup returns nothing for a WithElementSyntax |
There was a problem hiding this comment.
I didn't follow. GetMemberGroup on an object creation returns symbols (see case BoundKind.ObjectCreationExpression: in GetSemanticSymbols)
Let's confirm expected behavior and track follow-up issue if needed #Closed
There was a problem hiding this comment.
As I mentioned in the test plan. We return nothing for a constructorinitializer syntax. That's a similar case of a keyword+arglist. I'm fine with this also not returning anything for now.
Or, put another way, why do we need this to return anything when we've never put in the effort to make this(...) return anything, and that's been in the language since v1.
There was a problem hiding this comment.
if we can, let's please have any further discussions on this here: #80613 (comment)
For now, i woudl like to just document the behavior in tests and not have this slow down getting this PR in.
There was a problem hiding this comment.
Got it. Thanks for the clarification. Added note to the GetMemberGroup line in test plan (ie. same behavior as this(...))
|
|
||
| class MyList<T> : List<T> | ||
| { | ||
| public MyList([NotNull] string? arg) |
There was a problem hiding this comment.
NotNull/MaybeNull are post-condition attributes; they would be more useful on an out parameter. I think they do nothing on by-value parameters #Closed
There was a problem hiding this comment.
They work just fine on by-value parameters. That's how things like ArgumentNotNullException.ThrowIfNull work. Or, as demonstrated here, how a warning is avoided on the call to Goo(s), since that expects a non-null string.
There was a problem hiding this comment.
Yes. this test is intentionally written this way. Note: i'm happy to do more 'nullable' testing. but would prefer that happen in #81978
| verify: Verification.FailsPEVerify).VerifyDiagnostics( | ||
| // (7,37): warning CS8625: Cannot convert null literal to non-nullable reference type. | ||
| // MyCollection<int> c = [with(null), 1, 2]; | ||
| Diagnostic(ErrorCode.WRN_NullAsNonNullable, "null").WithLocation(7, 37), |
There was a problem hiding this comment.
Why is this diagnostic appearing twice? #Closed
There was a problem hiding this comment.
fat fingered. fixed.
jcouv
left a comment
There was a problem hiding this comment.
Done with review pass (commit 40)
|
@333fred @jcouv would prefer this PR just be about basic mop of most of the test plan not including nullable. So fixing gaps where i tested for constructors but not builders (and vice versa). And any one-off sort of issues you think of here. These scenarios all seem to eb working well, and it would be good to get this in, and have nearly all of the test plan considered to be in good shape. Nullable will come in #81978 as that involves actual substantive changes to binding (done by me and rikki) and will then have a LOT of testing around many cases (including attributes). |
|
@jcouv @dotnet/roslyn-compiler for a second pair of eyes. tnx :) |
ee3bf52
into
dotnet:features/collection-expression-arguments
Relates to test plan #80613