Merge pull request #9994 from xedin/rdar-32431736

[Diagnostics] Add a fix-it for optional to raw representable type conversion

Author: xedin

include/swift/AST/DiagnosticsSema.def

@@ -2024,6 +2024,8 @@ NOTE(enum_raw_value_incrementing_from_here,none,
      "raw value auto-incremented from here",())
 NOTE(enum_raw_value_incrementing_from_zero,none,
      "raw value implicitly auto-incremented from zero",())
+NOTE(construct_raw_representable_from_unwrapped_value,none,
+     "construct %0 from unwrapped %1 value", (Type, Type))
 
 // Derived conformances
 

include/swift/AST/Expr.h

@@ -592,12 +592,12 @@ class alignas(8) Expr {
   /// a base class.
   bool isSuperExpr() const;
 
-  /// Returns true if directly appending a parameter list would be syntactically
-  /// valid.
+  /// Returns false if this expression needs to be wrapped in parens when
+  /// used inside of a any postfix expression, true otherwise.
   ///
-  /// Good examples: foo.bar, baz().
-  /// Bad examples: 
-  bool canAppendCallParentheses() const;
+  /// \param appendingPostfixOperator if the expression being
+  /// appended is a postfix operator like '!' or '?'.
+  bool canAppendPostfixExpression(bool appendingPostfixOperator = false) const;
 
   /// Returns true if this is an infix operator of some sort, including
   /// a builtin operator.

lib/AST/Expr.cpp

@@ -655,7 +655,7 @@ bool Expr::isSuperExpr() const {
   } while (true);
 }
 
-bool Expr::canAppendCallParentheses() const {
+bool Expr::canAppendPostfixExpression(bool appendingPostfixOperator) const {
   switch (getKind()) {
   case ExprKind::Error:
   case ExprKind::CodeCompletion:
@@ -749,11 +749,13 @@ bool Expr::canAppendCallParentheses() const {
     return false;
 
   case ExprKind::Call:
-  case ExprKind::PostfixUnary:
   case ExprKind::DotSyntaxCall:
   case ExprKind::ConstructorRefCall:
     return true;
 
+  case ExprKind::PostfixUnary:
+    return !appendingPostfixOperator;
+
   case ExprKind::PrefixUnary:
   case ExprKind::Binary:
     return false;
@@ -784,7 +786,7 @@ bool Expr::canAppendCallParentheses() const {
     // Implicit conversion nodes have no syntax of their own; defer to the
     // subexpression.
     return cast<ImplicitConversionExpr>(this)->getSubExpr()
-      ->canAppendCallParentheses();
+      ->canAppendPostfixExpression(appendingPostfixOperator);
 
   case ExprKind::ForcedCheckedCast:
   case ExprKind::ConditionalCheckedCast:

lib/Migrator/APIDiffMigratorPass.cpp

@@ -464,7 +464,7 @@ struct APIDiffMigratorPass : public ASTMigratorPass, public SourceEntityWalker {
     auto *SelfExpr = AllArgs[0].ArgExp;
     if (auto *IOE = dyn_cast<InOutExpr>(SelfExpr))
       SelfExpr = IOE->getSubExpr();
-    const bool NeedParen = !SelfExpr->canAppendCallParentheses();
+    const bool NeedParen = !SelfExpr->canAppendPostfixExpression();
 
     // Remove the global function name: "Foo(a, b..." to "a, b..."
     Editor.remove(CharSourceRange(SM, Call->getStartLoc(),

lib/Sema/CSApply.cpp

@@ -7216,17 +7216,6 @@ static bool exprNeedsParensAfterAddingAs(TypeChecker &TC, DeclContext *DC,
   return exprNeedsParensOutsideFollowingOperator(TC, DC, expr, rootExpr, asPG);
 }
 
-static bool exprNeedsParensInsidePostfixOperator(TypeChecker &TC,
-                                                 DeclContext *DC,
-                                                 Expr *expr) {
-  // Prefix and infix operators will bind outside of a postfix operator.
-  // Postfix operators will get token-merged with a new postfix operator.
-  return (isa<PrefixUnaryExpr>(expr) ||
-          isa<PostfixUnaryExpr>(expr) ||
-          isa<OptionalEvaluationExpr>(expr) ||
-          expr->isInfixOperator());
-}
-
 namespace {
   class ExprWalker : public ASTWalker {
     ExprRewriter &Rewriter;
@@ -7424,14 +7413,11 @@ bool ConstraintSystem::applySolutionFix(Expr *expr,
     } else {
       auto diag = TC.diagnose(affected->getLoc(),
                               diag::missing_unwrap_optional, type);
-      bool parensNeeded =
-        exprNeedsParensInsidePostfixOperator(TC, DC, affected);
-
-      if (parensNeeded) {
+      if (affected->canAppendPostfixExpression(true)) {
+        diag.fixItInsertAfter(affected->getEndLoc(), "!");
+      } else {
         diag.fixItInsert(affected->getStartLoc(), "(")
             .fixItInsertAfter(affected->getEndLoc(), ")!");
-      } else {
-        diag.fixItInsertAfter(affected->getEndLoc(), "!");
       }
     }
     return true;

lib/Sema/CSDiag.cpp

@@ -2790,7 +2790,7 @@ bool FailureDiagnosis::diagnoseConversionToBool(Expr *expr, Type exprType) {
     // Technically we only need them if there's something in 'expr' with
     // lower precedence than '!=', but the code actually comes out nicer
     // in most cases with parens on anything non-trivial.
-    if (expr->canAppendCallParentheses()) {
+    if (expr->canAppendPostfixExpression()) {
       prefix = prefix.drop_back();
       suffix = suffix.drop_front();
     }
@@ -2972,7 +2972,7 @@ bool FailureDiagnosis::diagnoseGeneralConversionFailure(Constraint *constraint){
       DeclName(CS->TC.Context.getIdentifier("boolValue")));
     if (!LookupResult.empty()) {
       if (isa<VarDecl>(LookupResult.begin()->Decl)) {
-        if (anchor->canAppendCallParentheses())
+        if (anchor->canAppendPostfixExpression())
           diagnose(anchor->getLoc(), diag::types_not_convertible_use_bool_value,
                    fromType, toType).fixItInsertAfter(anchor->getEndLoc(),
                                                       ".boolValue");
@@ -3596,7 +3596,7 @@ static bool tryRawRepresentableFixIts(InFlightDiagnostic &diag,
     if (fromTypeIsOptional && toTypeIsOptional) {
       // Use optional's map function to convert conditionally, like so:
       //   expr.map{ T(rawValue: $0) }
-      bool needsParens = !expr->canAppendCallParentheses();
+      bool needsParens = !expr->canAppendPostfixExpression();
       std::string mapCodeFix;
       if (needsParens) {
         diag.fixItInsert(exprRange.Start, "(");
@@ -3611,6 +3611,24 @@ static bool tryRawRepresentableFixIts(InFlightDiagnostic &diag,
     } else if (!fromTypeIsOptional) {
       diag.fixItInsert(exprRange.Start, convWrapBefore);
       diag.fixItInsertAfter(exprRange.End, convWrapAfter);
+    } else {
+      SmallString<16> fixItBefore(convWrapBefore);
+      SmallString<16> fixItAfter;
+
+      if (!expr->canAppendPostfixExpression(true)) {
+        fixItBefore += "(";
+        fixItAfter = ")";
+      }
+
+      fixItAfter += "!" + convWrapAfter.str();
+
+      diag.flush();
+      CS->TC.diagnose(expr->getLoc(),
+                      diag::construct_raw_representable_from_unwrapped_value,
+                      toType, fromType)
+        .highlight(exprRange)
+        .fixItInsert(exprRange.Start, fixItBefore)
+        .fixItInsert(exprRange.End, fixItAfter);
     }
   };
 

lib/Sema/MiscDiagnostics.cpp

@@ -548,7 +548,7 @@ static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E,
         auto diag = TC.diagnose(E->getEndLoc(),
                                 diag::warn_value_of_metatype_missing_self,
                                 E->getType()->getRValueInstanceType());
-        if (E->canAppendCallParentheses()) {
+        if (E->canAppendPostfixExpression()) {
           diag.fixItInsertAfter(E->getEndLoc(), ".self");
         } else {
           diag.fixItInsert(E->getStartLoc(), "(");
@@ -578,7 +578,7 @@ static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E,
 
       // Add fix-it to insert ".self".
       auto diag = TC.diagnose(E->getEndLoc(), diag::add_self_to_type);
-      if (E->canAppendCallParentheses()) {
+      if (E->canAppendPostfixExpression()) {
         diag.fixItInsertAfter(E->getEndLoc(), ".self");
       } else {
         diag.fixItInsert(E->getStartLoc(), "(");
@@ -2190,7 +2190,7 @@ VarDeclUsageChecker::~VarDeclUsageChecker() {
               if (isa<NamedPattern>(LP->getSubPattern())) {
                 auto initExpr = SC->getCond()[0].getInitializer();
                 if (initExpr->getStartLoc().isValid()) {
-                  unsigned noParens = initExpr->canAppendCallParentheses();
+                  unsigned noParens = initExpr->canAppendPostfixExpression();
 
                   // If the subexpr is an "as?" cast, we can rewrite it to
                   // be an "is" test.

lib/Sema/TypeCheckAvailability.cpp

@@ -1623,7 +1623,7 @@ static void fixItAvailableAttrRename(TypeChecker &TC,
     CharSourceRange selfExprRange =
         Lexer::getCharSourceRangeFromSourceRange(sourceMgr,
                                                  selfExpr->getSourceRange());
-    bool needsParens = !selfExpr->canAppendCallParentheses();
+    bool needsParens = !selfExpr->canAppendPostfixExpression();
 
     SmallString<64> selfReplace;
     if (needsParens)

lib/Sema/TypeCheckConstraints.cpp

@@ -3438,7 +3438,7 @@ CheckedCastKind TypeChecker::typeCheckCheckedCast(Type fromType,
             diag.fixItReplace(SourceRange(diagLoc, diagToRange.End), "!= nil");
 
             // Add parentheses if needed.
-            if (!fromExpr->canAppendCallParentheses()) {
+            if (!fromExpr->canAppendPostfixExpression()) {
               diag.fixItInsert(fromExpr->getStartLoc(), "(");
               diag.fixItInsertAfter(fromExpr->getEndLoc(), ")");
             }

test/Sema/enum_raw_representable.swift

@@ -83,3 +83,23 @@ class Outer {
     case a
   }
 }
+
+// rdar://problem/32431736 - Conversion fix-it from String to String raw value enum can't look through optionals
+
+func rdar32431736() {
+  enum E : String {
+    case A = "A"
+    case B = "B"
+  }
+
+  let items1: [String] = ["A", "a"]
+  let items2: [String]? = ["A"]
+
+  let myE1: E = items1.first
+  // expected-error@-1 {{cannot convert value of type 'String?' to specified type 'E'}}
+  // expected-note@-2 {{construct 'E' from unwrapped 'String' value}} {{17-17=E(rawValue: }} {{24-24=!)}}
+
+  let myE2: E = items2?.first
+  // expected-error@-1 {{cannot convert value of type 'String?' to specified type 'E'}}
+  // expected-note@-2 {{construct 'E' from unwrapped 'String' value}} {{17-17=E(rawValue: (}} {{25-25=)!)}}
+}