Skip to content

[Refactoring] SR-6051 Expansion of switch statement missing cases #12281

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

Jump to bottom
Merged
merged 4 commits into from
Oct 13, 2017
Merged

[Refactoring] SR-6051 Expansion of switch statement missing cases #12281

merged 4 commits into from
Oct 13, 2017

Conversation

Kacper20
Copy link
Contributor

@Kacper20 Kacper20 commented Oct 4, 2017

Refactor is available when the cursor is placed on switch keyword.
There are still a few more tests to be written so it's WIP.
Addresses SR-6051.

cc @nkcsgexi

CodaFi
CodaFi reviewed Oct 4, 2017
View reviewed changes
lib/IDE/Refactoring.cpp Outdated
SubjectED = SubjectTy->getAnyNominal()->getAsEnumOrEnumExtensionContext();
}
if (!SubjectED) {
DiagEngine.diagnose(ExpandedStmtLoc, diag::no_subject_enum);
Copy link
Contributor

@CodaFi CodaFi Oct 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This... isn’t correct. For one you can switch over arbitrary expressions. For two, tuple patterns exist. So isApplicable should probably be updated to take into account what expressions this action can handle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's actually how Expand Default is implemented right now, before this PR is introduced. I've just moved the code and generalized it to add handling when switch is selected. You're right — I will fix isApplicable in both Expand Default and Expand Switch Cases. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, sorry that wasn't your bug. Syntactic expansion here is a very nuanced topic for me 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's great that you've spotted this. There are couple other places like that in Expand Default performChange — where we could move the checking to isApplicable. It's a perfect moment to... refactor it :)

@Kacper20
Copy link
Contributor Author

Kacper20 commented Oct 4, 2017

@nkcsgexi @CodaFi Currently in Expand Default implementation there are other places when applicability is handled in performChange method like that:

  if (UnhandledElements.empty()) {
    DiagEngine.diagnose(ExpandedStmtLoc, diag::no_remaining_cases);
    return true;
  }

What's the reason to do this? Should we move it also to the isApplicable so that the refactor will be grayed out in Xcode? Are there any advantages of handling this in performChange?

@nkcsgexi
Copy link
Contributor

nkcsgexi commented Oct 4, 2017

@Kacper20 I think moving it to isApplicable makes sense. Could you update it?

@nkcsgexi
Copy link
Contributor

nkcsgexi commented Oct 4, 2017

Great work, @Kacper20 ! Since @CodaFi has in-depth knowledge about switch case exhaustivity checking. I'll delegate the review to him 😄

@nkcsgexi nkcsgexi requested a review from CodaFi October 4, 2017 22:57
@Kacper20
Copy link
Contributor Author

Kacper20 commented Oct 4, 2017

Yep, thanks for the answer! So let's wait with the review — I will update it tomorrow with the changes :)

@Kacper20 Kacper20 mentioned this pull request Oct 7, 2017
Merged
@Kacper20
Copy link
Contributor Author

Kacper20 commented Oct 7, 2017 •
edited
Loading

@CodaFi I think it's ready to review — I've moved the checking to isApplicable and written couple more tests. Thanks in advance!

@Kacper20 Kacper20 changed the title [WIP][Refactoring] SR-6051 Expansion of switch statement missing cases [Refactoring] SR-6051 Expansion of switch statement missing cases Oct 7, 2017
CodaFi
CodaFi requested changes Oct 10, 2017
View reviewed changes
Copy link
Contributor

@CodaFi CodaFi left a comment

Choose a reason for hiding this comment

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

Just one change request. Everything else looks much improved. Thanks!

lib/IDE/Refactoring.cpp Outdated
@@ -633,6 +633,15 @@ class TokenBasedRefactoringAction : public RefactoringAction {
}
};

struct ApplicabilityContext {
Copy link
Contributor

Choose a reason for hiding this comment

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

All of this is just to get the SourceFile plumbed into that helper function, right? But every refactoring action has access to TheFile anyway. You can probably revert this (and add a non-public accessor if you need it).

Copy link
Contributor Author

@Kacper20 Kacper20 Oct 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, that's the reason. The problem is that action has static isApplicable() method and isApplicable which is not static calls static one and is not overridable. It'll now have access to the file through ResolvedRangeInfo, as discussed in: #12268

Thanks!

@Kacper20
Copy link
Contributor Author

@CodaFi Thanks! Fixed now.

@Kacper20
Copy link
Contributor Author

@CodaFi could you run the tests?

@nkcsgexi
Copy link
Contributor

@swift-ci please smoke test

@Kacper20
Copy link
Contributor Author

Could you merge it? @CodaFi @nkcsgexi Thanks in advance! :)

@nkcsgexi nkcsgexi merged commit 8cd677e into swiftlang:master Oct 13, 2017
@swift-ci swift-ci mentioned this pull request Oct 7, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CodaFi CodaFi CodaFi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Kacper20 @nkcsgexi @CodaFi