[WIP] Generating member wise initializer with local refactoring #16983

mohitathwani · 2018-06-04T15:57:18Z

Enables local refactoring on a class declaration to generate member wise initializer

mohitathwani · 2018-06-04T15:59:42Z

@harlanhaskins @nkcsgexi I have taken all of my code from the previous PR and moved it to this branch. I figured it would be a lot faster to modify and update code to suit it to solve SR-7292

harlanhaskins · 2018-06-05T23:46:17Z

include/swift/IDE/RefactoringKinds.def

@@ -50,6 +50,8 @@ CURSOR_REFACTORING(ConvertToDoCatch, "Convert To Do/Catch", convert.do.catch)

 CURSOR_REFACTORING(TrailingClosure, "Convert To Trailing Closure", trailingclosure)

+CURSOR_REFACTORING(MemberWiseInitLocalRefactoring, "Generate Member Wise Initializer", member.wise.init.local.refactoring)


I'd use either Memberwise or Member-wise. There might even be a better word than that.

mohitathwani · 2018-06-06T16:25:44Z

TODO List:

Test case for situation where no members exist in a class
Should work for structs as well
Check to see if a member-wise init already exists. If yes, do nothing.
Figure out where exactly the generated init should be placed in the source file

mohitathwani · 2018-06-06T19:53:44Z

@harlanhaskins @nkcsgexi Is it safe to assume that this generated initializer should be inserted right after the declaration of the last stored property?

mohitathwani · 2018-06-10T00:51:33Z

Thanks @harlanhaskins @nkcsgexi for the help yesterday!

I'm working on wrapping up the PR and came across an interesting condition. Yesterday we figured out out the targetLocation of the generated init by using
parentPatternBinding->getEndLoc().getAdvancedLoc(1);

and while coming up with test cases, I see that an input of

class Person {
  var firstName: String!
  var lastName: String!
  let age: Int = 2
  var address: String?
  var a = 3, b = 1234567
}

yields the output

class Person {
  var firstName: String!
  var lastName: String!
  let age: Int = 2
  var address: String?
  var a = 3, b = 1
internal init(firstName: String?, lastName: String?, address: String?) {
self.firstName = firstName
self.lastName = lastName
self.address = address
}
234567
}

Our assumption of getEndLoc() on the pattern binding is not the end of the line. So I'm wondering how to go about determining the location of where to generate the init. Maybe after the opening brace?

nkcsgexi · 2018-06-11T17:38:30Z

@mohitathwani, Could you remove the relics from previous PR (generate doc-comment boiler template)?

mohitathwani · 2018-06-12T16:21:22Z

@nkcsgexi @harlanhaskins atm I'm trying to figure out the line ending of the parent pattern binding so that I can calculate the target location properly... Any pointers?

mohitathwani · 2018-06-12T21:52:14Z

So I tried getting the Stmt from varDecl->getParentPatternStmt() thinking that I would be able to get to the end of the line that contains the pattern binding but it turn's out it returns NULL

mohitathwani · 2018-06-14T02:33:41Z

@nkcsgexi @harlanhaskins I noticed something interesting today at work. I conformed a class to a protocol and saw that Xcode gave me a auto fix it pop up saying generate protocol stub, I clicked on it and the protocol method stubs were inserted right after the opening brace on the next line. So should I also go down that route?

mohitathwani · 2018-06-19T02:18:45Z

@nkcsgexi @harlanhaskins I think I'm read for a smoke test...

nkcsgexi · 2018-06-19T17:02:48Z

Sure! @swift-ci please smoke test

mohitathwani · 2018-06-20T06:26:14Z

Updated code and all tests passing locally...

rintaro · 2018-06-20T09:32:30Z

lib/IDE/Refactoring.cpp

+                            SourceManager &SM) {
+
+  swift::SourceLoc targetLocation =
+  classDecl->getBraces().Start.getAdvancedLoc(1);


Indent.
Also, please check getBraces().Start is isValid().

rintaro · 2018-06-20T09:32:39Z

lib/IDE/Refactoring.cpp

+    if (!parentPatternBinding) continue;
+
+    auto varDeclIndex =
+    parentPatternBinding->getPatternEntryIndexForVarDecl(varDecl);


Indentation, as Rintaro mentioned above.

rintaro · 2018-06-20T09:33:22Z

lib/IDE/Refactoring.cpp

+  if (auto classDecl = canGenerateMemberwiseInit(CursorInfo)) {
+    generateMemberwiseInit(classDecl, EditConsumer, SM);
+  }
+  else {


We use } else { style.
Also, we prefer early return. In this case:

auto classDecl = canGenerateMemberwiseInit(CursorInfo) if (!classDecl) return true; generateMemberwiseInit(classDecl, EditConsumer, SM); return false;

rintaro · 2018-06-20T09:38:12Z

lib/IDE/Refactoring.cpp

+
+    if (auto init = varDecl->getParentPatternBinding()->getInit(varDeclIndex)) {
+      if (init->getStartLoc().isValid())
+      continue;


rintaro · 2018-06-20T09:38:45Z

lib/IDE/Refactoring.cpp

+    EditConsumer.accept(SM, targetLocation, "\ninternal init(");
+
+    for (size_t i = 0; i < memberTypeVector.size(); i++) {
+      EditConsumer.accept(SM, targetLocation, memberNameVector[i] + ": " + memberTypeVector[i]);


80 columns, please.

rintaro · 2018-06-20T10:08:37Z

lib/IDE/Refactoring.cpp

+ClassDecl* canGenerateMemberwiseInit(ResolvedCursorInfo CursorInfo) {
+
+  ClassDecl *classDecl = dyn_cast<ClassDecl>(CursorInfo.ValueD);
+  if (!classDecl || classDecl->getStoredProperties().empty()) {


@nkcsgexi is there a way to determine this cursor is at actually the declaration?
For example, what if typealias Alias = /* cursorinfo here->*/TargetClass and TargetClass is declared in other file?

ResolvedCursorInfo.IsRef indicates whether the cursor is on a reference or a declaration name. I think this refactoring should be only available when ResolvedCursorInfo.IsRef is false.

mohitathwani · 2018-06-21T14:50:01Z

@nkcsgexi @rintaro I've incorporated your suggested changes.

rintaro · 2018-06-22T07:24:02Z

@swift-ci Please smoke test

mohitathwani · 2018-06-22T15:09:48Z

@rintaro @nkcsgexi Since all tests passed, I went ahead and squashed all commits in to one

harlanhaskins · 2018-06-22T18:39:37Z

@swift-ci please smoke test

mohitathwani · 2018-06-22T22:20:18Z

So what's the next step now?

harlanhaskins · 2018-06-22T22:23:35Z

Seems ready to merge. Deferring to @nkcsgexi

harlanhaskins · 2018-06-26T18:09:42Z

@nkcsgexi Good to merge?

nkcsgexi · 2018-06-26T18:23:04Z

@mohitathwani @harlanhaskins I'd like to do another round of review today 😄

mohitathwani · 2018-06-26T18:51:59Z

@nkcsgexi let me know if anything needs to be done from my side...

nkcsgexi

Great progress, @mohitathwani ! Some structural comments are inline.

nkcsgexi · 2018-06-26T23:03:40Z

lib/IDE/Refactoring.cpp

+
+    std::string memberType = varDecl->getType().getString();
+    memberTypeVector.push_back(memberType);
+  }


Can we move this for-loop into canGenerateMemberwiseInit? Instead of returning ClassDecl*, canGenerateMemberwiseInit should populate memberTypeVector and memberNameVector. isApplicable returns true only when these two vectors aren't empty. This way ensures that whenever the refactoring is available, we are going to change the users' codebase after applying it.

I'm not clear about this. If I move the said for loop into canGenerateMemberwiseInit, memberTypeVector and memberNameVector will be local to that function. When performChange is called, it checks if canGeneratememberWiseInit returns a valid object and then calls generateMemberwiseInit which needs these vectors to emit the generated initializer. So my question is should we pass in references to the two vectors so that they are available out side the scope of the function and is it okay to have these vectors be populated twice?

@mohitathwani yeah, you're right! We should pass memberTypeVector and memberTypeVector as reference to canGenerateMemberwiseInit (maybe we should rename it to colllectMemberForInit. isApplicable will check whether these vectors are empty and performChange will use these two vectors for actual code change.

nkcsgexi · 2018-06-26T23:04:43Z

lib/IDE/Refactoring.cpp

+bool RefactoringActionMemberwiseInitLocalRefactoring::
+  isApplicable(ResolvedCursorInfo Tok, DiagnosticEngine &Diag) {
+  if (Tok.Kind != CursorInfoKind::ValueRef && !Tok.ValueD)
+    return false;


We can move this if statement to canGenerateMemberwiseInit too. isApplicable should be a simple wrapper of canGenerateMemberwiseInit.

mohitathwani · 2018-06-26T23:14:30Z

@nkcsgexi Thanks! Give me a day or two to get this done :)

mohitathwani · 2018-07-02T03:38:43Z

@nkcsgexi Let me know your feedback...

mohitathwani · 2018-07-05T18:06:39Z

@nkcsgexi have you had any chance to look into this?

nkcsgexi

@mohitathwani some minor comments remain. :)

nkcsgexi · 2018-07-05T18:26:20Z

lib/IDE/Refactoring.cpp

@@ -2602,6 +2602,108 @@ bool RefactoringActionLocalizeString::performChange() {
  return false;
 }

+void generateMemberwiseInit(SourceEditConsumer &EditConsumer,


mark this function as static.

nkcsgexi · 2018-07-05T18:26:31Z

lib/IDE/Refactoring.cpp

+  }
+}
+
+ClassDecl* collectMembersForInit(ResolvedCursorInfo CursorInfo,


mark this function as static.

nkcsgexi · 2018-07-05T18:27:35Z

lib/IDE/Refactoring.cpp

@@ -2602,6 +2602,108 @@ bool RefactoringActionLocalizeString::performChange() {
  return false;
 }

+void generateMemberwiseInit(SourceEditConsumer &EditConsumer,
+                            SourceManager &SM,
+                            SmallVector<std::string, 5>& memberNameVector,


Instead of using SmallVector<std::string, 5>&, use SmallVectorImpl<std::string>&.

nkcsgexi · 2018-07-05T18:27:55Z

lib/IDE/Refactoring.cpp

+void generateMemberwiseInit(SourceEditConsumer &EditConsumer,
+                            SourceManager &SM,
+                            SmallVector<std::string, 5>& memberNameVector,
+                            SmallVector<std::string, 5>& memberTypeVector,


Same here, use SmallVectorImpl<std::string>&

nkcsgexi · 2018-07-05T18:28:20Z

lib/IDE/Refactoring.cpp

+                            SourceManager &SM,
+                            SmallVector<std::string, 5>& memberNameVector,
+                            SmallVector<std::string, 5>& memberTypeVector,
+                            swift::SourceLoc targetLocation) {


We should not need swift:: here.

nkcsgexi · 2018-07-05T18:29:27Z

lib/IDE/Refactoring.cpp

+    if (!parentPatternBinding) continue;
+
+    auto varDeclIndex =
+    parentPatternBinding->getPatternEntryIndexForVarDecl(varDecl);


Indentation, as Rintaro mentioned above.

nkcsgexi · 2018-07-05T18:32:10Z

lib/IDE/Refactoring.cpp

+
+  for (auto varDecl : classDecl->getStoredProperties()) {
+    auto parentPatternBinding = varDecl->getParentPatternBinding();
+    if (!parentPatternBinding) continue;


Move continue; to the next line with proper indentation.

nkcsgexi · 2018-07-05T19:35:02Z

lib/IDE/Refactoring.cpp

+  auto classDecl = collectMembersForInit(Tok, memberNameVector,
+                                         memberTypeVector);
+
+  return classDecl && memberTypeVector.size() > 0 &&


We only need to check !memberTypeVector .empty() here to answer the isApplicable question.

nkcsgexi · 2018-07-05T19:35:30Z

lib/IDE/Refactoring.cpp

+    return true;
+
+  swift::SourceLoc bracesStart = classDecl->getBraces().Start;
+  if (!bracesStart.isValid()) return true;


Move return true; to a new line.

nkcsgexi · 2018-07-05T19:35:37Z

lib/IDE/Refactoring.cpp

+  if (!bracesStart.isValid()) return true;
+
+  swift::SourceLoc targetLocation = bracesStart.getAdvancedLoc(1);
+  if (!targetLocation.isValid()) return true;


Move return true; to a new line.

mohitathwani · 2018-07-09T02:18:20Z

@nkcsgexi please have a look when you can :)

nkcsgexi · 2018-07-10T17:05:27Z

lib/IDE/Refactoring.cpp

+bool RefactoringActionMemberwiseInitLocalRefactoring::performChange() {
+
+  if (!CursorInfo.ValueD)
+    return true;


These two lines aren't necessary. collectMembersForInit should check it.

nkcsgexi · 2018-07-10T17:10:45Z

lib/IDE/Refactoring.cpp

+
+  swift::SourceLoc targetLocation = bracesStart.getAdvancedLoc(1);
+  if (!targetLocation.isValid())
+    return true;


Move these two source loc checks into collectMembersForInit and return targetLocation instead of classDecl from collectMembersForInit. The behavior should be a non-empty memberNameVector implies valid location for inserting the initializer and we're sure to perform change in users' code.

nkcsgexi · 2018-07-10T17:11:13Z

lib/IDE/Refactoring.cpp

+  if (!classDecl)
+    return true;
+
+  swift::SourceLoc bracesStart = classDecl->getBraces().Start;


We don't need swift:: here and other places.

mohitathwani · 2018-07-12T18:56:16Z

@nkcsgexi Pushed latest changes...

@dndydon Thanks for your encouragement and your company at the Swift summit. I would recommend you read this and check out my branch on your local machine and follow along the changes I've made. I'm not sure if there are any more refactoring tools to be built, but if @nkcsgexi has some more ideas then I am happy to collaborate :)

mohitathwani · 2018-07-16T22:47:55Z

@nkcsgexi any updates?

nkcsgexi

Looking good! One minor comment :)

nkcsgexi · 2018-07-16T23:07:49Z

lib/IDE/Refactoring.cpp

+    memberTypeVector.push_back(memberType);
+  }
+
+  return targetLocation;


Can we ensure we return invalid SourceLoc if memberNameVector or memberTypeVector is empty? Currently, we may return valid location even if we don't need to insert anything.

I'm a bit confused by one line in:
bool RefactoringActionMemberwiseInitLocalRefactoring::performChange().
This line: if (targetLocation.isInvalid())
I think should be: if (targetLocation.isApplicable())

nkcsgexi · 2018-07-16T23:09:35Z

lib/IDE/Refactoring.cpp

+                            SmallVectorImpl<std::string>& memberTypeVector,
+                            SourceLoc targetLocation) {
+
+  if (memberTypeVector.size() > 0) {


Address the previous comment and we don't need to check memberTypeVector isn't empty here. Instead, we can add:

assert(!memberTypeVector.empty()); assert(memberTypeVector.size() == memberNameVector.size());

mohitathwani · 2018-07-18T02:46:12Z

@nkcsgexi Here you go...

mohitathwani · 2018-07-24T18:04:11Z

@nkcsgexi did you have a chance to look at this?

nkcsgexi · 2018-07-24T18:07:37Z

ah, sorry about dropping this. I'll take another round of review today 😄

nkcsgexi · 2018-07-24T22:34:11Z

lib/IDE/Refactoring.cpp

+
+  EditConsumer.accept(SM, targetLocation, "\ninternal init(");
+
+  for (size_t i = 0; i < memberTypeVector.size(); i++) {


Change this line to for (size_t i = 0, n = memberTypeVector.size(); i < n; i++) {. We don't need to call size() every time we evaluate the loop condition.

nkcsgexi · 2018-07-24T22:35:08Z

lib/IDE/Refactoring.cpp

+
+  collectMembersForInit(Tok, memberNameVector, memberTypeVector);
+
+  return !memberTypeVector.empty();


We can merge these two lines to: return collectMembersForInit(Tok, memberNameVector, memberTypeVector).valid();.

nkcsgexi · 2018-07-24T22:36:29Z

lib/IDE/Refactoring.cpp

+    }
+  }
+
+  EditConsumer.accept(SM, targetLocation, ") {\n\n");


We only need one "\n" here

nkcsgexi · 2018-07-24T22:36:52Z

lib/IDE/Refactoring.cpp

+                        "self." + varName + " = " + varName + "\n");
+  }
+
+  EditConsumer.accept(SM, targetLocation, "\n}\n");


We don't need the first "\n" in the string literal.

nkcsgexi · 2018-07-24T22:37:38Z

lib/IDE/Refactoring.cpp

+isApplicable(ResolvedCursorInfo Tok, DiagnosticEngine &Diag) {
+
+  SmallVector<std::string, 5> memberNameVector;
+  SmallVector<std::string, 5> memberTypeVector;


Use SmallVector<std::string, 8> instead of SmallVector<std::string, 5> here and other places.

nkcsgexi · 2018-07-24T22:38:01Z

lib/IDE/Refactoring.cpp

+bool RefactoringActionMemberwiseInitLocalRefactoring::performChange() {
+
+  SmallVector<std::string, 5> memberNameVector;
+  SmallVector<std::string, 5> memberTypeVector;


Use SmallVector<std::string, 8> instead of SmallVector<std::string, 5> here and other places.

nkcsgexi · 2018-07-24T22:42:50Z

test/refactoring/MemberwiseInit/class_members.swift

+
+// RUN: %empty-directory(%t.result)
+// RUN: %refactor -memberwise-init -source-filename %s -pos=1:8 > %t.result/class_members.swift
+// RUN: diff -u %S/Outputs/class_members/class_members.swift.expected %t.result/class_members.swift


Could you also add an applicable test in test/refactoring/RefactoringKind? This refactoring shouldn't be available in value reference cases, like when cursor points to Person in _ = Person().

@nkcsgexi should this test be written in basic.swift file in /RefactoringKind/ or should I create a new file?

I think a new test file will be better.

@nkcsgexi So I have a file

class Person { var firstName: String! var lastName: String! var age: Int! var planet = "Earth", solarSystem = "Milky Way" var avgHeight = 175 } let _ = Person() // RUN: %refactor -memberwise-init -source-filename %s -pos=9:10 | %FileCheck %s -check-prefix=CHECK-NONE // CHECK-NONE: Action begins // CHECK-NONE-NEXT: Action ends

and this seems to be causing an assertion failure:
Assertion failed: (RealOffset+Size <= Buffer.size() && "Invalid location"), function RemoveText, file /Users/Labs/Documents/swift-source/llvm/tools/clang/lib/Rewrite/Rewriter.cpp, line 55.

Any idea what's going on? Am I missing some LLVM command or something?

@nkcsgexi the above assertion seems to be failing for the the extract.swift file in /RefactoringKind/ as well...

I figured out the problem, I was passing the -memberwise-init flag in the RUN command

mohitathwani · 2018-07-26T16:40:42Z

@nkcsgexi whenever you have time :)

nkcsgexi

Looking great! Only one minor thing before merging.

nkcsgexi · 2018-07-26T18:20:16Z

lib/IDE/Refactoring.cpp

+                           SmallVectorImpl<std::string>& memberNameVector,
+                           SmallVectorImpl<std::string>& memberTypeVector) {
+
+  if (CursorInfo.Kind != CursorInfoKind::ValueRef && !CursorInfo.ValueD)


We don't need the first condition. if (!CursorInfo.ValueD) is sufficient here.

@nkcsgexi made the change and squashed the commits

nkcsgexi · 2018-07-26T18:40:24Z

@swift-ci please smoke test

nkcsgexi · 2018-07-26T20:04:35Z

All tests passed! Merging 🚢

mohitathwani · 2018-07-26T20:25:54Z

Thanks to @harlanhaskins @nkcsgexi for helping me out and guiding me with this PR and @rintaro and @dndydon for your code reviews... Appreciate all the help I received :)

nkcsgexi · 2018-07-27T03:56:25Z

@mohitathwani Congrats on your first contribution to Swift! 🎉

mohitathwani changed the title ~~Setting up the process to solve SR-7292~~ [WIP] Generating member wise initializer with local refactoring Jun 4, 2018

harlanhaskins reviewed Jun 5, 2018

View reviewed changes

rintaro reviewed Jun 20, 2018

View reviewed changes

mohitathwani force-pushed the SR-7292 branch 2 times, most recently from edfe6af to ca62cbc Compare June 22, 2018 15:09

nkcsgexi reviewed Jun 26, 2018

View reviewed changes

nkcsgexi reviewed Jul 5, 2018

View reviewed changes

nkcsgexi reviewed Jul 10, 2018

View reviewed changes

nkcsgexi reviewed Jul 16, 2018

View reviewed changes

nkcsgexi reviewed Jul 24, 2018

View reviewed changes

nkcsgexi reviewed Jul 26, 2018

View reviewed changes

SR-7292 - Refactoring tool for generating member wise init

ce11229

mohitathwani force-pushed the SR-7292 branch from 2ee1a8f to ce11229 Compare July 26, 2018 18:32

nkcsgexi merged commit bff3f8a into swiftlang:master Jul 26, 2018

		@@ -50,6 +50,8 @@ CURSOR_REFACTORING(ConvertToDoCatch, "Convert To Do/Catch", convert.do.catch)

		CURSOR_REFACTORING(TrailingClosure, "Convert To Trailing Closure", trailingclosure)

		CURSOR_REFACTORING(MemberWiseInitLocalRefactoring, "Generate Member Wise Initializer", member.wise.init.local.refactoring)


		EditConsumer.accept(SM, targetLocation, "\ninternal init(");

		for (size_t i = 0; i < memberTypeVector.size(); i++) {


		collectMembersForInit(Tok, memberNameVector, memberTypeVector);

		return !memberTypeVector.empty();

[WIP] Generating member wise initializer with local refactoring #16983

[WIP] Generating member wise initializer with local refactoring #16983

Uh oh!

Conversation

mohitathwani commented Jun 4, 2018

Uh oh!

mohitathwani commented Jun 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

mohitathwani commented Jun 6, 2018

Uh oh!

mohitathwani commented Jun 10, 2018

Uh oh!

nkcsgexi commented Jun 11, 2018

Uh oh!

mohitathwani commented Jun 12, 2018

Uh oh!

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

Uh oh!

Uh oh!

mohitathwani commented Jun 14, 2018

Uh oh!

mohitathwani commented Jun 19, 2018

Uh oh!

nkcsgexi commented Jun 19, 2018

Uh oh!

mohitathwani commented Jun 20, 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!

rintaro Jun 20, 2018 • 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!

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!

mohitathwani commented Jun 21, 2018

Uh oh!

rintaro commented Jun 22, 2018

Uh oh!

mohitathwani commented Jun 22, 2018

Uh oh!

harlanhaskins commented Jun 22, 2018

Uh oh!

mohitathwani commented Jun 22, 2018

Uh oh!

harlanhaskins commented Jun 22, 2018

Uh oh!

harlanhaskins commented Jun 26, 2018

Uh oh!

nkcsgexi commented Jun 26, 2018

Uh oh!

mohitathwani commented Jun 26, 2018

Uh oh!

nkcsgexi 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!

mohitathwani commented Jun 26, 2018

mohitathwani commented Jun 6, 2018 •

edited

Loading

mohitathwani commented Jun 12, 2018 •

edited

Loading

rintaro Jun 20, 2018 •

edited

Loading