Skip to content

EnC: Include added types in changed types of emit result#55099

Merged
tmat merged 4 commits intodotnet:mainfrom
tmat:ChangedTypes
Jul 26, 2021
Merged

EnC: Include added types in changed types of emit result#55099
tmat merged 4 commits intodotnet:mainfrom
tmat:ChangedTypes

Conversation

@tmat
Copy link
Copy Markdown
Member

@tmat tmat commented Jul 23, 2021 •
edited
Loading

The types are reported to the application via runtime event. The application needs to know about added types, not just updated ones.

+ Simplify some EnC tests.

@tmat tmat requested review from a team as code owners July 23, 2021 23:32
@ghost ghost added the Area-Compilers label Jul 23, 2021
@tmat
Copy link
Copy Markdown
Member Author

tmat commented Jul 23, 2021

@davidwengier @cston PTAL

tmat
tmat commented Jul 23, 2021
View reviewed changes
Comment thread src/Compilers/CSharp/Test/Emit/Emit/EditAndContinue/EditAndContinueTests.cs
[WorkItem(962219, "http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/962219")]
[Fact]
public void AddMethod_WithAttributes()
public void PartialMethod()
Copy link
Copy Markdown
Member Author

@tmat tmat Jul 23, 2021

Choose a reason for hiding this comment

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

PartialMethod

Moved

@tmat
Copy link
Copy Markdown
Member Author

tmat commented Jul 23, 2021

@dotnet/roslyn-compiler

@tmat tmat changed the title Include added types in changed types of emit result EnC: Include added types in changed types of emit result Jul 23, 2021
tmat
tmat commented Jul 26, 2021
View reviewed changes
Comment thread src/Compilers/Core/Portable/Emit/EditAndContinue/DeltaMetadataWriter.cs
{
case SymbolChange.Added:
_typeDefs.Add(typeDef);
_changedTypeDefs.Add(typeDef);
Copy link
Copy Markdown
Member Author

@tmat tmat Jul 26, 2021

Choose a reason for hiding this comment

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

_changedTypeDefs.Add(typeDef);

Significant change here, other changes are just renames.

jcouv
jcouv reviewed Jul 26, 2021
View reviewed changes
Comment thread src/Compilers/Core/Portable/PublicAPI.Unshipped.txt
abstract Microsoft.CodeAnalysis.SyntaxTree.GetLineMappings(System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) -> System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.LineMapping>!
const Microsoft.CodeAnalysis.WellKnownMemberNames.PrintMembersMethodName = "PrintMembers" -> string!
Microsoft.CodeAnalysis.Compilation.EmitDifference(Microsoft.CodeAnalysis.Emit.EmitBaseline! baseline, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.Emit.SemanticEdit>! edits, System.Func<Microsoft.CodeAnalysis.ISymbol!, bool>! isAddedSymbol, System.IO.Stream! metadataStream, System.IO.Stream! ilStream, System.IO.Stream! pdbStream, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) -> Microsoft.CodeAnalysis.Emit.EmitDifferenceResult!
Microsoft.CodeAnalysis.Emit.EmitDifferenceResult.ChangedTypes.get -> System.Collections.Immutable.ImmutableArray<System.Reflection.Metadata.TypeDefinitionHandle>
Copy link
Copy Markdown
Member

@jcouv jcouv Jul 26, 2021

Choose a reason for hiding this comment

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

Tagging @333fred since affects public API

jcouv
jcouv reviewed Jul 26, 2021
View reviewed changes
Comment thread src/Compilers/Core/Portable/Emit/EditAndContinue/DeltaMetadataWriter.cs Outdated
jcouv
jcouv reviewed Jul 26, 2021
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Emitter/EditAndContinue/EmitHelpers.cs Outdated
jcouv
jcouv reviewed Jul 26, 2021
View reviewed changes
Comment thread src/Compilers/VisualBasic/Portable/Emit/EditAndContinue/EmitHelpers.vb Outdated
jcouv
jcouv reviewed Jul 26, 2021
View reviewed changes
Comment thread src/Features/Core/Portable/EditAndContinue/EditSession.cs Outdated
@jcouv
Copy link
Copy Markdown
Member

jcouv commented Jul 26, 2021 

                        updatedTypeTokens,

I'm not familiar with ManagedModuleUpdate, but it expects UpdatedTypes. Is it okay to give it a set of changed types? (should that type be adjusted too?)

Refers to: src/Features/Core/Portable/EditAndContinue/EditSession.cs:783 in 49bb078. [](commit_id = 49bb078, deletion_comment = False)

@tmat
Copy link
Copy Markdown
Member Author

tmat commented Jul 26, 2021

Is it okay to give it a set of changed types? (should that type be adjusted too?)

Yes, ManagedModuleUpdate just communicates the types to the runtime. Renaming it is more complicated so we'll leave that for a separate PR.

jcouv
jcouv reviewed Jul 26, 2021
View reviewed changes
Comment thread src/Compilers/Core/Portable/Emit/EditAndContinue/DeltaMetadataWriter.cs
if (!_methodDefs.IsAddedNotChanged(def) && def.GetBody(Context)?.SequencePoints.Length > 0)
{
methods.Add(MetadataTokens.MethodDefinitionHandle(_methodDefs[def]));
methods.Add(MetadataTokens.MethodDefinitionHandle(_methodDefs.GetRowId(def)));
Copy link
Copy Markdown
Member

@jcouv jcouv Jul 26, 2021

Choose a reason for hiding this comment

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

📝 indexer was replaced with a GetRowId method

jcouv
jcouv reviewed Jul 26, 2021
View reviewed changes
Comment thread src/Compilers/Core/Portable/Emit/EditAndContinue/DeltaMetadataWriter.cs
protected override ITypeDefinition GetTypeDef(TypeDefinitionHandle handle)
{
return _typeDefs[MetadataTokens.GetRowNumber(handle)];
return _typeDefs.GetDefinition(MetadataTokens.GetRowNumber(handle));
Copy link
Copy Markdown
Member

@jcouv jcouv Jul 26, 2021

Choose a reason for hiding this comment

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

📝 indexer replaced with GetDefinition method

jcouv
jcouv reviewed Jul 26, 2021
View reviewed changes
Comment thread src/Compilers/Core/Portable/Emit/EditAndContinue/DeltaMetadataWriter.cs
jcouv
jcouv reviewed Jul 26, 2021
View reviewed changes
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.

Done with review pass (iteration 3). Will look at test changes after LDM (in 2 hours)

@jcouv
Copy link
Copy Markdown
Member

jcouv commented Jul 26, 2021 

                        updatedTypeTokens,

What was the resolution for this question?

In reply to: 886862232

Refers to: src/Features/Core/Portable/EditAndContinue/EditSession.cs:783 in 49bb078. [](commit_id = 49bb078, deletion_comment = False)

jcouv
jcouv approved these changes Jul 26, 2021
View reviewed changes
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 4)

@tmat
Copy link
Copy Markdown
Member Author

tmat commented Jul 26, 2021 

                        updatedTypeTokens,

Yes, it's ok to give it set of all types.

In reply to: 886973377

Refers to: src/Features/Core/Portable/EditAndContinue/EditSession.cs:783 in 49bb078. [](commit_id = 49bb078, deletion_comment = False)

@RikkiGibson RikkiGibson self-assigned this Jul 26, 2021
RikkiGibson
RikkiGibson approved these changes Jul 26, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

LGTM although it's difficult for me to judge whether the changes in test baselines are correct or not.

@tmat
Copy link
Copy Markdown
Member Author

tmat commented Jul 26, 2021

@RikkiGibson Thanks!

@tmat tmat merged commit ac374af into dotnet:main Jul 26, 2021
@tmat tmat deleted the ChangedTypes branch July 26, 2021 21:02
@ghost ghost added this to the Next milestone Jul 26, 2021
@allisonchou allisonchou modified the milestones: Next, 17.0.P3 Jul 27, 2021
@commit-lint commit-lint bot mentioned this pull request Aug 5, 2021
Open
@davidwengier davidwengier mentioned this pull request Aug 25, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davidwengier davidwengier davidwengier approved these changes

@RikkiGibson RikkiGibson RikkiGibson approved these changes

@jcouv jcouv jcouv approved these changes

Assignees

@RikkiGibson RikkiGibson

Labels

Area-Compilers

Projects

None yet

Milestone

17.0.P3

Development

Successfully merging this pull request may close these issues.

5 participants

@tmat @jcouv @davidwengier @RikkiGibson @allisonchou