Skip to content

[EnC] Allow renaming methods, properties, events etc.#62364

Merged
davidwengier merged 12 commits intodotnet:mainfrom
davidwengier:EnCRenames
Jul 21, 2022
Merged

[EnC] Allow renaming methods, properties, events etc.#62364
davidwengier merged 12 commits intodotnet:mainfrom
davidwengier:EnCRenames

Conversation

@davidwengier
Copy link
Member

@davidwengier davidwengier commented Jul 5, 2022 •
edited
Loading

Part of #59264

Includes #62151 so just review eba47b0 (#62364) or wait until that merges

Also fixes #51011 because after this change, the incorrect edits were just silly.

@ghost ghost added the Area-Compilers label Jul 5, 2022
@davidwengier davidwengier changed the title [En] [EnC] Allow renaming methods, properties, events etc. Jul 5, 2022
@davidwengier davidwengier requested a review from tmat July 5, 2022 02:35
@davidwengier davidwengier marked this pull request as ready for review July 5, 2022 02:35
@davidwengier davidwengier requested review from a team as code owners July 5, 2022 02:35
@davidwengier davidwengier mentioned this pull request Jul 6, 2022
Open
@davidwengier
Copy link
Member Author

davidwengier commented Jul 6, 2022

Rather than create a third PR with mostly the same commits, I've cheekily added active statement checking to this one. Sorry, not sorry 😛

davidwengier
davidwengier commented Jul 10, 2022
View reviewed changes
src/Compilers/Core/Portable/Emit/EditAndContinue/DeltaMetadataWriter.cs
var synthesizedMembers = (_previousGeneration.Ordinal == 0) ? module.GetAllSynthesizedMembers() : _previousGeneration.SynthesizedMembers;

Debug.Assert(module.EncSymbolChanges is not null);
var deletedMembers = (_previousGeneration.Ordinal == 0) ? module.EncSymbolChanges.GetAllDeletedMethods() : _previousGeneration.DeletedMembers;
Copy link
Member Author

@davidwengier davidwengier Jul 10, 2022

Choose a reason for hiding this comment

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

I didn't do this originally because it is not needed, as it is used when re-adding deleted members, and its impossible to do that in generation 1, but without it the DeletedMembers property of the EmitBaseline doesn't represent the truth for generation 1, which was annoying for tests.

@davidwengier
Copy link
Member Author

davidwengier commented Jul 12, 2022

Ping @tmat for review

1 similar comment
@davidwengier
Copy link
Member Author

davidwengier commented Jul 15, 2022

Ping @tmat for review

@tmat
Copy link
Member

tmat commented Jul 15, 2022

Could you pls rebase, so that this PR only contains new changes?

@tmat
Copy link
Member

tmat commented Jul 15, 2022 

        foreach (var pair in previousMembers)

nit: can use deconstruction

Refers to: src/Compilers/Core/Portable/Emit/EditAndContinue/SymbolMatcher.cs:178 in 85d4b0f. [](commit_id = 85d4b0f, deletion_comment = False)

tmat
tmat reviewed Jul 15, 2022
View reviewed changes
tmat
tmat reviewed Jul 15, 2022
View reviewed changes
@davidwengier
Copy link
Member Author

davidwengier commented Jul 15, 2022

Could you pls rebase, so that this PR only contains new changes?

Sure thing, though unless you look commit-by-commit the diff should be right.. at least in GitHub it is, maybe CodeFlow gets confused?

@tmat
Copy link
Member

tmat commented Jul 15, 2022

Yeah, might be CodeFlow. I see some changes that don't seem to belong.

@davidwengier
Copy link
Member Author

davidwengier commented Jul 15, 2022

Rebased and force pushed. If github ends up hiding your comments, rest assured they're saved in my Outlook :D

@tmat
Copy link
Member

tmat commented Jul 15, 2022

Reviewed up to commit 10.

@davidwengier
Copy link
Member Author

davidwengier commented Jul 16, 2022

Added commit 11 to keep you on your toes!

@davidwengier
Copy link
Member Author

davidwengier commented Jul 21, 2022

The commits will continue until morale improves (or @tmat reviews the PR)

tmat
tmat approved these changes Jul 21, 2022
View reviewed changes
Copy link
Member

@tmat tmat left a comment

Choose a reason for hiding this comment

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

:shipit:

@davidwengier davidwengier merged commit 26a60d7 into dotnet:main Jul 21, 2022
@davidwengier davidwengier deleted the EnCRenames branch July 21, 2022 12:23
@ghost ghost added this to the Next milestone Jul 21, 2022
@Youssef1313 Youssef1313 mentioned this pull request Jul 21, 2022
Closed
333fred added a commit to 333fred/roslyn that referenced this pull request Jul 21, 2022
@333fred
Merge remote-tracking branch 'upstream/main' into forbid-inumber
230897a 
* upstream/main: (53 commits)
  Remove 'mangleName' parameter in PENamedTypeSymbolNonGeneric (dotnet#62813)
  Produce errors for 'partial file record' (dotnet#62686)
  [EnC] Allow renaming methods, properties, events etc. (dotnet#62364)
  Update AbstractFileHeaderDiagnosticAnalyzer.cs
  Rename file
  Make static
  Move tests
  Refactor
  Support ref field assignment in object initializers (dotnet#62584)
  Fix cref tags on generated VB's SyntaxFactory (dotnet#62578)
  Simplify
  Simplify
  Simplify
  Simplify
  Simplify
  Simplify
  Simplify
  Simplify
  Move helper methods
  Remove serialization
  ...
@Youssef1313 Youssef1313 mentioned this pull request Jul 21, 2022
Merged
@RikkiGibson RikkiGibson mentioned this pull request Jul 21, 2022
Closed
This was referenced Jul 26, 2022
Closed
Closed
@allisonchou allisonchou modified the milestones: Next, 17.4 P1 Jul 26, 2022
@ryzngard ryzngard mentioned this pull request Jul 27, 2022
Merged
@tmat tmat mentioned this pull request Oct 4, 2023
Open
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tmat tmat tmat approved these changes

Assignees

No one assigned

Labels

Area-Compilers

Projects

None yet

Milestone

17.4 P1

Development

Successfully merging this pull request may close these issues.

EnC: member moves across partial definitions might be classified as renamed and not handled correctly

3 participants

@davidwengier @tmat @allisonchou