Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 4 commits into from
Jul 26, 2021
Merged

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

merged 4 commits into from
Jul 26, 2021

Conversation

tmat
Copy link
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
@tmat
Copy link
Member Author

tmat commented Jul 23, 2021

@davidwengier @cston PTAL

tmat
tmat commented Jul 23, 2021
View reviewed changes
src/Compilers/CSharp/Test/Emit/Emit/EditAndContinue/EditAndContinueTests.cs
[Fact]
public void AddMethod_WithAttributes()
public void PartialMethod()
Copy link
Member Author

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
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
src/Compilers/Core/Portable/Emit/EditAndContinue/DeltaMetadataWriter.cs
@@ -482,6 +486,8 @@ protected override void CreateIndicesForNonTypeMembers(ITypeDefinition typeDef)
{
case SymbolChange.Added:
_typeDefs.Add(typeDef);
_changedTypeDefs.Add(typeDef);
Copy link
Member Author

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
src/Compilers/Core/Portable/PublicAPI.Unshipped.txt
@@ -2,8 +2,8 @@
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
Member

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
Copy link
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
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
src/Compilers/Core/Portable/Emit/EditAndContinue/DeltaMetadataWriter.cs
@@ -262,19 +266,19 @@ public void GetUpdatedMethodTokens(ArrayBuilder<MethodDefinitionHandle> methods)
// The debugger tries to remap all modified methods, which requires presence of sequence points.
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
Member

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
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
Member

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
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.

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

@jcouv
Copy link
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
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
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
Contributor

@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
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

@jcouv jcouv jcouv approved these changes

@RikkiGibson RikkiGibson RikkiGibson approved these changes

@davidwengier davidwengier davidwengier 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