Skip to content

Conversation

@agocke
Copy link
Member

@agocke agocke commented Dec 16, 2019

This design tries to meld better analysis of nullable reference types in
local functions with performance. To keep the common case one pass,
local functions are analyzed using the starting state that is an
intersection of all the states before its usages (calls, delegate
conversions, etc), but the results of variables made nullable or
non-nullable inside the local function do not propagate to the callers.

@agocke agocke force-pushed the nullable-localfunc-body branch from d5bff1a to a5054ef Compare December 16, 2019 22:53
This design tries to meld better analysis of nullable reference types in
local functions with performance. To keep the common case one pass,
local functions are analyzed using the starting state that is an
intersection of all the states before its usages (calls, delegate
conversions, etc), but the results of variables made nullable or
non-nullable inside the local function do not propagate to the callers.
@agocke agocke force-pushed the nullable-localfunc-body branch from a5054ef to 58db000 Compare December 16, 2019 23:46
@agocke agocke marked this pull request as ready for review December 17, 2019 07:52
@agocke agocke requested a review from a team as a code owner December 17, 2019 07:52
@agocke agocke requested a review from cston December 17, 2019 07:52
@333fred
Copy link
Member

333fred commented Dec 17, 2019

Could you add the test shown in #40157? How does it interact with this change?

@333fred
Copy link
Member

333fred commented Dec 17, 2019

Done review pass (commit 1)

@agocke
Copy link
Member Author

agocke commented Jan 7, 2020

Could you add the test shown in #40157? How does it interact with this change?

I didn't try to address that. I can see two possible specs for the behavior in the absence of a call: we could treat captured variables as always being non-null, or we use the annotation.

My preference would be on the first, but I don't think we've made a decision either way.

@jcouv
Copy link
Member

jcouv commented Jan 7, 2020

I'll take a look shortly.
This PR fixes an issue that is blocking my PR to make var nullable (#40755)

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 2)

@jcouv jcouv added this to the 16.5 milestone Jan 8, 2020
@jcouv jcouv self-assigned this Jan 8, 2020
@jcouv
Copy link
Member

jcouv commented Jan 8, 2020

@agocke Anything holding us back from merging this?
This is blocking my PR on making var infer nullable types. Thanks

@jcouv jcouv merged commit e1ac8e4 into dotnet:master Jan 8, 2020
@jcouv
Copy link
Member

jcouv commented Jan 8, 2020

Merged to unblock my other PR. Andy said it's ready to merge

@agocke agocke deleted the nullable-localfunc-body branch January 8, 2020 21:01
@agocke
Copy link
Member Author

agocke commented Jan 8, 2020

SGTM thanks

333fred added a commit to 333fred/roslyn that referenced this pull request Jan 8, 2020
* 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants