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

Support changing return types of methods, properties, events #63486

Merged
merged 19 commits into from
Sep 13, 2022
Merged

Support changing return types of methods, properties, events #63486

merged 19 commits into from
Sep 13, 2022

Conversation

davidwengier
Copy link
Contributor

No description provided.

@davidwengier davidwengier requested a review from tmat August 22, 2022 03:35
@davidwengier davidwengier changed the title Support changing return types of methods Support changing return types of methods, properties, events Aug 22, 2022
@davidwengier davidwengier marked this pull request as ready for review August 22, 2022 08:17
@davidwengier davidwengier requested review from a team as code owners August 22, 2022 08:17
@davidwengier davidwengier mentioned this pull request Aug 23, 2022
Open
@davidwengier davidwengier mentioned this pull request Aug 29, 2022
Merged
@davidwengier
Copy link
Contributor Author

So this got a lot bigger. Sorry!

The TL;DR is that whilst just pretending properties and events were the same as methods for deletes, for changes that didn't hold up, and we have to properly track them as deleted members, and issue inserts for the actual property/event not just the accessors.

tmat
tmat reviewed Sep 9, 2022
View reviewed changes
src/Compilers/Core/Portable/Emit/EditAndContinue/DeltaMetadataWriter.cs Outdated
// If this is a field that is being added, but it's part of a property or event that has been deleted
// and is now being re-added, we don't want to add the field twice, so we ignore the change.
// Unlike properties and methods, since we can't replace a field with a MissingMethodException
// we don't need to update it at all.
Copy link
Member

Choose a reason for hiding this comment

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

We'll want to update the field's name to "_Deleted" at some point (see dotnet/runtime#75154)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't support deleting fields at all yet, so the comment could definitely have a "for now" modifier.

I did see that issue though, and it's not clear to me whether we would want our deleted symbols here to have the name "_Deleted" or if we'd just write the name to the metadata table as "_Deleted", to ensure that we can still find the symbols in order to "undelete" them later.. or if thats just an implementation detail of Roslyn.. or if indeed that matters at all 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

I think that's impl detail. If we remember the token of the deleted member and can update it back to a new name when re-added/renamed back it doesn't matter much if the symbol has the _Deleted name or the metadata writer special cases deleted members.

@tmat
Copy link
Member

tmat commented Sep 9, 2022

Reviewed (commit 16)

tmat
tmat reviewed Sep 12, 2022
View reviewed changes
src/Compilers/Core/Portable/Emit/EditAndContinue/DeltaMetadataWriter.cs Outdated
foreach (var eventDef in typeDef.GetEvents(this.Context))
{
if (!_eventMap.Contains(typeRowId))
{
_eventMap.Add(typeRowId);
}

this.AddDefIfNecessary(_eventDefs, eventDef);
var eventChange = _changes.GetChange(eventDef);
eventChange = _changes.FixSymbolChangeForReAddedMembers(eventDef, eventChange, DefinitionExistsInAnyPreviousGeneration);
Copy link
Member

Choose a reason for hiding this comment

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

FixSymbolChangeForReAddedMembers

Seems like each call to FixSymbolChangeForReAddedMembers is preceded by _changes.GetChange, so we can move GetChange to FixSymbolChangeForReAddedMembers and rename it to GetSymbolChangeForReAddedMembers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that should work. I originally tried to put all the logic in GetChange itself, so everything just worked magically, but that broke too many other things

tmat
tmat reviewed Sep 12, 2022
View reviewed changes
src/Compilers/Core/Portable/Emit/EditAndContinue/DeltaMetadataWriter.cs Outdated
foreach (var eventDef in typeDef.GetEvents(this.Context))
{
if (!_eventMap.Contains(typeRowId))
{
_eventMap.Add(typeRowId);
}

this.AddDefIfNecessary(_eventDefs, eventDef);
var eventChange = _changes.GetChange(eventDef);
eventChange = _changes.FixSymbolChangeForReAddedMembers(eventDef, eventChange, DefinitionExistsInAnyPreviousGeneration);
Copy link
Member

Choose a reason for hiding this comment

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

DefinitionExistsInAnyPreviousGeneration

Maybe call this upfront and pass a boolean? It's just a couple of dictionary lookups so we are not saving much by sometimes not calling this. Also if we call it upfront we can remove the type switch since now we know what kind of member are we dealing with

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO that would make things messy. The problem is that FixSymbolChangeForReAddedMembers is a little bit recursive so we'd have to bring some of that logic out to here. Personally I like that its all in that one method.

Copy link
Member

Choose a reason for hiding this comment

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

I see, did not noticed that.

@tmat
Copy link
Member

tmat commented Sep 12, 2022

Reviewed (commit 17)

@davidwengier davidwengier enabled auto-merge (squash) September 13, 2022 05:22
@davidwengier davidwengier merged commit 065cd8a into dotnet:main Sep 13, 2022
@davidwengier davidwengier deleted the EnCReturnTypeChanges branch September 13, 2022 06:03
@ghost ghost added this to the Next milestone Sep 13, 2022
@allisonchou allisonchou mentioned this pull request Sep 14, 2022
Closed
cston
cston reviewed Sep 14, 2022
View reviewed changes
Copy link
Member

@cston cston left a comment

Choose a reason for hiding this comment

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

The compiler changes LGTM, with a few minor comments.

@davidwengier
Copy link
Contributor Author

Thanks @cston will get out a follow up today, just to close the loop

@davidwengier davidwengier mentioned this pull request Sep 15, 2022
Merged
@Cosifne Cosifne modified the milestones: Next, 17.4 P3 Sep 26, 2022
@tmat tmat added this to the 17.4 milestone May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cston cston cston left review comments

@tmat tmat tmat approved these changes

Assignees
No one assigned
Labels
Area-Interactive
Projects
None yet
Milestone
17.4
Development

Successfully merging this pull request may close these issues.
4 participants
@davidwengier @tmat @cston @Cosifne