Skip to content

Conversation

@Regno
Copy link
Contributor

@Regno Regno commented Aug 19, 2019

Implement action to convert if statements from:

var e = E.case1
if e == .case1 {}
else if e == .case2 {}
else {}

to

  var e = E.case1
  switch e {
  case .case1: break
  case .case2: break
  default: break
  }

Resolves SR-5740.

@nkcsgexi

@akyrtzi akyrtzi requested a review from nkcsgexi August 19, 2019 22:58
@Regno
Copy link
Contributor Author

Regno commented Jan 7, 2020

@nathawes Could you review this PR please?

@nathawes nathawes self-requested a review January 7, 2020 18:32
Copy link
Contributor

@nathawes nathawes left a comment

Choose a reason for hiding this comment

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

Looking good so far! I thought of a few edge cases that might need handling and some tests to add while looking over it. I mentioned some potential enhancements to make it applicable in more situations in case you're interested in working on them, but those can certainly be left for a future PR too.

auto FirstArgumentType = FirstArgument->getType();
auto TypeKind = FirstArgumentType->getKind();
if (TypeKind != TypeKind::Enum)
return false;
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 it's ok to restrict it to just enums for now, but technically you could allow this refactoring to work for any type that implements the ~= operator, rather than just enums. e.g. the below is fine:

let x = 10
                                    // switch x {
if x == 5 {                         // case 5: ...
} else if x == 4 {                  // case 4: ...
} else if 1...3 ~= x {              // case 1...3: ...
} else {                            // default: ...
}                                   // }

Allowing any type that conforms to the Equatable protocol (which provides a default implementation for ~=) is probably easier to check for and would probably cover most cases:

struct MyType: Equatable { let v: String }
let y = MyType(v: "hello")
                                    // switch y {
if y == MyType(v: "bye") {          // case MyType(v: "bye"): ...
} else if x == MyType(v: "tchau") { // case MyType(v: "tchau"): ...
} else {                            // default: ...
}                                   // }

I think you can do the check using ModuleDecl::conformsToProtocol() to check if FirstArgumentType conforms to the Equatable protocol which you can get via Ctx->getProtocol(KnownProtocolKind::Equatable). I think you need to call it on the module the defines FirstArgumentType. Alternatively, IDETypechecking.h has an isConvertibleTo function that might to the right thing without needing the ModuleDecl.

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 added such test cases too.

But I don't check that type conforms to the Equatable protocol. I just suppose that user will try to convert a valid if statement.

@Regno
Copy link
Contributor Author

Regno commented Feb 2, 2020

I can't reopen this PR after force push to branch. So I create new one: #29596

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants