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.

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

Address issues with nullable flow analysis slot container types and changing types during inference #39301

Merged
merged 15 commits into from
Feb 24, 2020
Merged

Address issues with nullable flow analysis slot container types and changing types during inference #39301

merged 15 commits into from
Feb 24, 2020

Conversation

gafter
Copy link
Member

@gafter gafter commented Oct 15, 2019 •
edited
Loading

Fixes #39220
Fixes #39297
Fixes #33428

@gafter gafter added Bug Area-Compilers PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Oct 15, 2019
@gafter gafter added this to the 16.4 milestone Oct 15, 2019
@gafter gafter requested a review from a team as a code owner October 15, 2019 22:45
@gafter gafter self-assigned this Oct 15, 2019
@jinujoseph jinujoseph modified the milestones: 16.4, 16.5 Dec 11, 2019
Partial fix for dotnet#39220
f253208 
Fixes dotnet#39220

Filter out "bad" slots not a member of the container.

Fixes dotnet#33428
@gafter gafter changed the title DRAFT changes. On hold pending #20648 Address issues with nullable flow analysis slot container types and changing types during inference Jan 6, 2020
@gafter gafter removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jan 6, 2020
@gafter gafter requested a review from cston January 6, 2020 23:22
@gafter
Copy link
Member Author

gafter commented Jan 6, 2020 •
edited
Loading

@cston Can you please have a look at this? We discussed this earlier today. #Resolved

cston
cston approved these changes Jan 7, 2020
View reviewed changes
Copy link
Member

@cston cston left a comment

Choose a reason for hiding this comment

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

LGTM. Minor comments only.

@gafter
Copy link
Member Author

gafter commented Jan 14, 2020 •
edited
Loading

@cston You last reviewed iteration 2. I've made a number of changes since then. Can you please have another look?

@dotnet/roslyn-compiler May I please have a second review of this nullable change? It adds assertions to check invariants around the way slots are used.
#Resolved

@cston
Copy link
Member

cston commented Jan 15, 2020 •
edited
Loading

#nullable disable

restore #Closed

Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:170 in 5217bea. [](commit_id = 5217bea, deletion_comment = False)

@cston
Copy link
Member

cston commented Jan 15, 2020 •
edited by gafter
Loading 

        if (!(CurrentSymbol is MethodSymbol methodSymbol))

Is this change needed? Same question for other changes of _symbol to CurrentSymbol. #Resolved

Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:1429 in 15a06b8. [](commit_id = 15a06b8, deletion_comment = False)

@gafter
Copy link
Member Author

gafter commented Jan 16, 2020 •
edited
Loading 

        if (!(CurrentSymbol is MethodSymbol methodSymbol))

No sure which change you're referring to (the one in EnterParameters?), but yes, at least the one inside VisitYieldReturn is needed. I'll revert the rest. I'm not saying I know which is correct, but the tests don't distinguish.

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

Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:1429 in 15a06b8. [](commit_id = 15a06b8, deletion_comment = False)

@gafter
Copy link
Member Author

gafter commented Jan 16, 2020 •
edited
Loading

@AlekseyTs I think I've addressed your comments. Do you have any others? #Resolved

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jan 16, 2020 •
edited by gafter
Loading

I think I've addressed your comments. Do you have any others?

I do not have any other comments at the moment, but that was the only file I looked at. #Resolved

@gafter
Copy link
Member Author

gafter commented Jan 17, 2020 •
edited
Loading

@dotnet/roslyn-compiler May I please have a second review of this nullability bug fix that strengthens its internal assertions? #Resolved

@jcouv jcouv modified the milestones: 16.5, 16.6.P1 Feb 14, 2020
@gafter
Copy link
Member Author

gafter commented Feb 14, 2020

@dotnet/roslyn-compiler May I please have a second review of this nullability bug fix that strengthens its internal assertions?

@gafter gafter merged commit c288341 into dotnet:master Feb 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlekseyTs AlekseyTs AlekseyTs left review comments

@jaredpar jaredpar jaredpar approved these changes

@cston cston cston approved these changes

Assignees

@gafter gafter

Labels
Area-Compilers Bug
Projects
None yet
Milestone
16.6.P1
6 participants
@gafter @cston @AlekseyTs @jaredpar @jinujoseph @jcouv