Skip to content

Fully update the type inferrer and add tests for generate field#36173

Merged
jasonmalinowski merged 2 commits intodotnet:masterfrom
jasonmalinowski:fix-generate-field-nullability
Jun 12, 2019
Merged

Fully update the type inferrer and add tests for generate field#36173
jasonmalinowski merged 2 commits intodotnet:masterfrom
jasonmalinowski:fix-generate-field-nullability

Conversation

@jasonmalinowski
Copy link
Copy Markdown
Member

@jasonmalinowski jasonmalinowski commented Jun 5, 2019

"Fixes" generate field in that this really just fixes up the rest of the TypeInferrer -- at this point once that was fixed the feature just lit up. Added some basic tests to cover.

@jasonmalinowski jasonmalinowski requested a review from a team as a code owner June 5, 2019 02:31
@jasonmalinowski jasonmalinowski self-assigned this Jun 5, 2019
@jasonmalinowski jasonmalinowski force-pushed the fix-generate-field-nullability branch from 0f49a54 to 992c4db Compare June 12, 2019 06:00
@jasonmalinowski jasonmalinowski changed the title Fix generate field nullability handling Fully update the type inferrer and add tests for generate field Jun 12, 2019
jasonmalinowski
jasonmalinowski commented Jun 12, 2019
View reviewed changes
src/EditorFeatures/CSharpTest/TypeInferrer/TypeInferrerTests.cs Outdated
Copy link
Copy Markdown
Member Author

@jasonmalinowski jasonmalinowski Jun 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Since I think binding of indexers are now working (:wink:), I updated the test to be more appropriate.

jasonmalinowski
jasonmalinowski commented Jun 12, 2019
View reviewed changes
src/Workspaces/CSharp/Portable/LanguageServices/CSharpTypeInferenceService.TypeInferrer.cs Outdated
Copy link
Copy Markdown
Member Author

@jasonmalinowski jasonmalinowski Jun 12, 2019

Choose a reason for hiding this comment

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

GetAllTypeArgments finds all the type arguments including for parent types -- but since all callers passed 0 for parameterIndex it was moot.

Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi Jun 12, 2019

Choose a reason for hiding this comment

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

lol

@jasonmalinowski jasonmalinowski force-pushed the fix-generate-field-nullability branch from 992c4db to 3001540 Compare June 12, 2019 18:10
jasonmalinowski
jasonmalinowski commented Jun 12, 2019
View reviewed changes
src/Workspaces/CSharp/Portable/LanguageServices/CSharpTypeInferenceService.TypeInferrer.cs Outdated
Copy link
Copy Markdown
Member Author

@jasonmalinowski jasonmalinowski Jun 12, 2019

Choose a reason for hiding this comment

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

@CyrusNajmabadi This scenario seems off -- shouldn't it be looking for Add methods or something else here...? I guess I'm not quite following this one.

Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi Jun 12, 2019

Choose a reason for hiding this comment

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

Yes. I think you're correct. Not sure why we did things this way.

@jasonmalinowski jasonmalinowski force-pushed the fix-generate-field-nullability branch from 3001540 to f472469 Compare June 12, 2019 18:40
ryzngard
ryzngard approved these changes Jun 12, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@ryzngard ryzngard left a comment

Choose a reason for hiding this comment

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

Yup, looks like a lot of tests and WithNullability additions. I couldn't immediately think of a scenario not covered, so LGTM

@jinujoseph jinujoseph added this to the 16.2.P4 milestone Jun 12, 2019
@jasonmalinowski jasonmalinowski merged commit 0852a2b into dotnet:master Jun 12, 2019
@jasonmalinowski jasonmalinowski deleted the fix-generate-field-nullability branch June 12, 2019 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

@ryzngard ryzngard ryzngard approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jasonmalinowski jasonmalinowski

Labels

Approved to merge Area-IDE

Projects

None yet

Milestone

16.2.P4

Development

Successfully merging this pull request may close these issues.

4 participants

@jasonmalinowski @ryzngard @CyrusNajmabadi @jinujoseph