Skip to content

Add the correct nullable annotations to generated iterator code#33986

Merged
chsienki merged 12 commits intodotnet:masterfrom
chsienki:iterator_current_nullable
Mar 14, 2019
Merged

Add the correct nullable annotations to generated iterator code#33986
chsienki merged 12 commits intodotnet:masterfrom
chsienki:iterator_current_nullable

Conversation

@chsienki
Copy link
Copy Markdown
Member

@chsienki chsienki commented Mar 9, 2019 •
edited
Loading

This is mainly about plumbing through TypeSymbolWithAnnotations to the correct point so we can generate the attribute based on it's value.

  • Expose the IteratorElementType as a TypeSymbolWithAnnotations
  • Return the annotated type, or a default as needed from the required locations
  • Allow state machine field to take a TSWA and pass it in via the iterator rewriter
  • Fix test

Fixes #30010

@chsienki chsienki changed the title Iterator current nullable Adds the correct nullable annotations to generated iterator code Mar 11, 2019
@chsienki chsienki changed the title Adds the correct nullable annotations to generated iterator code Add the correct nullable annotations to generated iterator code Mar 11, 2019
@chsienki chsienki marked this pull request as ready for review March 11, 2019 17:38
@chsienki chsienki requested a review from a team as a code owner March 11, 2019 17:38
@chsienki
Copy link
Copy Markdown
Member Author

chsienki commented Mar 11, 2019

@dotnet/roslyn-compiler for review please

@jcouv jcouv added this to the 16.1.P1 milestone Mar 11, 2019
jaredpar
jaredpar approved these changes Mar 11, 2019
View reviewed changes
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:

cston
cston reviewed Mar 11, 2019
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Lowering/MethodToClassRewriter.cs Outdated
Copy link
Copy Markdown
Contributor

@cston cston Mar 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

TypeSymbolWithAnnotations.Create(null) [](start = 27, length = 38)

default #Closed

Copy link
Copy Markdown
Member

@jaredpar jaredpar Mar 12, 2019 •
edited by chsienki
Loading

Choose a reason for hiding this comment

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

Rather that default should we add a static property with a decent name: Missing, Null, etc ...

cston
cston reviewed Mar 11, 2019
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Symbols/MethodSymbol.cs Outdated
Copy link
Copy Markdown
Contributor

@cston cston Mar 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

TypeSymbolWithAnnotations.Create(null) [](start = 25, length = 38)

default #Closed

cston
cston reviewed Mar 11, 2019
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Symbols/Source/LocalFunctionSymbol.cs Outdated
Copy link
Copy Markdown
Contributor

@cston cston Mar 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

_iteratorElementType = value; [](start = 16, length = 29)

The assignment is not thread-safe.

_iteratorElementType should probably be a TypeSymbolWithAnnotations.Builder.

Then this property would be:

get => _iteratorElementType.ToType();
set => _iteratorElementType.InterlockedInitialize(value);
``` #Closed

cston
cston reviewed Mar 11, 2019
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberMethodSymbol.cs Outdated
Copy link
Copy Markdown
Contributor

@cston cston Mar 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

_iteratorElementType = value [](start = 16, length = 28)

The assignment is not thread-safe. #Closed

@chsienki
Copy link
Copy Markdown
Member Author

chsienki commented Mar 14, 2019

@dotnet/roslyn-compiler for second review please

@chsienki chsienki force-pushed the iterator_current_nullable branch from 0bab959 to 2b3f148 Compare March 14, 2019 19:03
@chsienki
Copy link
Copy Markdown
Member Author

chsienki commented Mar 14, 2019

Rebase onto master and fixed up the errors.

I also renamed IteratorElementType to IteratorElementTypeWithAnnotations to be consistent with @gafter's changes

@chsienki chsienki merged commit e391828 into dotnet:master Mar 14, 2019
333fred added a commit to 333fred/roslyn that referenced this pull request Mar 19, 2019
@333fred
Merge remote-tracking branch 'dotnet/master' into merge-master
755f277 
* 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

Reviewers

@jaredpar jaredpar jaredpar approved these changes

+1 more reviewer

@cston cston cston approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-Compilers Feature - Nullable Reference Types Nullable Reference Types

Projects

None yet

Milestone

16.1.P1

Development

Successfully merging this pull request may close these issues.

4 participants

@chsienki @jaredpar @cston @jcouv