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] Allow renaming methods, properties, events etc. #62364

Merged
merged 12 commits into from
Jul 21, 2022
Merged

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

merged 12 commits into from
Jul 21, 2022

Conversation

davidwengier
Copy link
Contributor

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

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

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
@@ -190,6 +190,9 @@ internal EmitBaseline GetDelta(Compilation compilation, Guid encId, MetadataSize
// otherwise members from the current compilation have already been merged into the baseline.
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
Contributor Author

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
Contributor Author

Ping @tmat for review

1 similar comment
@davidwengier
Copy link
Contributor Author

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)

@davidwengier
Copy link
Contributor Author

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
Contributor Author

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
Contributor Author

Added commit 11 to keep you on your toes!

@davidwengier
Copy link
Contributor Author

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