Parse: Split off StorageImplInfo computation from accessor parsing

Soon this will be performed lazily as part of a request.

Author: slavapestov

lib/Parse/ParseDecl.cpp

@@ -4470,16 +4470,12 @@ struct Parser::ParsedAccessors {
 #include "swift/AST/AccessorKinds.def"
 
   void record(Parser &P, AbstractStorageDecl *storage, bool invalid,
-              ParseDeclOptions flags, SourceLoc staticLoc,
               const DeclAttributes &attrs,
-              TypeLoc elementTy, ParameterList *indices,
               SmallVectorImpl<Decl *> &decls);
 
   StorageImplInfo
   classify(Parser &P, AbstractStorageDecl *storage, bool invalid,
-           ParseDeclOptions flags, SourceLoc staticLoc,
-           const DeclAttributes &attrs,
-           TypeLoc elementTy, ParameterList *indices);
+           const DeclAttributes &attrs);
 
   /// Add an accessor.  If there's an existing accessor of this kind,
   /// return it.  The new accessor is still remembered but will be
@@ -4912,8 +4908,7 @@ Parser::parseDeclVarGetSet(Pattern *pattern, ParseDeclOptions Flags,
       accessors.Set->setInvalid();
   }
 
-  accessors.record(*this, PrimaryVar, Invalid, Flags, StaticLoc,
-                   Attributes, TyLoc, /*indices*/ nullptr, Decls);
+  accessors.record(*this, PrimaryVar, Invalid, Attributes, Decls);
 
   return makeParserResult(PrimaryVar);
 }
@@ -4940,13 +4935,10 @@ AccessorDecl *Parser::ParsedAccessors::add(AccessorDecl *accessor) {
 
 /// Record a bunch of parsed accessors into the given abstract storage decl.
 void Parser::ParsedAccessors::record(Parser &P, AbstractStorageDecl *storage,
-                                     bool invalid, ParseDeclOptions flags,
-                                     SourceLoc staticLoc,
+                                     bool invalid,
                                      const DeclAttributes &attrs,
-                                     TypeLoc elementTy, ParameterList *indices,
                                      SmallVectorImpl<Decl *> &decls) {
-  auto storageKind = classify(P, storage, invalid, flags, staticLoc, attrs,
-                              elementTy, indices);
+  auto storageKind = classify(P, storage, invalid, attrs);
   storage->setImplInfo(storageKind);
 
   decls.append(Accessors.begin(), Accessors.end());
@@ -4959,13 +4951,6 @@ static void flagInvalidAccessor(AccessorDecl *func) {
   }
 }
 
-static void ignoreInvalidAccessor(AccessorDecl *&func) {
-  if (func) {
-    flagInvalidAccessor(func);
-    func = nullptr;
-  }
-}
-
 static void diagnoseConflictingAccessors(Parser &P, AccessorDecl *first,
                                          AccessorDecl *&second) {
   if (!second) return;
@@ -4976,7 +4961,7 @@ static void diagnoseConflictingAccessors(Parser &P, AccessorDecl *first,
   P.diagnose(first->getLoc(), diag::previous_accessor,
              getAccessorNameForDiagnostic(first, /*article*/ false),
              /*already*/ false);
-  ignoreInvalidAccessor(second);
+  flagInvalidAccessor(second);
 }
 
 template <class... DiagArgs>
@@ -4986,11 +4971,11 @@ static void diagnoseAndIgnoreObservers(Parser &P,
                         typename std::enable_if<true, DiagArgs>::type... args) {
   if (auto &accessor = accessors.WillSet) {
     P.diagnose(accessor->getLoc(), diagnostic, /*willSet*/ 0, args...);
-    ignoreInvalidAccessor(accessor);
+    flagInvalidAccessor(accessor);
   }
   if (auto &accessor = accessors.DidSet) {
     P.diagnose(accessor->getLoc(), diagnostic, /*didSet*/ 1, args...);
-    ignoreInvalidAccessor(accessor);
+    flagInvalidAccessor(accessor);
   }
 }
 
@@ -5013,20 +4998,10 @@ static void diagnoseAndIgnoreObservers(Parser &P,
 ///   - MaterializeToTemporary, if the decl has a setter.
 static StorageImplInfo classifyWithHasStorageAttr(
   Parser::ParsedAccessors &accessors, ASTContext &ctx,
-  AbstractStorageDecl *storage, const DeclAttributes &attrs) {
-
-  // Determines if the storage is immutable, either by declaring itself as a
-  // `let` or by omitting a setter.
-  auto isImmutable = [&]() {
-    if (auto varDecl = dyn_cast<VarDecl>(storage))
-      return varDecl->isLet();
-    if (accessors.Set == nullptr) return true;
-    return false;
-  };
+  VarDecl *var, const DeclAttributes &attrs) {
 
-  // Default to stored writes.
-  WriteImplKind writeImpl = WriteImplKind::Stored;
-  ReadWriteImplKind readWriteImpl = ReadWriteImplKind::Stored;
+  WriteImplKind writeImpl;
+  ReadWriteImplKind readWriteImpl;
 
   if (accessors.Get && accessors.Set) {
     // If we see `@_hasStorage var x: T { get set }`, then our property has
@@ -5035,9 +5010,13 @@ static StorageImplInfo classifyWithHasStorageAttr(
       WriteImplKind::InheritedWithObservers :
       WriteImplKind::StoredWithObservers;
     readWriteImpl = ReadWriteImplKind::MaterializeToTemporary;
-  } else if (isImmutable()) {
+  } else if (var->isLet()) {
     writeImpl = WriteImplKind::Immutable;
     readWriteImpl = ReadWriteImplKind::Immutable;
+  } else {
+    // Default to stored writes.
+    writeImpl = WriteImplKind::Stored;
+    readWriteImpl = ReadWriteImplKind::Stored;
   }
 
   // Always force Stored reads if @_hasStorage is present.
@@ -5046,14 +5025,7 @@ static StorageImplInfo classifyWithHasStorageAttr(
 
 StorageImplInfo
 Parser::ParsedAccessors::classify(Parser &P, AbstractStorageDecl *storage,
-                                  bool invalid, ParseDeclOptions flags,
-                                  SourceLoc staticLoc,
-                                  const DeclAttributes &attrs,
-                                  TypeLoc elementTy, ParameterList *indices) {
-  GenericParamList *genericParams = nullptr;
-  if (auto *subscript = dyn_cast<SubscriptDecl>(storage))
-    genericParams = subscript->getGenericParams();
-
+                                  bool invalid, const DeclAttributes &attrs) {
   // If there was a problem parsing accessors, mark all parsed accessors
   // as invalid to avoid tripping up later invariants.
   // We also want to avoid diagnose missing accessors if something
@@ -5077,35 +5049,19 @@ Parser::ParsedAccessors::classify(Parser &P, AbstractStorageDecl *storage,
       diagnoseAndIgnoreObservers(P, *this,
                    diag::observing_accessor_conflicts_with_accessor,
                    getAccessorNameForDiagnostic(nonObserver, /*article*/ true));
-
-    // Otherwise, we have either a stored or inherited observing property.
-    } else {
-      if (attrs.hasAttribute<OverrideAttr>()) {
-        return StorageImplInfo(ReadImplKind::Inherited,
-                               WriteImplKind::InheritedWithObservers,
-                               ReadWriteImplKind::MaterializeToTemporary);
-      } else {
-        return StorageImplInfo(ReadImplKind::Stored,
-                               WriteImplKind::StoredWithObservers,
-                               ReadWriteImplKind::MaterializeToTemporary);
-      }
     }
   }
 
   // Okay, observers are out of the way.
-  assert(!WillSet && !DidSet);
 
   // 'get', 'read', and a non-mutable addressor are all exclusive.
-  ReadImplKind readImpl;
   if (Get) {
     diagnoseConflictingAccessors(P, Get, Read);
     diagnoseConflictingAccessors(P, Get, Address);
-    readImpl = ReadImplKind::Get;
   } else if (Read) {
     diagnoseConflictingAccessors(P, Read, Address);
-    readImpl = ReadImplKind::Read;
   } else if (Address) {
-    readImpl = ReadImplKind::Address;
+    // Nothing can go wrong.
 
   // If there's a writing accessor of any sort, there must also be a
   // reading accessor.
@@ -5121,76 +5077,116 @@ Parser::ParsedAccessors::classify(Parser &P, AbstractStorageDecl *storage,
                  getAccessorNameForDiagnostic(mutator, /*article*/ true));
     }
 
-    readImpl = ReadImplKind::Get;
-
   // Subscripts always have to have some sort of accessor; they can't be
   // purely stored.
   } else if (isa<SubscriptDecl>(storage)) {
     if (!invalid) {
       P.diagnose(LBLoc, diag::subscript_without_get);
     }
+  }
+
+  // A mutable addressor is exclusive with 'set' and 'modify', but
+  // 'set' and 'modify' can appear together.
+  if (Set) {
+    diagnoseConflictingAccessors(P, Set, MutableAddress);
+  } else if (Modify) {
+    diagnoseConflictingAccessors(P, Modify, MutableAddress);
+  }
+
+  if (auto *var = dyn_cast<VarDecl>(storage)) {
+    // Allow the @_hasStorage attribute to override all the accessors we parsed
+    // when making the final classification.
+    if (attrs.hasAttribute<HasStorageAttr>()) {
+      // The SIL rules for @_hasStorage are slightly different from the non-SIL
+      // rules. In SIL mode, @_hasStorage marks that the type is simply stored,
+      // and the only thing that determines mutability is the existence of the
+      // setter.
+      //
+      // FIXME: SIL should not be special cased here. The behavior should be
+      //        consistent between SIL and non-SIL.
+      //        The strategy here should be to keep track of all opaque accessors
+      //        along with enough information to access the storage trivially
+      //        if allowed. This could be a representational change to
+      //        StorageImplInfo such that it keeps a bitset of listed accessors
+      //        and dynamically determines the access strategy from that.
+      if (P.isInSILMode())
+        return StorageImplInfo::getSimpleStored(
+          StorageIsMutable_t(Set != nullptr));
+
+      return classifyWithHasStorageAttr(*this, P.Context, var, attrs);
+    }
+  }
 
+  // 'get', 'read', and a non-mutable addressor are all exclusive.
+  ReadImplKind readImpl;
+  if (Get) {
+    readImpl = ReadImplKind::Get;
+  } else if (Read) {
+    readImpl = ReadImplKind::Read;
+  } else if (Address) {
+    readImpl = ReadImplKind::Address;
+
+  // If there's a writing accessor of any sort, there must also be a
+  // reading accessor.
+  } else if (auto mutator = findFirstMutator()) {
     readImpl = ReadImplKind::Get;
 
+  // Subscripts always have to have some sort of accessor; they can't be
+  // purely stored.
+  } else if (isa<SubscriptDecl>(storage)) {
+    readImpl = ReadImplKind::Get;
+
+  // Check if we have observers.
+  } else if (WillSet || DidSet) {
+    if (attrs.hasAttribute<OverrideAttr>()) {
+      readImpl = ReadImplKind::Inherited;
+    } else {
+      readImpl = ReadImplKind::Stored;
+    }
+
   // Otherwise, it's stored.
   } else {
     readImpl = ReadImplKind::Stored;
   }
 
-  // A mutable addressor is exclusive with 'set' and 'modify', but
-  // 'set' and 'modify' can appear together.
   // Prefer using 'set' and 'modify' over a mutable addressor.
   WriteImplKind writeImpl;
   ReadWriteImplKind readWriteImpl;
   if (Set) {
-    diagnoseConflictingAccessors(P, Set, MutableAddress);
     writeImpl = WriteImplKind::Set;
     if (Modify) {
       readWriteImpl = ReadWriteImplKind::Modify;
     } else {
       readWriteImpl = ReadWriteImplKind::MaterializeToTemporary;
     }
   } else if (Modify) {
-    diagnoseConflictingAccessors(P, Modify, MutableAddress);
     writeImpl = WriteImplKind::Modify;
     readWriteImpl = ReadWriteImplKind::Modify;
   } else if (MutableAddress) {
     writeImpl = WriteImplKind::MutableAddress;
     readWriteImpl = ReadWriteImplKind::MutableAddress;
 
-  // Otherwise, it's stored if there was no specific reading accessor.
+  // Check if we have observers.
+  } else if (readImpl == ReadImplKind::Inherited) {
+    writeImpl = WriteImplKind::InheritedWithObservers;
+    readWriteImpl = ReadWriteImplKind::MaterializeToTemporary;
+
+  // Otherwise, it's stored.
   } else if (readImpl == ReadImplKind::Stored) {
-    writeImpl = WriteImplKind::Stored;
-    readWriteImpl = ReadWriteImplKind::Stored;
+    if (WillSet || DidSet) {
+      writeImpl = WriteImplKind::StoredWithObservers;
+      readWriteImpl = ReadWriteImplKind::MaterializeToTemporary;
+    } else {
+      writeImpl = WriteImplKind::Stored;
+      readWriteImpl = ReadWriteImplKind::Stored;
+    }
 
   // Otherwise, it's immutable.
   } else {
     writeImpl = WriteImplKind::Immutable;
     readWriteImpl = ReadWriteImplKind::Immutable;
   }
 
-  // Allow the @_hasStorage attribute to override all the accessors we parsed
-  // when making the final classification.
-  if (attrs.hasAttribute<HasStorageAttr>()) {
-    // The SIL rules for @_hasStorage are slightly different from the non-SIL
-    // rules. In SIL mode, @_hasStorage marks that the type is simply stored,
-    // and the only thing that determines mutability is the existence of the
-    // setter.
-    //
-    // FIXME: SIL should not be special cased here. The behavior should be
-    //        consistent between SIL and non-SIL.
-    //        The strategy here should be to keep track of all opaque accessors
-    //        along with enough information to access the storage trivially
-    //        if allowed. This could be a representational change to
-    //        StorageImplInfo such that it keeps a bitset of listed accessors
-    //        and dynamically determines the access strategy from that.
-    if (P.isInSILMode())
-      return StorageImplInfo::getSimpleStored(
-        StorageIsMutable_t(Set != nullptr));
-
-    return classifyWithHasStorageAttr(*this, P.Context, storage, attrs);
-  }
-
   return StorageImplInfo(readImpl, writeImpl, readWriteImpl);
 }
 
@@ -6603,8 +6599,7 @@ Parser::parseDeclSubscript(SourceLoc StaticLoc,
   }
 
   accessors.record(*this, Subscript, (Invalid || !Status.isSuccess()),
-                   Flags, StaticLoc, Attributes,
-                   ElementTy.get(), Indices.get(), Decls);
+                   Attributes, Decls);
 
   // No need to setLocalDiscriminator because subscripts cannot
   // validly appear outside of type decls.