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

Allow parameter renames in Edit and Continue #54856

Merged
merged 15 commits into from
Aug 10, 2021
Merged

Allow parameter renames in Edit and Continue #54856

merged 15 commits into from
Aug 10, 2021

Conversation

davidwengier
Copy link
Contributor

@davidwengier davidwengier commented Jul 16, 2021 •
edited
Loading

Fixes #52563
Fixes #48635

@davidwengier davidwengier requested review from a team as code owners July 16, 2021 01:51
@davidwengier davidwengier force-pushed the EnCRenameParameters branch from 00771cc to 95b2551 Compare July 20, 2021 08:12
@davidwengier davidwengier requested a review from a team as a code owner July 23, 2021 00:10
@davidwengier davidwengier force-pushed the EnCRenameParameters branch from 27c9822 to 5705a3b Compare July 26, 2021 03:56
@davidwengier davidwengier force-pushed the EnCRenameParameters branch from fb75822 to 93aa07c Compare July 28, 2021 02:06
@davidwengier
Copy link
Contributor Author

@tmat PTAL

Now that the other PR is in, this is pretty straight forward

@davidwengier davidwengier mentioned this pull request Jul 30, 2021
Merged
@lambdageek lambdageek mentioned this pull request Jul 30, 2021
Closed
tmat
tmat requested changes Jul 30, 2021
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.

🕐

cston
cston approved these changes Jul 30, 2021
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.

Compiler changes LGTM.

@davidwengier
Copy link
Contributor Author

Worked out my symbol issues, the tests were bad and I feel bad. Updated with all feedback addressed now.

@RikkiGibson RikkiGibson self-assigned this Aug 4, 2021
tmat
tmat reviewed Aug 4, 2021
View reviewed changes
src/Compilers/CSharp/Test/Emit/Emit/EditAndContinue/EditAndContinueTests.cs
@@ -1404,6 +1495,10 @@ class C
EncValidation.VerifyModuleMvid(2, reader1, reader2);

CheckNames(readers, reader2.GetTypeDefNames());
CheckNames(readers, reader2.GetMethodDefNames(), "F");
CheckNames(readers, reader2.GetParameterDefNames(), "", "arg");
Copy link
Member

Choose a reason for hiding this comment

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

""

Curious, where does this empty name come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The method return

tmat
tmat approved these changes Aug 5, 2021
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
Copy link
Contributor Author

Ping @dotnet/roslyn-compiler for a second review

@jaredpar
Copy link
Member

jaredpar commented Aug 9, 2021

@RikkiGibson PTAL

RikkiGibson
RikkiGibson approved these changes Aug 10, 2021
View reviewed changes
src/Features/Core/Portable/EditAndContinue/AbstractEditAndContinueAnalyzer.cs
@@ -5260,7 +5286,7 @@ private static bool TryMapParameter((SyntaxNode? Node, int Ordinal) parameterKey
var newLambdaSymbol = GetLambdaExpressionSymbol(newModel, newLambda, cancellationToken);

// signature validation:
if (!ParametersEquivalent(oldLambdaSymbol.Parameters, newLambdaSymbol.Parameters, exact: false))
if (!ParameterTypesEquivalent(oldLambdaSymbol.Parameters, newLambdaSymbol.Parameters, exact: false))
Copy link
Contributor

Choose a reason for hiding this comment

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

I was confused by this method name. Don't we also consider a change to the parameter's ref kind a rude edit? Do we have existing tests for such scenarios?

Copy link
Member

@tmat tmat Aug 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

RefKind is part of parameter type.

@davidwengier davidwengier merged commit 9683739 into dotnet:main Aug 10, 2021
@davidwengier davidwengier deleted the EnCRenameParameters branch August 10, 2021 21:20
@ghost ghost added this to the Next milestone Aug 10, 2021
@jaredpar jaredpar mentioned this pull request Aug 17, 2021
Open
@dibarbet dibarbet modified the milestones: Next, 17.0.P4 Aug 31, 2021
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

@cston cston cston approved these changes

@RikkiGibson RikkiGibson RikkiGibson approved these changes

Assignees

@RikkiGibson RikkiGibson

Labels
Area-Compilers Area-Interactive
Projects
None yet
Milestone
17.0.P4
Development

Successfully merging this pull request may close these issues.

EnC - Allow parameter renames EnC: Updating lambda parameters to discards breaks EnC
7 participants
@davidwengier @jaredpar @tmat @RikkiGibson @cston @dibarbet @jcouv