[SR-11295] [Re-apply] Unnecessary Coercion explicitly same type coercion warning

include/swift/AST/DiagnosticsSema.def

@@ -998,23 +998,29 @@ 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(literal_conditional_downcast_to_coercion,none,
         "conditional downcast from literal to %0 always fails; "
         "consider using 'as' coercion",
         (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.

include/swift/Basic/LangOptions.h

@@ -303,6 +303,10 @@ namespace swift {
     /// If set to true, the diagnosis engine can assume the emitted diagnostics
     /// will be used in editor. This usually leads to more aggressive fixit.
     bool DiagnosticsEditorMode = false;
+
+    /// If set to true, disable redundant coercions e.g. Double(1) as Double
+    /// warning diagnostics.
+    bool DisableRedundantCoercionWarning = false;
 
     /// Whether to enable Swift 3 @objc inference, e.g., for members of
     /// Objective-C-derived classes and 'dynamic' members.

include/swift/Option/FrontendOptions.td

@@ -667,7 +667,7 @@ def prebuilt_module_cache_path_EQ :
   Alias<prebuilt_module_cache_path>;
 
 def force_public_linkage : Flag<["-"], "force-public-linkage">,
-   HelpText<"Force public linkage for private symbols. Used by LLDB.">;
+  HelpText<"Force public linkage for private symbols. Used by LLDB.">;
 
 def dump_api_path : Separate<["-"], "dump-api-path">,
   HelpText<"The path to output swift interface files for the compiled source files">;
@@ -676,14 +676,17 @@ def group_info_path : Separate<["-"], "group-info-path">,
   HelpText<"The path to collect the group information of the compiled module">;
 
 def diagnostics_editor_mode : Flag<["-"], "diagnostics-editor-mode">,
-HelpText<"Diagnostics will be used in editor">;
+  HelpText<"Diagnostics will be used in editor">;
+
+def disable_redundant_coercion_warning : Flag<["-"], "disable-redundant-coercion-warning">,
+  HelpText<"Disable redundant coercion warning diagnostic">;
 
 def validate_tbd_against_ir_EQ: Joined<["-"], "validate-tbd-against-ir=">,
-    HelpText<"Compare the symbols in the IR against the TBD file that would be generated.">,
-    MetaVarName<"<level>">;
+  HelpText<"Compare the symbols in the IR against the TBD file that would be generated.">,
+  MetaVarName<"<level>">;
 
 def bypass_batch_mode_checks: Flag<["-"], "bypass-batch-mode-checks">,
-    HelpText<"Bypass checks for batch-mode errors.">;
+  HelpText<"Bypass checks for batch-mode errors.">;
 
 def enable_verify_exclusivity : Flag<["-"], "enable-verify-exclusivity">,
   HelpText<"Enable verification of access markers used to enforce exclusivity.">;

lib/Frontend/CompilerInvocation.cpp

@@ -381,6 +381,9 @@ static bool ParseLangArgs(LangOptions &Opts, ArgList &Args,
 
   Opts.DiagnosticsEditorMode |= Args.hasArg(OPT_diagnostics_editor_mode,
                                             OPT_serialize_diagnostics_path);
+
+  Opts.DisableRedundantCoercionWarning |=
+    Args.hasArg(OPT_disable_redundant_coercion_warning);
 
   Opts.EnableExperimentalStaticAssert |=
     Args.hasArg(OPT_enable_experimental_static_assert);

lib/Sema/CSDiagnostics.cpp

@@ -753,6 +753,11 @@ bool GenericArgumentsMismatchFailure::diagnoseAsError() {
       break;
     }
 
+    case ConstraintLocator::ExplicitTypeCoercion: {
+      diagnostic = diag::cannot_convert_coerce;
+      break;
+    }
+
     default:
       break;
     }
@@ -2069,7 +2074,8 @@ bool ContextualFailure::diagnoseAsError() {
     diagnostic = diag::cannot_convert_condition_value;
     break;
   }
-
+
+  case ConstraintLocator::ExplicitTypeCoercion:
   case ConstraintLocator::InstanceType: {
     if (diagnoseCoercionToUnrelatedType())
       return true;
@@ -5540,6 +5546,31 @@ bool ThrowingFunctionConversionFailure::diagnoseAsError() {
   return true;
 }
 
+bool UnnecessaryCoercionFailure::diagnoseAsError() {
+  auto expr = getAsExpr<CoerceExpr>(getAnchor());
+  auto sourceRange =
+      SourceRange(expr->getLoc(), expr->getCastTypeRepr()->getSourceRange().End);
+  auto castType = getType(expr->getCastTypeRepr());
+
+  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(diag::unnecessary_same_typealias_type_coercion,
+                     getFromType(), castType)
+          .fixItRemove(sourceRange);
+      return true;
+    }
+  }
+
+  emitDiagnostic(diag::unnecessary_same_type_coercion,
+                 castType)
+      .fixItRemove(sourceRange);
+  return true;
+}
+
 bool InOutConversionFailure::diagnoseAsError() {
   auto *locator = getLocator();
   auto path = locator->getPath();

lib/Sema/CSDiagnostics.h

@@ -1791,6 +1791,27 @@ class ExpandArrayIntoVarargsFailure final : public ContextualFailure {
   void tryDropArrayBracketsFixIt(const 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(const Solution &solution,
+                             Type fromType, Type toType,
+                             ConstraintLocator *locator)
+      : ContextualFailure(solution, fromType, toType, locator) {}
+
+  bool diagnoseAsError() override;
+};
+
 /// Diagnose a situation there is a mismatch between argument and parameter
 /// types e.g.:
 ///

lib/Sema/CSFix.cpp

@@ -1136,6 +1136,76 @@ IgnoreContextualType *IgnoreContextualType::create(ConstraintSystem &cs,
       IgnoreContextualType(cs, resultTy, specifiedTy, locator);
 }
 
+bool RemoveUnnecessaryCoercion::diagnose(const Solution &solution, bool asNote) const {
+  UnnecessaryCoercionFailure failure(solution, getFromType(), getToType(),
+                                     getLocator());
+  return failure.diagnose(asNote);
+}
+
+bool RemoveUnnecessaryCoercion::attempt(ConstraintSystem &cs, Type fromType,
+                                        Type toType,
+                                        ConstraintLocatorBuilder locator) {
+  auto &ctx = cs.getASTContext();
+  if (ctx.LangOpts.DisableRedundantCoercionWarning)
+    return false;
+
+  auto last = locator.last();
+  bool isExplicitCoercion =
+      last && last->is<LocatorPathElt::ExplicitTypeCoercion>();
+  if (!isExplicitCoercion)
+    return false;
+
+  auto expr = getAsExpr<CoerceExpr>(locator.getAnchor());
+
+  // Check to ensure the from and to types are equal the cast type.
+  // This is required to handle cases where the conversion is done
+  // using compiler intrinsics of _HasCustomAnyHashableRepresentation
+  // to AnyHashable where if we coerce a generic type that conforms to
+  // this protocol to AnyHashable we match equal types here, but the
+  // explicit coercion is still required.
+  auto castType = cs.getType(expr->getCastTypeRepr());
+  if (!fromType->isEqual(castType) && cs.isAnyHashableType(castType))
+    return false;
+
+  auto toTypeRepr = expr->getCastTypeRepr();
+
+  // Don't emit this diagnostic for Implicitly unwrapped optional types
+  // e.g. i as Int!
+  if (isa<ImplicitlyUnwrappedOptionalTypeRepr>(toTypeRepr))
+    return false;
+
+  // If we are dealing with an OverloadedDeclRefExpr the coercion
+  // is probably being used to match a specific overload function type.
+  bool hasOverloadedDeclRefSubExpr = false;
+  expr->forEachChildExpr([&](Expr *childExpr) -> Expr *{
+    if (isa<OverloadedDeclRefExpr>(childExpr)) {
+      hasOverloadedDeclRefSubExpr = true;
+      return nullptr;
+    }
+    return childExpr;
+  });
+
+  if (hasOverloadedDeclRefSubExpr)
+    return false;
+
+  auto coercedType = cs.getType(expr->getSubExpr());
+  if (auto *typeVariable = coercedType->getAs<TypeVariableType>()) {
+    auto representative = cs.getRepresentative(typeVariable);
+    if (auto *typeSourceLocator =
+            cs.getTypeVariableBindingLocator(representative)) {
+      // If the type variable binding source locator is the same the
+      // contextual type equality is coming from this coercion.
+      if (typeSourceLocator == cs.getConstraintLocator(locator))
+        return false;
+    }
+  }
+
+  auto *fix = new (cs.getAllocator()) RemoveUnnecessaryCoercion(
+      cs, fromType, toType, cs.getConstraintLocator(locator));
+
+  return cs.recordFix(fix);
+}
+
 bool IgnoreAssignmentDestinationType::diagnose(const Solution &solution,
                                                bool asNote) const {
   auto &cs = getConstraintSystem();

lib/Sema/CSFix.h

@@ -204,6 +204,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.
@@ -1561,6 +1565,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(const Solution &solution, 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

@@ -1949,7 +1949,7 @@ ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2,
     auto *typeVar = createTypeVariable(getConstraintLocator(argLoc),
                                        TVO_CanBindToNoEscape);
     params.emplace_back(typeVar);
-    assignFixedType(typeVar, input);
+    assignFixedType(typeVar, input, getConstraintLocator(argLoc));
   };
 
   {
@@ -2825,7 +2825,7 @@ ConstraintSystem::matchTypesBindTypeVar(
                : getTypeMatchFailure(locator);
   }
 
-  assignFixedType(typeVar, type);
+  assignFixedType(typeVar, type, getConstraintLocator(locator));
 
   return getTypeMatchSuccess();
 }
@@ -3424,7 +3424,8 @@ bool ConstraintSystem::repairFailures(
     });
   };
 
-  if (path.empty()) {
+  if (path.empty() ||
+      path.back().is<LocatorPathElt::ExplicitTypeCoercion>()) {
     if (!anchor)
       return false;
 
@@ -4465,9 +4466,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();
     }
   }
 
@@ -4554,9 +4562,10 @@ ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind,
         if (auto *iot = type1->getAs<InOutType>()) {
           if (!rep2->getImpl().canBindToLValue())
             return getTypeMatchFailure(locator);
-          assignFixedType(rep2, LValueType::get(iot->getObjectType()));
+          assignFixedType(rep2, LValueType::get(iot->getObjectType()),
+                          getConstraintLocator(locator));
         } else {
-          assignFixedType(rep2, type1);
+          assignFixedType(rep2, type1, getConstraintLocator(locator));
         }
         return getTypeMatchSuccess();
       } else if (typeVar1 && !typeVar2) {
@@ -4569,9 +4578,10 @@ ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind,
         if (auto *lvt = type2->getAs<LValueType>()) {
           if (!rep1->getImpl().canBindToInOut())
             return getTypeMatchFailure(locator);
-          assignFixedType(rep1, InOutType::get(lvt->getObjectType()));
+          assignFixedType(rep1, InOutType::get(lvt->getObjectType()),
+                          getConstraintLocator(locator));
         } else {
-          assignFixedType(rep1, type2);
+          assignFixedType(rep1, type2, getConstraintLocator(locator));
         }
         return getTypeMatchSuccess();
       } if (typeVar1 && typeVar2) {
@@ -10112,6 +10122,7 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
   case FixKind::SpecifyClosureParameterType:
   case FixKind::SpecifyClosureReturnType:
   case FixKind::AddQualifierToAccessTopLevelName:
+  case FixKind::RemoveUnnecessaryCoercion:
     llvm_unreachable("handled elsewhere");
   }
 
@@ -10497,11 +10508,18 @@ void ConstraintSystem::addExplicitConversionConstraint(
   SmallVector<Constraint *, 3> constraints;
 
   auto locatorPtr = getConstraintLocator(locator);
+  ConstraintLocator *coerceLocator = locatorPtr;
 
+  auto *anchor = getAsExpr(locator.getAnchor());
+  if (anchor && 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);