Merge pull request #26710 from LucianoPAlmeida/SR-11295-warning-unecessary-casts

[Diagnostics][Qol] SR-11295 Emit diagnostics for same type coercion.

Author: xedin

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

@@ -4427,9 +4427,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;
   }
 
@@ -5058,6 +5056,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

@@ -709,8 +709,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.
@@ -1462,11 +1462,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:
@@ -1776,6 +1776,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,38 @@ 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) {
+  auto last = locator.last();
+  bool isExplicitCoercion =
+      last && last->is<LocatorPathElt::ExplicitTypeCoercion>();
+  if (!isExplicitCoercion)
+    return false;
+
+  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,10 @@ 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 +1344,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().is<LocatorPathElt::ExplicitTypeCoercion>()) {
     if (!anchor)
       return false;
 
@@ -3218,9 +3219,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();
     }
   }
 
@@ -8029,6 +8037,7 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
   case FixKind::GenericArgumentsMismatch:
   case FixKind::AllowMutatingMemberOnRValueBase:
   case FixKind::AllowTupleSplatForSingleParameter:
+  case FixKind::RemoveUnnecessaryCoercion:
     llvm_unreachable("handled elsewhere");
   }
 
@@ -8335,11 +8344,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:
     case Condition:
       auto numValues = numNumericValuesInPathElement(elt.getKind());
       for (unsigned i = 0; i < numValues; ++i)
@@ -133,6 +134,7 @@ unsigned LocatorPathElt::getNewSummaryFlags() const {
   case ConstraintLocator::KeyPathRoot:
   case ConstraintLocator::KeyPathValue:
   case ConstraintLocator::KeyPathComponentResult:
+  case ConstraintLocator::ExplicitTypeCoercion:
   case ConstraintLocator::Condition:
     return 0;
 
@@ -458,7 +460,11 @@ void ConstraintLocator::dump(SourceManager *sm, raw_ostream &out) {
     case KeyPathComponentResult:
       out << "key path component result";
       break;
-
+        
+    case ExplicitTypeCoercion:
+      out << "type coercion";
+      break;
+      
     case Condition:
       out << "condition expression";
       break;

lib/Sema/ConstraintLocator.h

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

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)
+
 /// The condition associated with 'if' expression or ternary operator.
 SIMPLE_LOCATOR_PATH_ELT(Condition)
 

lib/Sema/ConstraintSystem.cpp

@@ -3000,6 +3000,11 @@ void constraints::simplifyLocator(Expr *&anchor,
       path = path.slice(1);
       continue;
     }
+        
+    case ConstraintLocator::ExplicitTypeCoercion: {
+      path = path.slice(1);
+      continue;
+    }
 
     default:
       // FIXME: Lots of other cases to handle.