Skip to content

Check implicit receiver in nested initializers#34141

Merged
jcouv merged 4 commits intodotnet:masterfrom
jcouv:initializer-bug
Mar 18, 2019
Merged

Check implicit receiver in nested initializers#34141
jcouv merged 4 commits intodotnet:masterfrom
jcouv:initializer-bug

Conversation

@jcouv
Copy link
Copy Markdown
Member

@jcouv 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 a team as a code owner March 15, 2019 02:16
@jcouv jcouv force-pushed the initializer-bug branch from 7f80a16 to 628eab3 Compare March 15, 2019 04:00
Copy link
Copy Markdown
Member

@333fred 333fred Mar 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

@jcouv jcouv Mar 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@333fred 333fred Mar 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the last suggestion


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

@333fred
Copy link
Copy Markdown
Member

333fred commented Mar 15, 2019

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

Copy link
Copy Markdown
Member

@333fred 333fred Mar 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
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

Copy link
Copy Markdown
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (commit 4)

@jcouv
Copy link
Copy Markdown
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)
);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved offline.

@jcouv jcouv merged commit e05ae87 into dotnet:master Mar 18, 2019
@jcouv jcouv deleted the initializer-bug branch March 18, 2019 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants