Merge pull request #6064 from slavapestov/killing-substituted-type-access-check

Refactor accessibility checking to use TypeReprs (most of the time) rather than Types

Author: slavapestov

include/swift/AST/AccessScope.h

@@ -68,8 +68,11 @@ class AccessScope {
   /// Returns the narrowest access scope if this and the specified access scope
   /// have common intersection, or None if scopes don't intersect.
   const Optional<AccessScope> intersectWith(AccessScope accessScope) const {
-    if (hasEqualDeclContextWith(accessScope))
-      return *this;
+    if (hasEqualDeclContextWith(accessScope)) {
+      if (isPrivate())
+        return *this;
+      return accessScope;
+    }
     if (isChildOf(accessScope))
       return *this;
     if (accessScope.isChildOf(*this))

lib/Sema/TypeCheckDecl.cpp

@@ -1134,67 +1134,96 @@ static void configureImplicitSelf(TypeChecker &tc,
 
 namespace {
 
-class TypeAccessScopeChecker : private TypeWalker {
-  using TypeAccessScopeCacheMap = TypeChecker::TypeAccessScopeCacheMap;
-  TypeAccessScopeCacheMap &Cache;
+class AccessScopeChecker {
   const SourceFile *File;
-  SmallVector<Optional<AccessScope>, 8> RawScopeStack;
+  TypeChecker::TypeAccessScopeCacheMap &Cache;
 
-  explicit TypeAccessScopeChecker(TypeAccessScopeCacheMap &cache,
-                                  const SourceFile *file)
-      : Cache(cache), File(file) {
-    // Always have something on the stack.
-    RawScopeStack.push_back(None);
-  }
+protected:
+  Optional<AccessScope> Scope;
+
+  AccessScopeChecker(const DeclContext *useDC,
+                     decltype(TypeChecker::TypeAccessScopeCache) &caches)
+      : File(useDC->getParentSourceFile()), Cache(caches[File]),
+        Scope(AccessScope::getPublic()) {}
 
-  bool shouldVisitOriginalSubstitutedType() override { return true; }
+  bool visitDecl(ValueDecl *VD) {
+    if (!VD || isa<GenericTypeParamDecl>(VD))
+      return true;
 
-  Action walkToTypePre(Type ty) override {
-    // Assume failure until we post-visit this node.
-    // This will be correct as long as we don't ever have self-referential
-    // Types.
-    auto cached = Cache.find(ty);
+    // FIXME: Figure out why AssociatedTypeDecls don't always have
+    // accessibility here.
+    if (!VD->hasAccessibility()) {
+      if (isa<AssociatedTypeDecl>(VD))
+        return true;
+    }
+
+    auto cached = Cache.find(VD);
     if (cached != Cache.end()) {
-      Optional<AccessScope> &last = RawScopeStack.back();
-      if (last.hasValue())
-        last = last.getValue().intersectWith(cached->second);
-      return Action::SkipChildren;
+      Scope = Scope->intersectWith(cached->second);
+      return Scope.hasValue();
     }
 
-    auto AS = AccessScope::getPublic();
-    if (auto alias = dyn_cast<NameAliasType>(ty.getPointer()))
-      AS = alias->getDecl()->getFormalAccessScope(File);
-    else if (auto nominal = ty->getAnyNominal())
-      AS = nominal->getFormalAccessScope(File);
-    RawScopeStack.push_back(AS);
+    auto AS = VD->getFormalAccessScope(File);
+    auto result = Cache.insert(std::make_pair(VD, AS));
+    assert(result.second);
+    (void) result;
 
-    return Action::Continue;
+    Scope = Scope->intersectWith(AS);
+    return Scope.hasValue();
   }
+};
 
-  Action walkToTypePost(Type ty) override {
-    Optional<AccessScope> last = RawScopeStack.pop_back_val();
-    if (last.hasValue()) {
-      Cache.insert(std::make_pair(ty, *last));
+class TypeReprAccessScopeChecker : private ASTWalker, AccessScopeChecker {
+  TypeReprAccessScopeChecker(const DeclContext *useDC,
+                             decltype(TypeChecker::TypeAccessScopeCache) &caches)
+      : AccessScopeChecker(useDC, caches) {}
 
-      Optional<AccessScope> &prev = RawScopeStack.back();
-      if (prev.hasValue())
-        prev = prev.getValue().intersectWith(*last);
-    }
+  bool walkToTypeReprPre(TypeRepr *TR) override {
+    auto CITR = dyn_cast<ComponentIdentTypeRepr>(TR);
+    if (!CITR)
+      return true;
+
+    return visitDecl(CITR->getBoundDecl());
+  }
 
-    return Action::Continue;
+  bool walkToTypeReprPost(TypeRepr *TR) override {
+    return Scope.hasValue();
   }
 
 public:
   static Optional<AccessScope>
-  getAccessScope(Type ty, const DeclContext *useDC,
+  getAccessScope(TypeRepr *TR, const DeclContext *useDC,
                  decltype(TypeChecker::TypeAccessScopeCache) &caches) {
-    const SourceFile *file = useDC->getParentSourceFile();
-    auto &cache = caches[file];
-    ty.walk(TypeAccessScopeChecker(cache, file));
-    auto iter = cache.find(ty);
-    if (iter == cache.end())
-      return None;
-    return iter->second;
+    TypeReprAccessScopeChecker checker(useDC, caches);
+    TR->walk(checker);
+    return checker.Scope;
+  }
+};
+
+class TypeAccessScopeChecker : private TypeWalker, AccessScopeChecker {
+  TypeAccessScopeChecker(const DeclContext *useDC,
+                         decltype(TypeChecker::TypeAccessScopeCache) &caches)
+      : AccessScopeChecker(useDC, caches) {}
+
+  Action walkToTypePre(Type T) {
+    ValueDecl *VD;
+    if (auto *TAD = dyn_cast<NameAliasType>(T.getPointer()))
+      VD = TAD->getDecl();
+    else if (auto *NTD = T->getAnyNominal())
+      VD = NTD;
+    else
+      VD = nullptr;
+
+    return visitDecl(VD) ? Action::Continue : Action::Stop;
+  }
+
+public:
+  static Optional<AccessScope>
+  getAccessScope(Type T, const DeclContext *useDC,
+                 decltype(TypeChecker::TypeAccessScopeCache) &caches) {
+    TypeAccessScopeChecker checker(useDC, caches);
+    T.walk(checker);
+    return checker.Scope;
   }
 };
 
@@ -1223,9 +1252,9 @@ void TypeChecker::computeDefaultAccessibility(ExtensionDecl *ED) {
       if (!TL.getType())
         return Accessibility::Public;
       auto accessScope =
-          TypeAccessScopeChecker::getAccessScope(TL.getType(),
-                                                 ED->getDeclContext(),
-                                                 TypeAccessScopeCache);
+          TypeReprAccessScopeChecker::getAccessScope(TL.getTypeRepr(),
+                                                     ED->getDeclContext(),
+                                                     TypeAccessScopeCache);
       // This is an error case and will be diagnosed elsewhere.
       if (!accessScope.hasValue())
         return Accessibility::Public;
@@ -1406,6 +1435,8 @@ class TypeAccessScopeDiagnoser : private ASTWalker {
                                            const DeclContext *useDC) {
     assert(!accessScope.isPublic() &&
            "why would we need to find a public access scope?");
+    if (TR == nullptr)
+      return nullptr;
     TypeAccessScopeDiagnoser diagnoser(accessScope, useDC);
     TR->walk(diagnoser);
     return diagnoser.offendingType;
@@ -1446,9 +1477,15 @@ static void checkTypeAccessibilityImpl(
       contextAccessScope.getDeclContext()->isLocalContext())
     return;
 
+  // TypeRepr checking is more accurate, but we must also look at TypeLocs
+  // without a TypeRepr, for example for 'var' declarations with an inferred
+  // type.
   auto typeAccessScope =
-    TypeAccessScopeChecker::getAccessScope(TL.getType(), useDC,
-                                           TC.TypeAccessScopeCache);
+    (TL.getTypeRepr()
+     ? TypeReprAccessScopeChecker::getAccessScope(TL.getTypeRepr(), useDC,
+                                                  TC.TypeAccessScopeCache)
+     : TypeAccessScopeChecker::getAccessScope(TL.getType(), useDC,
+                                              TC.TypeAccessScopeCache));
 
   // Note: This means that the type itself is invalid for this particular
   // context, because it references declarations from two incompatible scopes.
@@ -1461,12 +1498,11 @@ static void checkTypeAccessibilityImpl(
       contextAccessScope.isChildOf(*typeAccessScope))
     return;
 
-  const TypeRepr *complainRepr = nullptr;
-  if (TypeRepr *TR = TL.getTypeRepr()) {
-    complainRepr =
-        TypeAccessScopeDiagnoser::findTypeWithScope(TR, *typeAccessScope,
-                                                    useDC);
-  }
+  const TypeRepr *complainRepr =
+        TypeAccessScopeDiagnoser::findTypeWithScope(
+            TL.getTypeRepr(),
+            *typeAccessScope,
+            useDC);
   diagnose(*typeAccessScope, complainRepr);
 }
 

lib/Sema/TypeCheckProtocol.cpp

@@ -2268,9 +2268,7 @@ void ConformanceChecker::recordTypeWitness(AssociatedTypeDecl *assocType,
     aliasDecl->getAliasType()->setRecursiveProperties(
         type->getRecursiveProperties());
 
-    Type interfaceTy = aliasDecl->getAliasType();
-    if (interfaceTy->hasArchetype())
-      interfaceTy = ArchetypeBuilder::mapTypeOutOfContext(DC, type);
+    auto interfaceTy = DC->mapTypeOutOfContext(aliasDecl->getAliasType());
     aliasDecl->setInterfaceType(MetatypeType::get(interfaceTy));
 
     aliasDecl->setImplicit();

lib/Sema/TypeCheckType.cpp

@@ -570,13 +570,7 @@ Type TypeChecker::applyUnboundGenericArguments(
       type = ArchetypeBuilder::mapTypeOutOfContext(TAD, TAD->getUnderlyingType());
     }
 
-    type = type.subst(dc->getParentModule(), subs, SubstFlags::UseErrorType);
-
-    // FIXME: return a SubstitutedType to preserve the fact that
-    // we resolved a generic TypeAlias, for availability diagnostics.
-    // A better fix might be to introduce a BoundGenericAliasType
-    // which desugars as appropriate.
-    return SubstitutedType::get(TAD->getAliasType(), type, Context);
+    return type.subst(dc->getParentModule(), subs, SubstFlags::UseErrorType);
   }
 
   // Form the bound generic type.

lib/Sema/TypeChecker.h

@@ -491,16 +491,13 @@ class TypeChecker final : public LazyResolver {
   /// during type checking.
   llvm::SetVector<NominalTypeDecl *> ValidatedTypes;
 
-  using TypeAccessScopeCacheMap = llvm::DenseMap<Type, AccessScope>;
+  using TypeAccessScopeCacheMap = llvm::DenseMap<const ValueDecl *, AccessScope>;
 
-  /// Caches the outermost scope where a particular type can be used, relative
-  /// to a particular file.
+  /// Caches the outermost scope where a particular declaration can be used,
+  /// relative to a particular file.
   ///
   /// The file is used to handle things like \c \@testable. A null-but-present
   /// value means the type is public.
-  ///
-  /// This can't use CanTypes because typealiases may have more limited types
-  /// than their underlying types.
   llvm::DenseMap<const SourceFile *, TypeAccessScopeCacheMap>
     TypeAccessScopeCache;
 

stdlib/public/core/Arrays.swift.gyb

@@ -1771,7 +1771,7 @@ extension ${Self} {
   public mutating func replaceSubrange<C>(
     _ subrange: Range<Int>,
     with newElements: C
-  ) where C : Collection, C.Iterator.Element == _Buffer.Element {
+  ) where C : Collection, C.Iterator.Element == Element {
 % if Self in ['Array', 'ContiguousArray']:
     _precondition(subrange.lowerBound >= self._buffer.startIndex,
       "${Self} replace: subrange start is negative")
@@ -2291,7 +2291,7 @@ extension ${Self} {
   public mutating func replaceRange<C>(
     _ subRange: Range<Int>,
     with newElements: C
-  ) where C : Collection, C.Iterator.Element == _Buffer.Element {
+  ) where C : Collection, C.Iterator.Element == Element {
     Builtin.unreachable()
   }
 

test/Sema/accessibility.swift

@@ -637,3 +637,21 @@ internal struct AssocTypeOuterProblem2 {
     fileprivate typealias Assoc = Int // expected-error {{type alias 'Assoc' must be as accessible as its enclosing type because it matches a requirement in protocol 'AssocTypeProto'}} {{5-16=internal}}
   }
 }
+
+// This code was accepted in Swift 3
+public protocol P {
+  associatedtype Element
+
+  func f() -> Element
+}
+
+struct S<T> : P {
+  func f() -> T { while true {} }
+}
+
+public struct G<T> {
+  typealias A = S<T> // expected-note {{type declared here}}
+
+  public func foo<U : P>(u: U) where U.Element == A.Element {}
+  // expected-error@-1 {{instance method cannot be declared public because its generic requirement uses an internal type}}
+}

test/Sema/accessibility_private.swift

@@ -210,3 +210,24 @@ fileprivate struct SR2579 {
   private var outerProperty = Inner().innerProperty // expected-warning {{property should not be declared in this context because its type 'SR2579.Inner.InnerPrivateType' uses a private type}}
   var outerProperty2 = Inner().innerProperty // expected-warning {{property should be declared private because its type 'SR2579.Inner.InnerPrivateType' uses a private type}}
 }
+
+// FIXME: Dependent member lookup of typealiases is not subject
+// to accessibility checking.
+struct Generic<T> {
+  fileprivate typealias Dependent = T
+}
+
+var x: Generic<Int>.Dependent = 3
+// expected-error@-1 {{variable must be declared private or fileprivate because its type uses a fileprivate type}}
+
+func internalFuncWithFileprivateAlias() -> Generic<Int>.Dependent {
+// expected-error@-1 {{function must be declared private or fileprivate because its result uses a fileprivate type}}
+  return 3
+}
+
+private func privateFuncWithFileprivateAlias() -> Generic<Int>.Dependent {
+  return 3
+}
+
+// FIXME: No error here
+var y = privateFuncWithFileprivateAlias()