SR-11295: Emit diagnostics for same type coercion

Author: LucianoPAlmeida

include/swift/AST/DiagnosticsSema.def

@@ -949,18 +949,22 @@ ERROR(super_initializer_not_in_initializer,none,
 WARNING(isa_is_always_true,none, "'%0' test is always true",
         (StringRef))
 WARNING(isa_is_foreign_check,none,
-      "'is' test is always true because %0 is a Core Foundation type",
-      (Type))
+        "'is' test is always true because %0 is a Core Foundation type",
+        (Type))
 WARNING(conditional_downcast_coercion,none,
-      "conditional cast from %0 to %1 always succeeds",
-      (Type, Type))
-
+        "conditional cast from %0 to %1 always succeeds",
+        (Type, Type))
+WARNING(unnecessary_same_type_coercion,none,
+        "redundant cast to %0 has no effect",
+        (Type))
+WARNING(unnecessary_same_typealias_type_coercion,none,
+        "redundant cast from %0 to %1 has no effect",
+        (Type, Type))
 WARNING(forced_downcast_noop,none,
         "forced cast of %0 to same type has no effect", (Type))
-
 WARNING(forced_downcast_coercion,none,
-      "forced cast from %0 to %1 always succeeds; did you mean to use 'as'?",
-      (Type, Type))
+        "forced cast from %0 to %1 always succeeds; did you mean to use 'as'?",
+        (Type, Type))
 
 // Note: the Boolean at the end indicates whether bridging is required after
 // the cast.

lib/Sema/CSDiagnostics.cpp

@@ -4400,9 +4400,7 @@ bool InaccessibleMemberFailure::diagnoseAsError() {
     auto &cs = getConstraintSystem();
     auto *locator =
         cs.getConstraintLocator(baseExpr, ConstraintLocator::Member);
-    if (llvm::any_of(cs.getFixes(), [&](const ConstraintFix *fix) {
-          return fix->getLocator() == locator;
-        }))
+    if (cs.hasFixFor(locator))
       return false;
   }
 
@@ -5031,6 +5029,36 @@ bool ThrowingFunctionConversionFailure::diagnoseAsError() {
   return true;
 }
 
+bool UnnecessaryCoercionFailure::diagnoseAsError() {
+  auto expr = cast<CoerceExpr>(getAnchor());
+  auto sourceRange =
+      SourceRange(expr->getLoc(), expr->getCastTypeLoc().getSourceRange().End);
+
+  if (isa<TypeAliasType>(getFromType().getPointer()) &&
+      isa<TypeAliasType>(getToType().getPointer())) {
+    auto fromTypeAlias = cast<TypeAliasType>(getFromType().getPointer());
+    auto toTypeAlias = cast<TypeAliasType>(getToType().getPointer());
+    // If the typealias are different, we need a warning
+    // mentioning both types.
+    if (fromTypeAlias->getDecl() != toTypeAlias->getDecl()) {
+      emitDiagnostic(expr->getLoc(),
+                     diag::unnecessary_same_typealias_type_coercion,
+                     getFromType(), getToType())
+
+          .fixItRemove(sourceRange);
+    } else {
+      emitDiagnostic(expr->getLoc(), diag::unnecessary_same_type_coercion,
+                     getToType())
+          .fixItRemove(sourceRange);
+    }
+  } else {
+    emitDiagnostic(expr->getLoc(), diag::unnecessary_same_type_coercion,
+                   getToType())
+        .fixItRemove(sourceRange);
+  }
+  return true;
+}
+
 bool InOutConversionFailure::diagnoseAsError() {
   auto *anchor = getAnchor();
   auto *locator = getLocator();

lib/Sema/CSDiagnostics.h

@@ -156,7 +156,13 @@ class FailureDiagnostic {
   }
 
   /// \returns true is locator hasn't been simplified down to expression.
-  bool hasComplexLocator() const { return HasComplexLocator; }
+  bool hasComplexLocator() const {
+    bool isExplicitCoercion =
+        getLocator()
+            ->isLastElement<
+                ConstraintLocator::PathElement::ExplicitTypeCoercion>();
+    return HasComplexLocator && !isExplicitCoercion;
+  }
 
   /// \returns A parent expression if sub-expression is contained anywhere
   /// in the root expression or `nullptr` otherwise.
@@ -707,8 +713,8 @@ class ContextualFailure : public FailureDiagnostic {
   /// If we're trying to convert something to `nil`.
   bool diagnoseConversionToNil() const;
 
-  // If we're trying to convert something of type "() -> T" to T,
-  // then we probably meant to call the value.
+  /// If we're trying to convert something of type "() -> T" to T,
+  /// then we probably meant to call the value.
   bool diagnoseMissingFunctionCall() const;
 
   /// Produce a specialized diagnostic if this is an invalid conversion to Bool.
@@ -1460,11 +1466,11 @@ class MutatingMemberRefOnImmutableBase final : public FailureDiagnostic {
   bool diagnoseAsError() override;
 };
 
-// Diagnose an attempt to use AnyObject as the root type of a KeyPath
-//
-// ```swift
-// let keyPath = \AnyObject.bar
-// ```
+/// Diagnose an attempt to use AnyObject as the root type of a KeyPath
+///
+/// ```swift
+/// let keyPath = \AnyObject.bar
+/// ```
 class AnyObjectKeyPathRootFailure final : public FailureDiagnostic {
 
 public:
@@ -1774,6 +1780,27 @@ class ExpandArrayIntoVarargsFailure final : public ContextualFailure {
   void tryDropArrayBracketsFixIt(Expr *anchor) const;
 };
 
+/// Diagnose a situation where there is an explicit type coercion
+/// to the same type e.g.:
+///
+/// ```swift
+/// Double(1) as Double // redundant cast to 'Double' has no effect
+/// 1 as Double as Double // redundant cast to 'Double' has no effect
+/// let string = "String"
+/// let s = string as String // redundant cast to 'String' has no effect
+/// ```
+class UnnecessaryCoercionFailure final
+    : public ContextualFailure {
+      
+public:
+  UnnecessaryCoercionFailure(Expr *root, ConstraintSystem &cs,
+                             Type fromType, Type toType,
+                             ConstraintLocator *locator)
+      : ContextualFailure(root, cs, fromType, toType, locator) {}
+  
+  bool diagnoseAsError() override;
+};
+
 /// Diagnose a situation there is a mismatch between argument and parameter
 /// types e.g.:
 ///

lib/Sema/CSFix.cpp

@@ -857,6 +857,36 @@ IgnoreContextualType *IgnoreContextualType::create(ConstraintSystem &cs,
       IgnoreContextualType(cs, resultTy, specifiedTy, locator);
 }
 
+bool RemoveUnnecessaryCoercion::diagnose(Expr *root, bool asNote) const {
+  auto &cs = getConstraintSystem();
+  UnnecessaryCoercionFailure failure(root, cs, getFromType(), getToType(),
+                                     getLocator());
+  return failure.diagnose(asNote);
+}
+
+bool RemoveUnnecessaryCoercion::attempt(ConstraintSystem &cs, Type fromType,
+                                        Type toType,
+                                        ConstraintLocatorBuilder locator) {
+  if (auto last = locator.last()) {
+    if (last->is<LocatorPathElt::ExplicitTypeCoercion>()) {
+      auto expr = cast<CoerceExpr>(locator.getAnchor());
+      auto toTypeRepr = expr->getCastTypeLoc().getTypeRepr();
+
+      // only diagnosing for coercion where the left-side is a DeclRefExpr
+      // or a explicit/implicit coercion e.g. Double(1) as Double
+      if (!isa<ImplicitlyUnwrappedOptionalTypeRepr>(toTypeRepr) &&
+          (isa<DeclRefExpr>(expr->getSubExpr()) ||
+           isa<CoerceExpr>(expr->getSubExpr()))) {
+        auto *fix = new (cs.getAllocator()) RemoveUnnecessaryCoercion(
+            cs, fromType, toType, cs.getConstraintLocator(locator));
+
+        return cs.recordFix(fix);
+      }
+    }
+  }
+  return false;
+}
+
 bool IgnoreAssignmentDestinationType::diagnose(Expr *root, bool asNote) const {
   auto &cs = getConstraintSystem();
   auto *AE = cast<AssignExpr>(getAnchor());

lib/Sema/CSFix.h

@@ -196,6 +196,9 @@ enum class FixKind : uint8_t {
   /// Allow a single tuple parameter to be matched with N arguments
   /// by forming all of the given arguments into a single tuple.
   AllowTupleSplatForSingleParameter,
+  
+  /// Remove an unnecessary coercion ('as') if the types are already equal. e.g. Double(1) as Double
+  RemoveUnnecessaryCoercion,
 
   /// Allow a single argument type mismatch. This is the most generic
   /// failure related to argument-to-parameter conversions.
@@ -1340,6 +1343,24 @@ class IgnoreContextualType : public ContextualMismatch {
                                       ConstraintLocator *locator);
 };
 
+class RemoveUnnecessaryCoercion : public ContextualMismatch {
+protected:
+  RemoveUnnecessaryCoercion(ConstraintSystem &cs,
+                            Type fromType, Type toType,
+                            ConstraintLocator *locator)
+  : ContextualMismatch(cs, FixKind::RemoveUnnecessaryCoercion, fromType, toType,
+                       locator, /*isWarning*/ true) {}
+  
+public:
+  std::string getName() const override { return "remove unnecessary explicit type coercion"; }
+  
+  bool diagnose(Expr *root, bool asNote = false) const override;
+  
+  static bool attempt(ConstraintSystem &cs,
+                      Type fromType, Type toType,
+                      ConstraintLocatorBuilder locator);
+};
+
 class IgnoreAssignmentDestinationType final : public ContextualMismatch {
   IgnoreAssignmentDestinationType(ConstraintSystem &cs, Type sourceTy,
                                   Type destTy, ConstraintLocator *locator)

lib/Sema/CSSimplify.cpp

@@ -2587,7 +2587,8 @@ bool ConstraintSystem::repairFailures(
                         });
   };
 
-  if (path.empty()) {
+  if (path.empty() ||
+      path.back().getKind() == ConstraintLocator::ExplicitTypeCoercion) {
     if (!anchor)
       return false;
 
@@ -3212,9 +3213,16 @@ ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind,
   // let's defer it until later proper check.
   if (!(desugar1->is<DependentMemberType>() &&
         desugar2->is<DependentMemberType>())) {
-    // If the types are obviously equivalent, we're done.
-    if (desugar1->isEqual(desugar2) && !isa<InOutType>(desugar2)) {
-      return getTypeMatchSuccess();
+    if (desugar1->isEqual(desugar2)) {
+      if (kind >= ConstraintKind::Conversion &&
+          !flags.contains(TMF_ApplyingFix)) {
+        if (RemoveUnnecessaryCoercion::attempt(*this, type1, type2,
+                                               getConstraintLocator(locator))) {
+          return getTypeMatchFailure(locator);
+        }
+      }
+      if (!isa<InOutType>(desugar2))
+        return getTypeMatchSuccess();
     }
   }
 
@@ -7999,6 +8007,7 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
   case FixKind::GenericArgumentsMismatch:
   case FixKind::AllowMutatingMemberOnRValueBase:
   case FixKind::AllowTupleSplatForSingleParameter:
+  case FixKind::RemoveUnnecessaryCoercion:
     llvm_unreachable("handled elsewhere");
   }
 
@@ -8305,11 +8314,20 @@ void ConstraintSystem::addExplicitConversionConstraint(
   SmallVector<Constraint *, 3> constraints;
 
   auto locatorPtr = getConstraintLocator(locator);
+  ConstraintLocator *coerceLocator = locatorPtr;
+
+  if (allowFixes && shouldAttemptFixes()) {
+    auto *anchor = locator.getAnchor();
+    if (isa<CoerceExpr>(anchor) && !anchor->isImplicit()) {
+      coerceLocator =
+          getConstraintLocator(anchor, LocatorPathElt::ExplicitTypeCoercion());
+    }
+  }
 
   // Coercion (the common case).
   Constraint *coerceConstraint =
     Constraint::create(*this, ConstraintKind::Conversion,
-                       fromType, toType, locatorPtr);
+                       fromType, toType, coerceLocator);
   coerceConstraint->setFavored();
   constraints.push_back(coerceConstraint);
 

lib/Sema/ConstraintLocator.cpp

@@ -86,6 +86,7 @@ void ConstraintLocator::Profile(llvm::FoldingSetNodeID &id, Expr *anchor,
     case KeyPathRoot:
     case KeyPathValue:
     case KeyPathComponentResult:
+    case ExplicitTypeCoercion:
       auto numValues = numNumericValuesInPathElement(elt.getKind());
       for (unsigned i = 0; i < numValues; ++i)
         id.AddInteger(elt.getValue(i));
@@ -132,6 +133,7 @@ unsigned LocatorPathElt::getNewSummaryFlags() const {
   case ConstraintLocator::KeyPathRoot:
   case ConstraintLocator::KeyPathValue:
   case ConstraintLocator::KeyPathComponentResult:
+  case ConstraintLocator::ExplicitTypeCoercion:
     return 0;
 
   case ConstraintLocator::FunctionArgument:
@@ -456,6 +458,10 @@ void ConstraintLocator::dump(SourceManager *sm, raw_ostream &out) {
     case KeyPathComponentResult:
       out << "key path component result";
       break;
+        
+    case ExplicitTypeCoercion:
+      out << "type coercion";
+      break;
     }
   }
   out << ']';

lib/Sema/ConstraintLocator.h

@@ -89,6 +89,7 @@ class ConstraintLocator : public llvm::FoldingSetNode {
     case KeyPathRoot:
     case KeyPathValue:
     case KeyPathComponentResult:
+    case ExplicitTypeCoercion:
       return 0;
 
     case ContextualType:

lib/Sema/ConstraintLocatorPathElts.def

@@ -164,6 +164,9 @@ SIMPLE_LOCATOR_PATH_ELT(UnresolvedMember)
 /// The candidate witness during protocol conformance checking.
 CUSTOM_LOCATOR_PATH_ELT(Witness)
 
+/// An explicit type coercion.
+SIMPLE_LOCATOR_PATH_ELT(ExplicitTypeCoercion)
+
 #undef LOCATOR_PATH_ELT
 #undef CUSTOM_LOCATOR_PATH_ELT
 #undef SIMPLE_LOCATOR_PATH_ELT

test/ClangImporter/Darwin_test.swift

@@ -9,7 +9,8 @@ _ = nil as Fract? // expected-error{{use of undeclared type 'Fract'}}
 _ = nil as Darwin.Fract? // okay
 
 _ = 0 as OSErr
-_ = noErr as OSStatus // noErr is from the overlay
+// noErr is from the overlay
+_ = noErr as OSStatus // expected-warning {{redundant cast to 'OSStatus' (aka 'Int32') has no effect}} {{11-23=}}
 _ = 0 as UniChar
 
 _ = ProcessSerialNumber()

test/ClangImporter/MixedSource/mixed-target-using-header.swift

@@ -53,12 +53,13 @@ func testSuppressed() {
 #endif
 
 func testMacro() {
-  _ = CONSTANT as CInt
+  _ = CONSTANT as CInt // expected-warning {{redundant cast to 'CInt' (aka 'Int32') has no effect}} {{16-24=}}
 }
 
 func testFoundationOverlay() {
   _ = NSUTF8StringEncoding // no ambiguity
-  _ = NSUTF8StringEncoding as UInt // and we should get the overlay version
+  // and we should get the overlay version
+  _ = NSUTF8StringEncoding as UInt // expected-warning {{redundant cast to 'UInt' has no effect}} {{28-36=}}
 }
 
 #if !SILGEN

test/ClangImporter/Security_test.swift

@@ -4,8 +4,8 @@
 
 import Security
 
-_ = kSecClass as CFString
-_ = kSecClassGenericPassword as CFString
+_ = kSecClass as CFString // expected-warning {{redundant cast to 'CFString' has no effect}} {{15-27=}}
+_ = kSecClassGenericPassword as CFString // expected-warning {{redundant cast to 'CFString' has no effect}} {{30-42=}}
 _ = kSecClassGenericPassword as CFDictionary // expected-error {{'CFString?' is not convertible to 'CFDictionary'}} {{30-32=as!}}
 
 func testIntegration() {

test/ClangImporter/attr-swift_private.swift

@@ -102,7 +102,7 @@ public func testTopLevel() {
 
 _ = __PrivAnonymousA
 _ = __E0PrivA
-_ = __PrivE1A as __PrivE1
+_ = __PrivE1A as __PrivE1 // expected-warning {{redundant cast to '__PrivE1' has no effect}} {{15-27=}}
 _ = NSEnum.__privA
 _ = NSEnum.B
 _ = NSOptions.__privA

test/ClangImporter/cf.swift

@@ -90,9 +90,9 @@ func testTollFree1(_ ccmduct: CCMutableDuct) {
 }
 
 func testChainedAliases(_ fridge: CCRefrigerator) {
-  _ = fridge as CCRefrigerator
+  _ = fridge as CCRefrigerator // expected-warning {{redundant cast to 'CCRefrigerator' has no effect}} {{14-32=}}
 
-  _ = fridge as CCFridge
+  _ = fridge as CCFridge // expected-warning {{redundant cast to 'CCFridge' (aka 'CCRefrigerator') has no effect}} {{14-26=}}
   _ = fridge as CCFridgeRef // expected-error{{'CCFridgeRef' has been renamed to 'CCFridge'}} {{17-28=CCFridge}}
 }
 

test/ClangImporter/cfuncs_parse.swift

@@ -15,7 +15,7 @@ func test_cfunc2(_ i: Int) {
 #else
   let f = cfunc2(i, 17)
 #endif
-  _ = f as Float
+  _ = f as Float // expected-warning {{redundant cast to 'Float' has no effect}} {{9-18=}}
   cfunc2(b:17, a:i) // expected-error{{extraneous argument labels 'b:a:' in call}}
   // expected-error@-1 {{cannot convert value of type 'Int' to expected argument type 'Int32'}}
   cfunc2(17, i) // expected-error{{cannot convert value of type 'Int' to expected argument type 'Int32'}}