Skip to content

Track nullable state across boxing conversions#34087

Merged
cston merged 3 commits intodotnet:masterfrom
cston:boxing
Mar 14, 2019
Merged

Track nullable state across boxing conversions#34087
cston merged 3 commits intodotnet:masterfrom
cston:boxing

Conversation

@cston
Copy link
Copy Markdown
Contributor

@cston cston commented Mar 13, 2019

Fixes #33387
See also #32599

@cston
Copy link
Copy Markdown
Contributor Author

cston commented Mar 14, 2019

@dotnet/roslyn-compiler please review.

Copy link
Copy Markdown
Member

@jaredpar jaredpar 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 added this to the 16.1.P1 milestone Mar 14, 2019
Copy link
Copy Markdown
Member

@jcouv jcouv Mar 14, 2019

Choose a reason for hiding this comment

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

Boxing [](start = 48, length = 6)

from our discussion, it would be good to leave a comment saying why we don't need to create new slots for boxing/unboxing (ie. assignment will do that anyways).
Also, the scenario you wrote on the whiteboard ((S)(object)new S() { F = null }) is a good illustration of current behavior, if that's not already tested. #Resolved

Copy link
Copy Markdown
Member

@jcouv jcouv Mar 14, 2019

Choose a reason for hiding this comment

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

targetType.TypeKind == TypeKind.TypeParameter [](start = 56, length = 45)

I found this change surprising. Why isn't targetType.IsReferenceType sufficient? #ByDesign

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Unconstrained type parameters, and type parameters constrained to value types, were not handled here nor in the else previously.


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

Copy link
Copy Markdown
Member

@jcouv jcouv Mar 14, 2019

Choose a reason for hiding this comment

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

NewT [](start = 35, length = 4)

Should this depend on the T (constrained or not, ...)? #ByDesign

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I believe we should allow all type parameters.


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

Copy link
Copy Markdown
Member

@jcouv jcouv Mar 14, 2019

Choose a reason for hiding this comment

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

Nulalble [](start = 81, length = 8)

nit: typo #Resolved

Copy link
Copy Markdown
Member

@jcouv jcouv Mar 14, 2019

Choose a reason for hiding this comment

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

Nulalble [](start = 81, length = 8)

same (typo) #Resolved

Copy link
Copy Markdown
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 self-assigned this Mar 14, 2019
@cston cston merged commit a6be4ae into dotnet:master Mar 14, 2019
@cston cston deleted the boxing branch March 14, 2019 22:47
333fred added a commit to 333fred/roslyn that referenced this pull request Mar 19, 2019
* dotnet/master: (345 commits)
  Update indexers based on analyzer receiver (dotnet#34134)
  Skip test DecimalBinaryOp_03 (dotnet#34199)
  Remove earlier nullable documentation (dotnet#34153)
  Rewrite FindReferencesTests as theories
  Apply a hang mitigating timeout to ExecuteCommand
  Warn on __refvalue null dereference: (dotnet#34135)
  Update dependencies from https://github.com/dotnet/arcade build 20190312.7 (dotnet#34112)
  Update vs branch for 16.1
  Fix struct layout error when nullable enabled: (dotnet#34128)
  Remove IgnoreInsignificantNullableModifiersDifference (dotnet#34096)
  Add the correct nullable annotations to generated iterator code (dotnet#33986)
  Move Rename implementation to new fully loaded document API.
  handle encapsulate field command
  change the way extract method handle partial load
  handle orangize document
  Add back Go to definition method.
  Revert "Move Go to definition to new fully loaded document API."
  Revert "Move Find references implementation to new fully loaded document API."
  Move Find references implementation to new fully loaded document API.
  Track nullable state across boxing conversions (dotnet#34087)
  ...
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