Skip to content

Adjust how NotNull interacts with MaybeNullWhen#42322

Merged
jcouv merged 1 commit intodotnet:masterfrom
jcouv:nullable-override
Mar 16, 2020
Merged

Adjust how NotNull interacts with MaybeNullWhen#42322
jcouv merged 1 commit intodotnet:masterfrom
jcouv:nullable-override

Conversation

@jcouv
Copy link
Copy Markdown
Member

@jcouv jcouv commented Mar 10, 2020

In an override situation, we want to allow an assignment from a [NotNull, MaybeNullWhen(true)] T parameter to an identical parameter.

Fixes #42169

@jcouv jcouv added this to the 16.6.P2 milestone Mar 10, 2020
@jcouv jcouv requested a review from a team as a code owner March 10, 2020 19:04
@jcouv jcouv self-assigned this Mar 10, 2020
@RikkiGibson RikkiGibson self-requested a review March 13, 2020 20:57
RikkiGibson
RikkiGibson reviewed Mar 13, 2020
View reviewed changes
Comment thread src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs
public class C
{
internal virtual bool TryGetValueCore<TKey, TValue>(TKey key, [MaybeNull] [NotNullWhen(false)] out TValue value)
where TKey : class
Copy link
Copy Markdown
Member

@RikkiGibson RikkiGibson Mar 13, 2020

Choose a reason for hiding this comment

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

is TKey necessary to reproduce the issue (before the compiler change)?

Copy link
Copy Markdown
Member Author

@jcouv jcouv Mar 13, 2020

Choose a reason for hiding this comment

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

It's not. This came from the use case I ran into in an adoption PR. I'll remove the parameter :-)

@gafter
Copy link
Copy Markdown
Member

gafter commented Mar 16, 2020

Can you please explain the relationship between the bug (and its symptoms) and the bug fix? I would have expected you to be making a fix in the override checks.

@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Mar 16, 2020

@gafter The overrides check computes the nullability of a value (from one of the signatures) and checks if it can be assigned to the type (from the other signature).
For conditional logic, we do that twice. This results in some [MaybeNull, NotNull] cases.

This fix is so that we can assign a value from a [MaybeNull, NotNull] string parameter to a [MaybeNull, NotNull] string parameter by adjusting the nullability of the value. Previously, that would produce a maybe-null value, but now that produces a not-null value.

gafter
gafter approved these changes Mar 16, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@gafter gafter left a comment

Choose a reason for hiding this comment

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

:shipit:

@jcouv jcouv merged commit 8e477d8 into dotnet:master Mar 16, 2020
@ghost ghost modified the milestones: 16.6.P2, Next Mar 16, 2020
@jcouv jcouv deleted the nullable-override branch March 16, 2020 17:51
@jcouv jcouv modified the milestones: Next, 16.6.P2 Mar 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RikkiGibson RikkiGibson RikkiGibson approved these changes

+1 more reviewer

@gafter gafter gafter approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jcouv jcouv

Labels

Area-Compilers Feature - Nullable Reference Types Nullable Reference Types

Projects

None yet

Milestone

16.6.P2

Development

Successfully merging this pull request may close these issues.

Overriding method with [MaybeNull, NottNullWhen(true)] produces a warning

3 participants

@jcouv @gafter @RikkiGibson