Merge pull request #4421 from slavapestov/small-cleanups

Small cleanups

Author: slavapestov

include/swift/AST/ASTContext.h

@@ -206,8 +206,8 @@ class ASTContext {
   /// The name of the SwiftShims module "SwiftShims".
   Identifier SwiftShimsModuleName;
 
-  /// Note: in non-NDEBUG builds, tracks the context of each archetype
-  /// type, which can be very useful for debugging.
+  /// Note: in non-NDEBUG builds, tracks the context of each primary
+  /// archetype type, which can be very useful for debugging.
   llvm::DenseMap<ArchetypeType *, DeclContext *> ArchetypeContexts;
 
   // Define the set of known identifiers.

include/swift/AST/Types.h

@@ -3748,12 +3748,10 @@ class ArchetypeType final : public SubstitutableType,
   /// True if this is the 'Self' parameter of a protocol or an associated type
   /// of 'Self'.
   bool isSelfDerived() {
-    ArchetypeType *t = this;
+    ArchetypeType *t = getPrimary();
 
-    do {
-      if (t->getSelfProtocol())
-        return true;
-    } while ((t = t->getParent()));
+    if (t && t->getSelfProtocol())
+      return true;
 
     return false;
   }
@@ -3810,6 +3808,16 @@ class ArchetypeType final : public SubstitutableType,
     return ParentOrOpened.isNull();
   }
 
+  /// getPrimary - Return the primary archetype parent of this archetype.
+  ArchetypeType *getPrimary() const {
+    assert(!getOpenedExistentialType() && "Check for opened existential first");
+
+    auto *archetype = this;
+    while (auto *parent = archetype->getParent())
+      archetype = parent;
+    return const_cast<ArchetypeType *>(archetype);
+  }
+
   /// Retrieve the ID number of this opened existential.
   UUID getOpenedExistentialID() const {
     assert(getOpenedExistentialType() && "Not an opened existential archetype");

lib/AST/ASTContext.cpp

@@ -2601,6 +2601,10 @@ void ASTContext::dumpArchetypeContext(ArchetypeType *archetype,
 void ASTContext::dumpArchetypeContext(ArchetypeType *archetype,
                                       llvm::raw_ostream &os,
                                       unsigned indent) const {
+  archetype = archetype->getPrimary();
+  if (!archetype)
+    return;
+
   auto knownDC = ArchetypeContexts.find(archetype);
   if (knownDC != ArchetypeContexts.end())
     knownDC->second->printContext(os, indent);

lib/AST/ASTVerifier.cpp

@@ -78,7 +78,7 @@ struct ASTNodeBase {};
     using ScopeLike = llvm::PointerUnion<DeclContext *, BraceStmt *>;
     SmallVector<ScopeLike, 4> Scopes;
 
-    /// The set of archetypes that are currently available.
+    /// The set of primary archetypes that are currently available.
     SmallPtrSet<ArchetypeType *, 4> ActiveArchetypes;
 
     /// \brief The stack of optional evaluations active at this point.
@@ -106,8 +106,8 @@ struct ASTNodeBase {};
       for (auto genericParams = dc->getGenericParamsOfContext();
            genericParams;
            genericParams = genericParams->getOuterParameters()) {
-        ActiveArchetypes.insert(genericParams->getAllArchetypes().begin(),
-                                genericParams->getAllArchetypes().end());
+        for (auto *param : genericParams->getParams())
+          ActiveArchetypes.insert(param->getArchetype());
       }
     }
 
@@ -446,8 +446,11 @@ struct ASTNodeBase {};
             return false;
           }
 
+          // Get the primary archetype.
+          auto *parent = archetype->getPrimary();
+
           // Otherwise, the archetype needs to be from this scope.
-          if (ActiveArchetypes.count(archetype) == 0) {
+          if (ActiveArchetypes.count(parent) == 0) {
             // FIXME: Make an exception for serialized extensions, which don't
             // currently have the correct archetypes.
             if (auto activeScope = Scopes.back().dyn_cast<DeclContext *>()) {
@@ -463,7 +466,7 @@ struct ASTNodeBase {};
             Out << "AST verification error: archetype "
                 << archetype->getString() << " not allowed in this context\n";
 
-            auto knownDC = Ctx.ArchetypeContexts.find(archetype);
+            auto knownDC = Ctx.ArchetypeContexts.find(parent);
             if (knownDC != Ctx.ArchetypeContexts.end()) {
               llvm::errs() << "archetype came from:\n";
               knownDC->second->dumpContext();
@@ -534,8 +537,9 @@ struct ASTNodeBase {};
 
       // Add any archetypes from this scope into the set of active archetypes.
       if (auto genericParams = getImmediateGenericParams(scope))
-        ActiveArchetypes.insert(genericParams->getAllArchetypes().begin(),
-                                genericParams->getAllArchetypes().end());
+        for (auto *param : genericParams->getParams())
+          if (auto *archetype = param->getArchetype())
+            ActiveArchetypes.insert(archetype);
     }
     void pushScope(BraceStmt *scope) {
       Scopes.push_back(scope);
@@ -546,7 +550,11 @@ struct ASTNodeBase {};
       // Remove archetypes from this scope from the set of active archetypes.
       if (auto genericParams
             = getImmediateGenericParams(Scopes.back().get<DeclContext*>())) {
-        for (auto archetype : genericParams->getAllArchetypes()) {
+        for (auto *param : genericParams->getParams()) {
+          auto *archetype = param->getArchetype();
+          if (archetype == nullptr)
+            continue;
+
           if (!ActiveArchetypes.erase(archetype)) {
             llvm::errs() << "archetype " << archetype
                          << " not introduced by scope?\n";

lib/SIL/SILVerifier.cpp

@@ -49,27 +49,27 @@ static llvm::cl::opt<bool> SkipUnreachableMustBeLastErrors(
 // prevent release builds from triggering spurious unused variable warnings.
 #ifndef NDEBUG
 
-/// Returns true if A is an opened existential type, Self, or is equal to an
-/// archetype in F's nested archetype list.
-///
-/// FIXME: Once Self has been removed in favor of opened existential types
-/// everywhere, remove support for self.
+/// Returns true if A is an opened existential type or is equal to an
+/// archetype from F's generic context.
 static bool isArchetypeValidInFunction(ArchetypeType *A, SILFunction *F) {
-  // The only two cases where an archetype is always legal in a function is if
-  // it is self or if it is from an opened existential type. Currently, Self is
-  // being migrated away from in favor of opened existential types, so we should
-  // remove the special case here for Self when that process is completed.
-  //
-  // *NOTE* Associated types of self are not valid here.
-  if (!A->getOpenedExistentialType().isNull() || A->getSelfProtocol())
+  if (!A->getOpenedExistentialType().isNull())
     return true;
 
-  // Ok, we have an archetype, make sure it is in the nested archetypes of our
-  // caller.
-  for (auto Iter : F->getContextGenericParams()->getAllNestedArchetypes())
-    if (A->isEqual(&*Iter))
-      return true;
-  return A->getIsRecursive();
+  // Find the primary archetype.
+  A = A->getPrimary();
+
+  // Ok, we have a primary archetype, make sure it is in the nested generic
+  // parameters of our caller.
+  for (auto *params = F->getContextGenericParams();
+       params != nullptr;
+       params = params->getOuterParameters()) {
+
+    for (auto param : params->getParams())
+      if (param->getArchetype()->isEqual(A))
+        return true;
+  }
+
+  return false;
 }
 
 namespace {

lib/Sema/CSDiag.cpp

@@ -1982,6 +1982,7 @@ class FailureDiagnosis :public ASTVisitor<FailureDiagnosis, /*exprresult*/bool>{
 
   bool visitExpr(Expr *E);
   bool visitIdentityExpr(IdentityExpr *E);
+  bool visitTryExpr(TryExpr *E);
   bool visitTupleExpr(TupleExpr *E);
 
   bool visitUnresolvedMemberExpr(UnresolvedMemberExpr *E);
@@ -6277,6 +6278,12 @@ bool FailureDiagnosis::visitIdentityExpr(IdentityExpr *E) {
   return false;
 }
 
+/// A TryExpr doesn't change it's argument, nor does it change the contextual
+/// type.
+bool FailureDiagnosis::visitTryExpr(TryExpr *E) {
+  return visit(E->getSubExpr());
+}
+
 bool FailureDiagnosis::visitExpr(Expr *E) {
   // Check each of our immediate children to see if any of them are
   // independently invalid.
@@ -6331,7 +6338,7 @@ void ConstraintSystem::diagnoseFailureForExpr(Expr *expr) {
   // inconsistent state.
   simplify(/*ContinueAfterFailures*/true);
 
-  // Look through RebindSelfInConstructorExpr to avoid weird sema issues.
+  // Look through RebindSelfInConstructorExpr to avoid weird Sema issues.
   if (auto *RB = dyn_cast<RebindSelfInConstructorExpr>(expr))
     expr = RB->getSubExpr();
 

lib/Sema/TypeCheckGeneric.cpp

@@ -815,7 +815,8 @@ void TypeChecker::finalizeGenericParamList(ArchetypeBuilder &builder,
 
 #ifndef NDEBUG
   // Record archetype contexts.
-  for (auto archetype : genericParams->getAllArchetypes()) {
+  for (auto *param : genericParams->getParams()) {
+    auto *archetype = param->getArchetype();
     if (Context.ArchetypeContexts.count(archetype) == 0)
       Context.ArchetypeContexts[archetype] = dc;
   }

lib/Sema/TypeCheckType.cpp

@@ -1410,9 +1410,9 @@ static bool isDefaultNoEscapeContext(const DeclContext *DC) {
 }
 
 // Hack to apply context-specific @escaping to an AST function type.
-static Type adjustFunctionExtInfo(DeclContext *DC,
-                                  Type ty,
-                                  TypeResolutionOptions options) {
+static Type applyNonEscapingFromContext(DeclContext *DC,
+                                        Type ty,
+                                        TypeResolutionOptions options) {
   // Remember whether this is a function parameter.
   bool isFunctionParam =
     options.contains(TR_FunctionInput) ||
@@ -1426,7 +1426,11 @@ static Type adjustFunctionExtInfo(DeclContext *DC,
   if (defaultNoEscape && !extInfo.isNoEscape()) {
     extInfo = extInfo.withNoEscape();
 
-    // We lost the sugar to flip the isNoEscape bit
+    // We lost the sugar to flip the isNoEscape bit.
+    //
+    // FIXME: It would be better to add a new AttributedType sugared type,
+    // which would wrap the NameAliasType or ParenType, and apply the
+    // isNoEscape bit when de-sugaring.
     return FunctionType::get(funcTy->getInput(), funcTy->getResult(), extInfo);
   }
 
@@ -1467,7 +1471,7 @@ Type TypeChecker::resolveIdentifierType(
   // Hack to apply context-specific @escaping to a typealias with an underlying
   // function type.
   if (result->is<FunctionType>())
-    result = adjustFunctionExtInfo(DC, result, options);
+    result = applyNonEscapingFromContext(DC, result, options);
 
   // We allow a type to conform to a protocol that is less available than
   // the type itself. This enables a type to retroactively model or directly
@@ -1716,7 +1720,7 @@ Type TypeResolver::resolveType(TypeRepr *repr, TypeResolutionOptions options) {
       // Default non-escaping for closure parameters
       auto result = resolveASTFunctionType(cast<FunctionTypeRepr>(repr), options);
       if (result && result->is<FunctionType>())
-        return adjustFunctionExtInfo(DC, result, options);
+        return applyNonEscapingFromContext(DC, result, options);
       return result;
     }
     return resolveSILFunctionType(cast<FunctionTypeRepr>(repr), options);
@@ -1987,10 +1991,6 @@ Type TypeResolver::resolveAttributedType(TypeAttributes &attrs,
           .fixItReplace(resultRange, "Never");
     }
 
-    if (attrs.has(TAK_noescape)) {
-      // FIXME: diagnostic to tell user this is redundant and drop it
-    }
-
     // Resolve the function type directly with these attributes.
     FunctionType::ExtInfo extInfo(rep,
                                   attrs.has(TAK_autoclosure),
@@ -2030,7 +2030,7 @@ Type TypeResolver::resolveAttributedType(TypeAttributes &attrs,
       attrs.clearAttribute(TAK_escaping);
     } else {
       // No attribute; set the isNoEscape bit if we're in parameter context.
-      ty = adjustFunctionExtInfo(DC, ty, options);
+      ty = applyNonEscapingFromContext(DC, ty, options);
     }
   }
 
@@ -2048,6 +2048,7 @@ Type TypeResolver::resolveAttributedType(TypeAttributes &attrs,
       }
     }
   } else if (hasFunctionAttr && fnRepr) {
+    // Remove the function attributes from the set so that we don't diagnose.
     for (auto i : FunctionAttrs)
       attrs.clearAttribute(i);
     attrs.convention = None;
@@ -2504,6 +2505,11 @@ Type TypeResolver::resolveTupleType(TupleTypeRepr *repr,
 
   // If this is the top level of a function input list, peel off the
   // ImmediateFunctionInput marker and install a FunctionInput one instead.
+  //
+  // If we have a single ParenType though, don't clear these bits; we
+  // still want to parse the type contained therein as if it were in
+  // parameter position, meaning function types are not @escaping by
+  // default.
   auto elementOptions = options;
   if (!repr->isParenType()) {
     elementOptions = withoutContext(elementOptions);

test/Constraints/diagnostics.swift

@@ -769,3 +769,14 @@ struct SR1752 {
 let sr1752: SR1752? = nil
 
 true ? nil : sr1752?.foo() // don't generate a warning about unused result since foo returns Void
+
+// <rdar://problem/27891805> QoI: FailureDiagnosis doesn't look through 'try'
+struct rdar27891805 {
+  init(contentsOf: String, encoding: String) throws {}
+  init(contentsOf: String, usedEncoding: inout String) throws {}
+  init<T>(_ t: T) {}
+}
+
+try rdar27891805(contentsOfURL: nil, usedEncoding: nil)
+// expected-error@-1 {{argument labels '(contentsOfURL:, usedEncoding:)' do not match any available overloads}}
+// expected-note@-2 {{overloads for 'rdar27891805' exist with these partially matching parameter lists: (contentsOf: String, encoding: String), (contentsOf: String, usedEncoding: inout String)}}