Skip to content

Handle dependent slots in pattern-matching null tests. #39625

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Jan 6, 2020
Merged

Handle dependent slots in pattern-matching null tests. #39625

merged 8 commits into from
Jan 6, 2020

Conversation

@gafter
Copy link
Member

@gafter gafter commented Nov 1, 2019

Fixes #39264

@gafter gafter added this to the 16.5 milestone Nov 1, 2019
@gafter gafter self-assigned this Nov 1, 2019
@gafter gafter marked this pull request as ready for review December 3, 2019 19:03
@gafter gafter requested a review from a team as a code owner December 3, 2019 19:03
@gafter gafter requested review from RikkiGibson and agocke December 3, 2019 19:03
@gafter
Copy link
Member Author

gafter commented Dec 3, 2019 •
edited by RikkiGibson
Loading

@agocke @RikkiGibson Could you please review this? #Closed

RikkiGibson
RikkiGibson reviewed Dec 3, 2019
View reviewed changes
RikkiGibson
RikkiGibson reviewed Dec 3, 2019
View reviewed changes
@RikkiGibson
Copy link
Member

RikkiGibson commented Dec 3, 2019 •
edited
Loading

Done with review pass (iteration 4) #Closed

@gafter
Copy link
Member Author

gafter commented Dec 4, 2019 •
edited
Loading

@agocke Could you please review this?

@RikkiGibson I think I have responded to all of your comments. Do you have any more? #Closed

RikkiGibson
RikkiGibson approved these changes Dec 4, 2019
View reviewed changes
Copy link
Member

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

:shipit:

333fred
333fred reviewed Dec 12, 2019
View reviewed changes
333fred
333fred reviewed Dec 12, 2019
View reviewed changes
@333fred
Copy link
Member

333fred commented Dec 12, 2019 •
edited by gafter
Loading

Done review pass (commit 5) #Closed

@gafter
Copy link
Member Author

gafter commented Dec 13, 2019 •
edited
Loading

@333fred I think I have responded to your comments. Do you have any others?

@agocke Could you review this please?
#Closed

333fred
333fred approved these changes Dec 13, 2019
View reviewed changes
Copy link
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 6)

agocke
agocke reviewed Dec 13, 2019
View reviewed changes
@gafter
Copy link
Member Author

gafter commented Dec 16, 2019 •
edited
Loading

@dotnet/roslyn-compiler Can I get a Sr engineer to review this, please? #Closed

@gafter
Copy link
Member Author

gafter commented Dec 18, 2019 •
edited
Loading

@agocke @cston I think I've responded to your comments. Do you have any others? #Closed

cston
cston reviewed Dec 18, 2019
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs Outdated
// with another state, those dependent states won't pollute values from the other state.
private void MarkDependentSlotsNotNull(int slot, TypeSymbol expressionType, ref LocalState state)
{
foreach (var member in expressionType.GetMembers())
Copy link
Contributor

@cston cston Dec 18, 2019 •
edited by gafter
Loading

Choose a reason for hiding this comment

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

foreach (var member [](start = 12, length = 19)

If expressionType has a member of the same type (a Next field for instance), won't we create many slots in this method? #Resolved

Copy link
Member Author

@gafter gafter Dec 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, we certainly will. There is a test for that. #Resolved

Copy link
Contributor

@cston cston Dec 18, 2019

Choose a reason for hiding this comment

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

As we discussed, would it be better to go one level deep only? That should address common cases without creating too many slots.

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

@jaredpar jaredpar added the Feature - Nullable Reference Types Nullable Reference Types label Dec 18, 2019
agocke
agocke reviewed Dec 18, 2019
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs Outdated
if (childSlot > 0)
{
state[childSlot] = NullableFlowState.NotNull;
MarkDependentSlotsNotNull(childSlot, member.GetTypeOrReturnType().Type, ref state);
Copy link
Member

@agocke agocke Dec 18, 2019 •
edited by gafter
Loading

Choose a reason for hiding this comment

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

This certainly seems to work, but I can't figure out what the base case in this recursion is. #Resolved

@gafter
Copy link
Member Author

gafter commented Jan 3, 2020 •
edited
Loading

@agocke @cston I think I've addressed your comments. Do you have any other comments? #Closed

cston
cston reviewed Jan 3, 2020
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs Outdated
}
}

IEnumerable<Symbol> getMembers(TypeSymbol type)
Copy link
Contributor

@cston cston Jan 3, 2020 •
edited by gafter
Loading

Choose a reason for hiding this comment

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

IEnumerable [](start = 12, length = 11)

static? #Resolved

cston
cston reviewed Jan 3, 2020
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs Outdated

yield break;

NamedTypeSymbol effectiveBase(TypeSymbol type) => type switch
Copy link
Contributor

@cston cston Jan 3, 2020 •
edited by gafter
Loading

Choose a reason for hiding this comment

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

NamedTypeSymbol [](start = 16, length = 15)

static #Resolved

cston
cston reviewed Jan 3, 2020
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs Outdated
var t => t.BaseTypeNoUseSiteDiagnostics,
};

ImmutableArray<NamedTypeSymbol> inheritedInterfaces(TypeSymbol type) => type switch
Copy link
Contributor

@cston cston Jan 3, 2020 •
edited by gafter
Loading

Choose a reason for hiding this comment

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

ImmutableArray [](start = 16, length = 14)

static #Resolved

@gafter
Copy link
Member Author

gafter commented Jan 3, 2020

@cston I think I've addressed all of your comments. Do you have any others?

@gafter gafter merged commit 86b6392 into dotnet:master Jan 6, 2020
333fred added a commit to 333fred/roslyn that referenced this pull request Jan 8, 2020
@333fred
Merge remote-tracking branch 'dotnet/master' into merge-master
9e13c01 
* dotnet/master: (592 commits)
  Improve nullable analysis of local functions (dotnet#40422)
  Fix race condition in CodeFixService
  Annotate CSharpCompilation (dotnet#40752)
  Revert "Merge pull request dotnet#40765 from dibarbet/revert_use_index"
  More feedback
  address feedback
  More refactoring and hardening of remote calls (dotnet#40395)
  Update PublishData.json
  Unskip and fix integration test
  Update configs for preview 2 snap
  Improve error message for CS0191 (dotnet#40748)
  Revert "Merge pull request dotnet#40410 from sharwell/use-index"
  PR feedback
  Report erroneous implicit conversions in a switch expression (dotnet#40678)
  Ignore dynamic vs object, etc in pattern-matching machinery. (dotnet#40677)
  Handle dependent slots in pattern-matching null tests. (dotnet#39625)
  Pass non-null arguments to avoid nullability warning
  Update dependencies from https://github.com/dotnet/arcade build 20200104.1 (dotnet#40749)
  [master] Update dependencies from dotnet/arcade (dotnet#40718)
  Expose display option to add ! on non-nullable types (dotnet#40519)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@agocke agocke agocke left review comments

@RikkiGibson RikkiGibson RikkiGibson approved these changes

@333fred 333fred 333fred approved these changes

+1 more reviewer

@cston cston cston approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@gafter gafter

Labels

Area-Compilers Feature - Nullable Reference Types Nullable Reference Types Feature - Pattern Matching Pattern Matching

Projects

None yet

Milestone

16.5

Development

Successfully merging this pull request may close these issues.

Roslyn doesn't understand pattern-matching null check for properties

6 participants

@gafter @RikkiGibson @333fred @agocke @cston @jaredpar