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.

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

Fix Extract Method in methods containing local functions outside the region #18424

Closed
wants to merge 2 commits into from

Conversation

sharwell
Copy link
Member

@sharwell sharwell commented Apr 4, 2017

Fixes #17165
Fixes #18347

Tagging @dotnet/roslyn-ide and @agocke for review.

@@ -75,6 +75,20 @@ protected override void EnterRegion()
base.EnterRegion();
}

protected override void NoteRead(Symbol variable, ParameterSymbol rangeVariableUnderlyingParameter)
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 we'll need some tests that validate thsi at the compiler level.

@CyrusNajmabadi
Copy link
Member

A lot of test failures here. Is the new behavior expected/desirable?

@sharwell
Copy link
Member Author

sharwell commented Apr 4, 2017

A lot of test failures here. Is the new behavior expected/desirable?

The first category of failures was due to accidental omission of a guard to prevent fields from being reported as data-out variables in the new NoteRead override. This has been corrected in e89161b by amending the previous comment.

I've convinced myself that the other failures are not desired behavior (a change in the model for code that fails to compile), so I'm working to restore the original behavior in these cases.

@jasonmalinowski jasonmalinowski removed their request for review April 5, 2017 19:46
@sharwell sharwell added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Apr 23, 2017
@agocke
Copy link
Member

agocke commented Jun 4, 2017

@sharwell I've looked at the issues and this change and I don't think this is the right change. I've created a PR with what I think is the proper change at #20004

@sharwell sharwell closed this Jun 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE cla-already-signed PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants