Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check implicit receiver in nested initializers #34141

Merged
merged 4 commits into from Mar 18, 2019

Conversation

@jcouv
Copy link
Member

jcouv commented Mar 15, 2019

Fixes #32495

@jcouv jcouv added this to the 16.1.P1 milestone Mar 15, 2019

@jcouv jcouv self-assigned this Mar 15, 2019

@jcouv jcouv requested a review from dotnet/roslyn-compiler as a code owner Mar 15, 2019

@jcouv jcouv force-pushed the jcouv:initializer-bug branch from 7f80a16 to 628eab3 Mar 15, 2019

}", options: WithNonNullTypesTrue());

comp.VerifyDiagnostics(
// (12,25): warning CS8602: Possible dereference of a null reference.

This comment has been minimized.

@333fred

333fred Mar 15, 2019

Member

Perhaps we should consider putting the warning on the f = part, and not the f2 and f3 parts. I don't think it's immediately obvious why there's an error on f2 or f3. #Closed

This comment has been minimized.

@jcouv

jcouv Mar 15, 2019

Author Member

It still won't be obvious, but I adjusted the location a bit ;-) #Closed

@@ -85,6 +85,81 @@ public override string ToString()
}
}";

[Fact, WorkItem(32495, "https://github.com/dotnet/roslyn/issues/32495")]

This comment has been minimized.

@333fred

333fred Mar 15, 2019

Member

Please add tests for:
When f is both not null and when it's defined in an oblivious context.
Doubly nested (Do we warn on both the nested f2 = { ... } and the outer f = { f2 = { ... } }?)
Object initializer inside an indexer that returns a nullable reference.

This comment has been minimized.

@jcouv

jcouv Mar 15, 2019

Author Member

I don't understand the last suggestion


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

@333fred

This comment has been minimized.

Copy link
Member

333fred commented Mar 15, 2019

Done review pass (commit 2). Would like some more tests.

public B? fb;
static void Main()
{
new C() { fc = { fb = { f2 = null} }};

This comment has been minimized.

@333fred

333fred Mar 15, 2019

Member

fc [](start = 18, length = 2)

What I meant was making fc here nullable as well: namely, what will happen when there's potential nested null derefs.

@jcouv

This comment has been minimized.

Copy link
Member Author

jcouv commented Mar 18, 2019

There was a transient failure in one of the async-iterator tests. I filed issue to investigate further: #34207

@333fred
Copy link
Member

333fred left a comment

LGTM (commit 4)

@jcouv

This comment has been minimized.

Copy link
Member Author

jcouv commented Mar 18, 2019

@cston @dotnet/roslyn-compiler for a second review. Thanks

// (6,14): warning CS8618: Non-nullable field 'f' is uninitialized.
// public class C
Diagnostic(ErrorCode.WRN_UninitializedNonNullableField, "C").WithArguments("field", "f").WithLocation(6, 14)
);

This comment has been minimized.

@cston

cston Mar 18, 2019

Member

); [](start = 16, length = 2)

Shouldn't we report CS8602: Possible dereference here too?

This comment has been minimized.

@jcouv

jcouv Mar 18, 2019

Author Member

Resolved offline.

@cston

cston approved these changes Mar 18, 2019

@jcouv jcouv merged commit e05ae87 into dotnet:master Mar 18, 2019

1 check passed

license/cla All CLA requirements met.
Details

@jcouv jcouv deleted the jcouv:initializer-bug branch Mar 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.