Skip to content

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

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

Merged
merged 1 commit into from
Jul 26, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions include/swift/IDE/RefactoringKinds.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 Memberwise Initializer", memberwise.init.local.refactoring)

RANGE_REFACTORING(ExtractExpr, "Extract Expression", extract.expr)

RANGE_REFACTORING(ExtractFunction, "Extract Method", extract.function)
Expand Down
104 changes: 104 additions & 0 deletions lib/IDE/Refactoring.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2602,6 +2602,110 @@ bool RefactoringActionLocalizeString::performChange() {
return false;
}

static void generateMemberwiseInit(SourceEditConsumer &EditConsumer,
SourceManager &SM,
SmallVectorImpl<std::string>& memberNameVector,
SmallVectorImpl<std::string>& memberTypeVector,
SourceLoc targetLocation) {

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

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

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

if (i != memberTypeVector.size() - 1) {
EditConsumer.accept(SM, targetLocation, ", ");
}
}

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

for (auto varName: memberNameVector) {
EditConsumer.accept(SM, targetLocation,
"self." + varName + " = " + varName + "\n");
}

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

static SourceLoc collectMembersForInit(ResolvedCursorInfo CursorInfo,
SmallVectorImpl<std::string>& memberNameVector,
SmallVectorImpl<std::string>& memberTypeVector) {

if (!CursorInfo.ValueD)
return SourceLoc();

ClassDecl *classDecl = dyn_cast<ClassDecl>(CursorInfo.ValueD);
if (!classDecl || classDecl->getStoredProperties().empty() ||
CursorInfo.IsRef) {
return SourceLoc();
}

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

SourceLoc targetLocation = bracesStart.getAdvancedLoc(1);
if (!targetLocation.isValid())
return SourceLoc();

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

auto varDeclIndex =
parentPatternBinding->getPatternEntryIndexForVarDecl(varDecl);

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

StringRef memberName = varDecl->getName().str();
memberNameVector.push_back(memberName.str());

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.


if (memberNameVector.empty() || memberTypeVector.empty()) {
return SourceLoc();
}

return targetLocation;
}

bool RefactoringActionMemberwiseInitLocalRefactoring::
isApplicable(ResolvedCursorInfo Tok, DiagnosticEngine &Diag) {

SmallVector<std::string, 8> memberNameVector;
SmallVector<std::string, 8> memberTypeVector;

return collectMembersForInit(Tok, memberNameVector,
memberTypeVector).isValid();
}

bool RefactoringActionMemberwiseInitLocalRefactoring::performChange() {

SmallVector<std::string, 8> memberNameVector;
SmallVector<std::string, 8> memberTypeVector;

SourceLoc targetLocation = collectMembersForInit(CursorInfo, memberNameVector,
memberTypeVector);
if (targetLocation.isInvalid())
return true;

generateMemberwiseInit(EditConsumer, SM, memberNameVector,
memberTypeVector, targetLocation);

return false;
}

static CharSourceRange
findSourceRangeToWrapInCatch(ResolvedCursorInfo CursorInfo,
SourceFile *TheFile,
Expand Down
14 changes: 14 additions & 0 deletions test/refactoring/MemberwiseInit/Outputs/class_members/class_members.swift.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
class Person {
internal init(firstName: String?, lastName: String?, age: Int?) {
self.firstName = firstName
self.lastName = lastName
self.age = age
}

var firstName: String!
var lastName: String!
var age: Int!
var planet = "Earth", solarSystem = "Milky Way"
var avgHeight = 175
}

11 changes: 11 additions & 0 deletions test/refactoring/MemberwiseInit/class_members.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
class Person {
var firstName: String!
var lastName: String!
var age: Int!
var planet = "Earth", solarSystem = "Milky Way"
var avgHeight = 175
}

// 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a new test file will be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

13 changes: 13 additions & 0 deletions test/refactoring/RefactoringKind/member_init_class_as_value.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
class Person {
var firstName: String!
var lastName: String!
var age: Int!
var planet = "Earth", solarSystem = "Milky Way"
var avgHeight = 175
}

let _ = Person()

// RUN: %refactor -source-filename %s -pos=9:10 | %FileCheck %s -check-prefix=CHECK-NONE
// CHECK-NONE: Action begins
// CHECK-NONE-NEXT: Action ends
3 changes: 2 additions & 1 deletion tools/swift-refactor/swift-refactor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ Action(llvm::cl::desc("kind:"), llvm::cl::init(RefactoringKind::None),
clEnumValN(RefactoringKind::TrailingClosure,
"trailingclosure", "Perform trailing closure refactoring"),
clEnumValN(RefactoringKind::ReplaceBodiesWithFatalError,
"replace-bodies-with-fatalError", "Perform trailing closure refactoring")));
"replace-bodies-with-fatalError", "Perform trailing closure refactoring"),
clEnumValN(RefactoringKind::MemberwiseInitLocalRefactoring, "memberwise-init", "Generate member wise initializer")));


static llvm::cl::opt<std::string>
Expand Down