Add a code refactoring to convert local function to method by alrz · Pull Request #16808 · dotnet/roslyn

alrz · 2017-01-27T23:51:37Z

Fixes #16288

alrz · 2017-01-28T06:06:19Z

Data-flow analysis fails for no appearant reason. Any thoughts?

CyrusNajmabadi · 2017-01-29T03:58:47Z

@agocke May know more about data-flow.

agocke · 2017-01-29T04:00:47Z

@alrz Local functions currently aren't supported in region analysis. #14214 tracks fixing it.

alrz · 2017-09-13T05:12:30Z

Since data-flow analysis on local functions is back, I synced the PR and added more tests. This is still a work-in-progress (see the opening description). Any early feedback is appreciated, though. thanks.

/cc @sharwell

alrz · 2017-09-20T16:23:23Z

~~Due to capturing, we might end up changing the behavior of the code e.g.~~ examples are identical.

object M(object arg)
{
  object Local() => arg;
  arg = null;
  return Local();
}

->

object M(object arg)
{
  arg = null;
  return Local(arg);
}

private static object Local(object arg) => arg;

Do we want to not suggest the refactoring in this case? Thoughts?

CyrusNajmabadi · 2017-09-21T21:49:55Z

It's a refactoring. So i feel they should be available to the user as their absence can be confusing. Since we're not going to be doin ga fix-all here it's not a problem for me. I do think that it would make sense to give a warning.

DustinCampbell

Just took a brief look through.

DustinCampbell · 2017-09-22T15:28:03Z

I'd love to see the type parameters reduced to just the ones that are used.

DustinCampbell · 2017-09-22T15:31:58Z

What about parameters from enclosing methods that are captured?

I expect this return those too! I'll add a test.

DustinCampbell · 2017-09-22T15:32:44Z

I'd recommend typeParameterList since there's a method.Decl.ParameterList property.

DustinCampbell · 2017-09-22T15:39:55Z

Again, typeParameterList is probably better for more readable code.

alrz · 2017-11-18T21:38:48Z

@sharwell @CyrusNajmabadi would love to hear your feedback on this. made progress but the code needs serious cleanup. thanks.

CyrusNajmabadi · 2017-11-18T23:50:54Z

Looking.

CyrusNajmabadi · 2017-11-18T23:52:21Z

Not a huge fan of this. Specifically, that it makes this code different than the rest of our features. I'd prefer another PR that changes the features layer to use this pattern FWIW.

CyrusNajmabadi · 2017-11-18T23:52:45Z

perhaps offer on the name of the local function as well.

I'll add test for [||]LocalName(), Local[||]Name(), [|LocalName|]() and LocalName[||](). And negative case for other places.

alrz · 2018-05-11T14:12:22Z

@jcouv This is no longer blocked. Can you please remove the label so I can get a review? Thanks.

jcouv · 2018-05-11T14:59:27Z

Removed blocked label.

CyrusNajmabadi · 2018-05-11T16:56:47Z

+    <value>Convert to regular method</value>
+  </data>
+  <data name="Convert_to_method" xml:space="preserve">
+    <value>Convert to method</value>


what is the difference between a method and a regular method?

Wait, I recall reverting to "Convert to method". The previous string is not supposed to be there.

CyrusNajmabadi · 2018-05-11T16:58:22Z

+                return;
+            }
+
+            context.RegisterRefactoring(new MyCodeAction(c => UpdateDocumentAsync(root, document, parentBlock, localFunction, c)));


alrz · 2018-06-02T11:41:12Z

@jinujoseph @sharwell What is the state of this PR? I haven't heard back from the team. please let me know if this needs any further work.

sharwell

This PR has an extraneous value is the resources file that needs to be removed, but is otherwise ready once tests pass and any merge conflicts are addressed.

sharwell · 2018-06-05T16:10:28Z

  <data name="Convert_to_for" xml:space="preserve">
    <value>Convert to 'for'</value>
  </data>
+  <data name="Convert_to_regular_method" xml:space="preserve">


❗️ Need to remove this extraneous value

# Conflicts: # src/Features/CSharp/Portable/CSharpFeaturesResources.resx

alrz · 2018-06-05T22:30:32Z

There's an oddity with flow analysis and structs in which they are decided to be written into even if they are not (e.g. when they are only dotted-off). @sharwell, do you know why is that the case?

jinujoseph · 2018-06-06T07:14:58Z

cc @mavasani

sharwell · 2018-08-02T15:44:31Z

There's an oddity with flow analysis and structs in which they are decided to be written into even if they are not (e.g. when they are only dotted-off).

Instance properties and methods in a value type can reassign this.

dnfclas added the cla-already-signed label Jan 27, 2017

Pilchie added the Area-IDE label Jan 30, 2017

Pilchie added this to the 2.1 milestone Jan 30, 2017

agocke changed the base branch from post-dev15 to master February 14, 2017 23:24

jaredpar modified the milestones: 15.1, 15.later Jun 27, 2017

sharwell added Community The pull request was submitted by a contributor who is not a Microsoft employee. and removed Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Aug 31, 2017

Pilchie assigned sharwell Sep 8, 2017

Pilchie requested review from a team and kuhlenh September 8, 2017 15:15

Pilchie added the Feature Specification label Sep 8, 2017

alrz force-pushed the local-func branch from e1792ab to 37f436f Compare September 12, 2017 21:12

DustinCampbell reviewed Sep 22, 2017

View reviewed changes

alrz force-pushed the local-func branch 2 times, most recently from 486d5f7 to f53bcb2 Compare September 22, 2017 17:40

alrz force-pushed the local-func branch from f53bcb2 to 91eabc0 Compare November 18, 2017 21:33

alrz force-pushed the local-func branch from 91eabc0 to aefdb28 Compare November 18, 2017 22:06

CyrusNajmabadi reviewed Nov 18, 2017

View reviewed changes

alrz added 7 commits April 6, 2018 02:48

Use static field to avoid unnecessary closures

23c4fd3

Renamings.

153fc56

Change localized string to 'Convert to method'

34e9f93

Add test for mutating structs

f7019bb

Exclude local function parameters from capture set

0486531

Use CapturedInside

0e8fc64

Update resources

fc77418

alrz force-pushed the local-func branch from e1783a1 to fc77418 Compare April 6, 2018 00:38

Revert code.

bfb9eda

jcouv removed the Blocked label May 11, 2018

CyrusNajmabadi reviewed May 11, 2018

View reviewed changes

sharwell approved these changes May 25, 2018

View reviewed changes

sharwell suggested changes Jun 5, 2018

View reviewed changes

alrz added 3 commits June 5, 2018 20:47

Merge remote-tracking branch 'origin/master' into local-func

6aeb657

# Conflicts: # src/Features/CSharp/Portable/CSharpFeaturesResources.resx

Update resources

2ab8ef4

Add test for nested captures

b6650b7

jinujoseph assigned mavasani Jun 19, 2018

jinujoseph modified the milestones: 15.8, 16.0 Jun 26, 2018

sharwell approved these changes Aug 2, 2018

View reviewed changes

Merge branch 'master' into local-func

28a92ea

sharwell merged commit f251e7a into dotnet:master Aug 2, 2018

alrz deleted the local-func branch August 2, 2018 17:56

Conversation

alrz commented Jan 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alrz commented Jan 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented Jan 29, 2017

Uh oh!

agocke commented Jan 29, 2017

Uh oh!

alrz commented Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alrz commented Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented Sep 21, 2017

Uh oh!

DustinCampbell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alrz commented Nov 18, 2017

Uh oh!

CyrusNajmabadi commented Nov 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alrz Nov 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alrz commented May 11, 2018

Uh oh!

jcouv commented May 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alrz commented Jun 2, 2018

Uh oh!

sharwell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alrz commented Jun 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jinujoseph commented Jun 6, 2018

Uh oh!

sharwell commented Aug 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

alrz commented Jan 27, 2017 •

edited

Loading

alrz commented Jan 28, 2017 •

edited

Loading

alrz commented Sep 13, 2017 •

edited

Loading

alrz commented Sep 20, 2017 •

edited

Loading

alrz Nov 19, 2017 •

edited

Loading

alrz commented Jun 5, 2018 •

edited

Loading

sharwell commented Aug 2, 2018 •

edited

Loading