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

[SR-5739] Refactoring action to collapse nested if statement #48309

Closed
akyrtzi opened this issue Aug 23, 2017 · 7 comments
Closed

[SR-5739] Refactoring action to collapse nested if statement #48309

akyrtzi opened this issue Aug 23, 2017 · 7 comments
Labels
compiler The Swift compiler in itself feature A feature request or implementation good first issue Good for newcomers refactoring Area → source tooling: refactoring source tooling Area: IDE support, SourceKit, and other source tooling

Comments

@akyrtzi
Copy link
Member

akyrtzi commented Aug 23, 2017

Previous ID SR-5739
Radar rdar://32751236
Original Reporter @akyrtzi
Type New Feature
Status Resolved
Resolution Done

Attachment: Download

Additional Detail from JIRA
Votes 0
Component/s Source Tooling
Labels New Feature, Refactoring, StarterProposal
Assignee None
Priority Medium

md5: 117ec8e1f9cb632e9ad9dd725519d643

Issue Description:

Implement action to change code from:

  if a > 2 {
    if a < 10 {}
  }

to

  if a > 2 && a < 10 {}

The action could also apply for if-let statements.

@swift-ci
Copy link
Collaborator

Comment by Maksym Grebenets (JIRA)

Hi there.

I was really interested in getting into basics of Swift development by looking at this ticket. But then I later noticed that it's been already assigned.

I hope it wouldn't cause any misunderstanding, I'm not really trying to steal the thunder here 🙂
I'm more after learning something new.

So I did some work and so far I was able to walk AST and collect enough information to decide whether refactoring is applicable, for example:

struct NestedIfStatementsInfo {
  IfStmt *FirstIfStmt = nullptr;
  llvm::SmallVector<StmtCondition, 4> Conditions;
  BraceStmt *LastThenStmt = nullptr;

  bool isApplicable() { return FirstIfStmt && Conditions.size() > 1 && LastThenStmt; }

  StmtCondition makeCombinedStmtCondition() {
    llvm::SmallVector<StmtConditionElement, 4> AllStmtConditionElements;
    std::for_each(Conditions.begin(), Conditions.end(), [&](StmtCondition SC) {
      AllStmtConditionElements.append(SC.begin(), SC.end());
    });
    return StmtCondition(AllStmtConditionElements);
  }
};

Here FirstIfStmt is the "root" if-statement, or in other words the first in a sequence of collapsible if-statements.

Conditions is list of conditions for all collapsible if-statements.

LastThenStmt is the actual "body" sitting inside all those collapsible ifs:

// Pseudocode
FirstIfStmt(Condition1) {
  SecondIfStmt(Condition2) {
    N-thIfStmt(ConditionN) {
      LastThenStmt
    }
  }
}

// After refactoring
NewCondition = Condition1 + Condition2 + ... + ConditionN
NewIfStmt(NewCondition) {
  LastThenStmt
}

This gives me enough info to decide if refactoring is applicable and should be enough to perform the refactoring action.

As a matter of fact, I have code that successfully passes "Is refactoring kind applicable" tests for a bunch of sample code.

The tough (for me) question I have is "How to perform such a refactoring?".

Does it have to be manipulations with text directly or is there a way to modify AST by using setCond() and setThenStmt()?

I've tried something like

Result.FirstIfStmt->setThenStmt(Result.LastThenStmt);
Result.FirstIfStmt->setCond(Result.makeCombinedStmtCondition());

Not sure that 1st line is even safe, I'm changing ThenStmt pointer to point to LastThenStmt. Is such operation even correct and will it actually cause changes in AST?
So far I get crashes when trying to pint out (and walk) this updated part of AST.

Even if I could work out the way to update AST, it still has to be written back to the EditConsumer in some way, right?

Any help or advice would be really appreciated!

@swift-ci
Copy link
Collaborator

Comment by Will T Ellis (JIRA)

I tried modifying the AST, but that doesn't seem to be the right approach. Instead, I use information from the AST to create new text and replace the original range of text in the EditConsumer. You can find my work-in-progress PR here.

@nkcsgexi
Copy link
Member

Hey mgrebenets (JIRA User), thank you for your interest in Swift development! Your logic about when this refactoring is applicable makes sense to me. For the code modification part, as willtellis (JIRA User) suggested, we can only manipulate text to realize it. The long term goal is to use Swift's new syntax library to enable tree transformation, but this is still work in progress.

@swift-ci
Copy link
Collaborator

Comment by Maksym Grebenets (JIRA)

Thanks willtellis (JIRA User) and @nkcsgexi

That was helpful. I've looked at your work in progress willtellis (JIRA User) and learned tons 🙂

I had no idea this action could actually be implemented as cursor-based refactoring. I also totally missed the fact we can just "copy-paste" code from original editor context..

Thanks.

@swift-ci
Copy link
Collaborator

Comment by Maksym Grebenets (JIRA)

Hi @nkcsgexi.

Not sure if this is the best place to ask, but I have lots of questions on building and trying out the toolchain in Xcode.

I've read your post here as well as Brian's post [here|https://modocache.io/getting-started-with-swift-development,] but there still a lot of unknowns for a Swift Dark Side beginner like me...

There are question like

  • Why do I get some failing tests when building toolchain from latest master?

  • Can I make OS X toolchain build any faster, e.g. turn off dsym generation or some tests?

  • Why when I integrate my custom keychain into Xcode I get stuff like this:

    and how to figure what went wrong...

I mean this is probably not the best place to ask questions like this.

There isn't Comments section on Swift.org blog either.

Is there a "designated" place then?
Or do you guys check for certain tags on StackOverflow? 🙂

@nkcsgexi
Copy link
Member

Hi mgrebenets (JIRA User),

All these questions are valuable and i'm sure you are not the only one who are interested in getting the answer. Instead of discussing this in the issue thread, could you email these questions to swift-dev@swift.org?

Xi

@swift-ci
Copy link
Collaborator

Comment by Will T Ellis (JIRA)

#11851

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@AnthonyLatsis AnthonyLatsis added feature A feature request or implementation good first issue Good for newcomers compiler The Swift compiler in itself source tooling Area: IDE support, SourceKit, and other source tooling and removed new feature StarterProposal labels Nov 11, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler The Swift compiler in itself feature A feature request or implementation good first issue Good for newcomers refactoring Area → source tooling: refactoring source tooling Area: IDE support, SourceKit, and other source tooling
Projects
None yet
Development

No branches or pull requests

4 participants