Skip to content

Issue delete and insert edits when changing parameters#62897

Merged
davidwengier merged 7 commits intodotnet:mainfrom
davidwengier:EnCSignatureChanges
Aug 2, 2022
Merged

Issue delete and insert edits when changing parameters#62897
davidwengier merged 7 commits intodotnet:mainfrom
davidwengier:EnCSignatureChanges

Conversation

@davidwengier
Copy link
Member

Part of #59264

I was going to do return type changes with this too, but need to either change SymbolKey, or changes to the SemanticEditInfoComparer: https://github.com/dotnet/roslyn/blob/main/src/Features/Core/Portable/EditAndContinue/AbstractEditAndContinueAnalyzer.cs#L3416

@davidwengier davidwengier requested review from a team as code owners July 25, 2022 01:14
@ghost ghost added the Area-Interactive label Jul 25, 2022
@davidwengier davidwengier requested a review from tmat July 25, 2022 01:14
@davidwengier
Copy link
Member Author

@tmat PTAL

@davidwengier davidwengier changed the title Issue delete and insert edits when changing parameter types Issue delete and insert edits when parameters Jul 26, 2022
@davidwengier davidwengier changed the title Issue delete and insert edits when parameters Issue delete and insert edits when changing parameters Jul 26, 2022

// Some sanity checks
if (otherModel is null ||
containingSymbol.DeclaringSyntaxReferences.Length != 1)
Copy link
Member

Choose a reason for hiding this comment

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

containingSymbol.DeclaringSyntaxReferences.Length != 1

Does this exclude partial methods? Doc please.

Copy link
Member Author

Choose a reason for hiding this comment

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

This part wasn't a problem with partial methods, so maybe I missed something, but I added a test, and another check to make the test pass :)

}

// Now we can work out which is the old and which is the new, depending on which map we found
// the match in
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't the caller already know this?

Copy link
Member Author

@davidwengier davidwengier Jul 27, 2022

Choose a reason for hiding this comment

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

The caller knows which map the lookup will work in, yes.

@tmat
Copy link
Member

tmat commented Jul 26, 2022

Reviewed 5 commits. Looks good, just a few minor things.

@davidwengier
Copy link
Member Author

ping @tmat

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 479fdd4 into dotnet:main Aug 2, 2022
@davidwengier davidwengier deleted the EnCSignatureChanges branch August 2, 2022 22:09
@ghost ghost added this to the Next milestone Aug 2, 2022
@dibarbet dibarbet removed this from the Next milestone Sep 1, 2022
@dibarbet dibarbet added this to the 17.4 P2 milestone Sep 1, 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants