Merge pull request #25692 from davidungar/A-6-14

[Name Lookup, ASTScope] simplified and debugged

Author: David Ungar

include/swift/AST/ASTScope.h

@@ -592,17 +592,18 @@ class AbstractASTScopeDeclConsumer {
   /// Takes an array in order to batch the consumption before setting
   /// IndexOfFirstOuterResult when necessary.
   virtual bool consume(ArrayRef<ValueDecl *> values, DeclVisibilityKind vis,
-                       Optional<bool> isCascadingUse,
                        NullablePtr<DeclContext> baseDC = nullptr) = 0;
 
   /// Eventually this functionality should move into ASTScopeLookup
-  virtual std::pair<bool, Optional<bool>>
-  lookupInSelfType(NullablePtr<DeclContext> selfDC, DeclContext *const scopeDC,
-                   NominalTypeDecl *const nominal,
-                   Optional<bool> isCascadingUse) = 0;
+  virtual bool
+  lookInMembers(NullablePtr<DeclContext> selfDC, DeclContext *const scopeDC,
+                NominalTypeDecl *const nominal,
+                function_ref<bool(Optional<bool>)> calculateIsCascadingUse) = 0;
 
 #ifndef NDEBUG
-  virtual void stopForDebuggingIfTargetLookup() = 0;
+  virtual void startingNextLookupStep() = 0;
+  virtual void finishingLookup(std::string) const = 0;
+  virtual bool isTargetLookup() const = 0;
 #endif
 };
 
@@ -615,19 +616,19 @@ class ASTScopeDeclGatherer : public AbstractASTScopeDeclConsumer {
   virtual ~ASTScopeDeclGatherer() = default;
 
   bool consume(ArrayRef<ValueDecl *> values, DeclVisibilityKind vis,
-               Optional<bool> isCascadingUse,
                NullablePtr<DeclContext> baseDC = nullptr) override;
 
   /// Eventually this functionality should move into ASTScopeLookup
-  std::pair<bool, Optional<bool>>
-  lookupInSelfType(NullablePtr<DeclContext>, DeclContext *const,
-                   NominalTypeDecl *const,
-                   Optional<bool> isCascadingUse) override {
-    return std::make_pair(false, isCascadingUse);
+  bool lookInMembers(NullablePtr<DeclContext>, DeclContext *const,
+                     NominalTypeDecl *const,
+                     function_ref<bool(Optional<bool>)>) override {
+    return false;
   }
 
 #ifndef NDEBUG
-  void stopForDebuggingIfTargetLookup() override {}
+  void startingNextLookupStep() override {}
+  void finishingLookup(std::string) const override {}
+  bool isTargetLookup() const override { return false; }
 #endif
 
   ArrayRef<ValueDecl *> getDecls() { return values; }
@@ -641,12 +642,17 @@ class ASTScope {
 
 public:
   ASTScope(SourceFile *);
-  static Optional<bool>
+
+  /// \return the scopes traversed
+  static llvm::SmallVector<const ast_scope::ASTScopeImpl *, 0>
   unqualifiedLookup(SourceFile *, DeclName, SourceLoc,
                     const DeclContext *startingContext,
-                    Optional<bool> isCascadingUse,
                     namelookup::AbstractASTScopeDeclConsumer &);
 
+  static Optional<bool>
+  computeIsCascadingUse(ArrayRef<const ast_scope::ASTScopeImpl *> history,
+                        Optional<bool> initialIsCascadingUse);
+
   LLVM_ATTRIBUTE_DEPRECATED(void dump() const LLVM_ATTRIBUTE_USED,
                             "only for use within the debugger");
   void print(llvm::raw_ostream &) const;

include/swift/AST/NameLookup.h

@@ -247,8 +247,11 @@ namespace swift {
     bool EnableDeserializationRecovery = true;
 
     /// Should we use \c ASTScope-based resolution for unqualified name lookup?
+    /// Default is in \c ParseLangArgs
+    ///
+    /// This is a staging flag; eventually it will be removed.
     bool EnableASTScopeLookup = false;
-    
+
     /// Someday, ASTScopeLookup will supplant lookup in the parser
     bool DisableParserLookup = false;
 

include/swift/Basic/LangOptions.h

@@ -869,7 +869,12 @@ def enable_astscope_lookup : Flag<["-"], "enable-astscope-lookup">,
   Flags<[FrontendOption]>,
   HelpText<"Enable ASTScope-based unqualified name lookup">;
 
-def disable_parser_lookup : Flag<["-"], "disable_parser_lookup">,
+def disable_astscope_lookup : Flag<["-"], "disable-astscope-lookup">,
+  Flags<[FrontendOption]>,
+  HelpText<"Disable ASTScope-based unqualified name lookup">;
+
+def disable_parser_lookup : Flag<["-"], "disable-parser-lookup">,
+  Flags<[FrontendOption]>,
   HelpText<"Disable parser lookup & use ast scope lookup only (experimental)">;
 
 def index_file : Flag<["-"], "index-file">,

include/swift/Option/Options.td

@@ -37,13 +37,18 @@ using namespace ast_scope;
 
 #pragma mark ASTScope
 
-
-Optional<bool> ASTScope::unqualifiedLookup(
+llvm::SmallVector<const ASTScopeImpl *, 0> ASTScope::unqualifiedLookup(
     SourceFile *SF, DeclName name, SourceLoc loc,
-    const DeclContext *startingContext, Optional<bool> isCascadingUse,
+    const DeclContext *startingContext,
     namelookup::AbstractASTScopeDeclConsumer &consumer) {
   return ASTScopeImpl::unqualifiedLookup(SF, name, loc, startingContext,
-                                         isCascadingUse, consumer);
+                                         consumer);
+}
+
+Optional<bool> ASTScope::computeIsCascadingUse(
+    ArrayRef<const ast_scope::ASTScopeImpl *> history,
+    Optional<bool> initialIsCascadingUse) {
+  return ASTScopeImpl::computeIsCascadingUse(history, initialIsCascadingUse);
 }
 
 void ASTScope::dump() const { impl->dump(); }
@@ -108,6 +113,20 @@ Stmt *LabeledConditionalStmtScope::getStmt() const {
   return getLabeledConditionalStmt();
 }
 
+bool AbstractFunctionBodyScope::isAMethod(
+    const AbstractFunctionDecl *const afd) {
+  // What makes this interesting is that a method named "init" which is not
+  // in a nominal type or extension decl body still gets an implicit self
+  // parameter (even though the program is illegal).
+  // So when choosing between creating a MethodBodyScope and a
+  // PureFunctionBodyScope do we go by the enclosing Decl (i.e.
+  // "afd->getDeclContext()->isTypeContext()") or by
+  // "bool(afd->getImplicitSelfDecl())"?
+  //
+  // Since the code uses \c getImplicitSelfDecl, use that.
+  return afd->getImplicitSelfDecl();
+}
+
 #pragma mark getLabeledConditionalStmt
 LabeledConditionalStmt *IfStmtScope::getLabeledConditionalStmt() const {
   return stmt;
@@ -132,6 +151,14 @@ ASTContext &ASTScopeImpl::getASTContext() const {
 
 #pragma mark getDeclContext
 
+NullablePtr<DeclContext> ASTScopeImpl::getDeclContext() const {
+  return nullptr;
+}
+
+NullablePtr<DeclContext> ASTSourceFileScope::getDeclContext() const {
+  return NullablePtr<DeclContext>(SF);
+}
+
 NullablePtr<DeclContext> GenericTypeOrExtensionScope::getDeclContext() const {
   return getGenericContext();
 }
@@ -168,6 +195,10 @@ NullablePtr<DeclContext> AttachedPropertyWrapperScope::getDeclContext() const {
       .getInitContext();
 }
 
+NullablePtr<DeclContext> AbstractFunctionDeclScope::getDeclContext() const {
+  return decl;
+}
+
 NullablePtr<DeclContext> AbstractFunctionParamsScope::getDeclContext() const {
   return matchingContext;
 }

lib/AST/ASTScope.cpp

@@ -95,43 +95,36 @@ class ScopeCreator final {
     // Implicit nodes don't have source information for name lookup.
     if (d->isImplicit())
       return false;
-    // Have also seen the following in an AST:
-    // Source::
-    //
-    //  func testInvalidKeyPathComponents() {
-    //    let _ = \.{return 0} // expected-error* {{}}
-    //  }
-    //  class X {
-    //    class var a: Int { return 1 }
-    //  }
-    //
-    // AST:
-    // clang-format off
-    //    (source_file "test.swift"
-    //     (func_decl range=[test.swift:1:3 - line:3:3] "testInvalidKeyPathComponents()" interface type='() -> ()' access=internal
-    //      (parameter_list range=[test.swift:1:36 - line:1:37])
-    //      (brace_stmt range=[test.swift:1:39 - line:3:3]
-    //       (pattern_binding_decl range=[test.swift:2:5 - line:2:11]
-    //        (pattern_any)
-    //        (error_expr implicit type='<null>'))
-    //
-    //       (pattern_binding_decl range=[test.swift:2:5 - line:2:5] <=== SOURCE RANGE WILL CONFUSE SCOPE CODE
-    //        (pattern_typed implicit type='<<error type>>'
-    //         (pattern_named '_')))
-    //      ...
-    // clang-format on
-    //
-    // So test the SourceRange
-    //
-    // But wait!
-    // var z = $x0 + $x1
-    // z
-    //
-    // z has start == end, but must be pushed to expand source range
-    //
-    // So, only check source ranges for PatternBindingDecls
-    if (isa<PatternBindingDecl>(d) && (d->getStartLoc() == d->getEndLoc()))
-      return false;
+    /// In \c Parser::parseDeclVarGetSet fake PBDs are created. Ignore them.
+    /// Example:
+    /// \code
+    /// class SR10903 { static var _: Int { 0 } }
+    /// \endcode
+    if (const auto *PBD = dyn_cast<PatternBindingDecl>(d))
+      if (PBD->isInvalid())
+        return false;
+    /// In
+    /// \code
+    /// @propertyWrapper
+    /// public struct Wrapper<T> {
+    ///   public var value: T
+    ///
+    ///   public init(body: () -> T) {
+    ///     self.value = body()
+    ///   }
+    /// }
+    ///
+    /// let globalInt = 17
+    ///
+    /// @Wrapper(body: { globalInt })
+    /// public var y: Int
+    /// \endcode
+    /// I'm seeing a dumped AST include:
+    /// (pattern_binding_decl range=[test.swift:13:8 - line:12:29]
+    const auto &SM = d->getASTContext().SourceMgr;
+    assert((d->getStartLoc().isInvalid() ||
+            !SM.isBeforeInBuffer(d->getEndLoc(), d->getStartLoc())) &&
+           "end-before-start will break tree search via location");
     return true;
   }
 
@@ -179,9 +172,10 @@ class ScopeCreator final {
       function_ref<void(NullablePtr<CaptureListExpr>, ClosureExpr *)>
           foundClosure);
 
+  // A safe way to discover this, without creating a circular request.
+  // Cannot call getAttachedPropertyWrappers.
   static bool hasCustomAttribute(VarDecl *vd) {
-    return AttachedPropertyWrapperScope::getCustomAttributesSourceRange(vd)
-        .isValid();
+    return vd->getAttrs().getAttribute<CustomAttr>();
   }
 
 public:
@@ -190,8 +184,9 @@ class ScopeCreator final {
 
   void createAttachedPropertyWrapperScope(PatternBindingDecl *patternBinding,
                                           ASTScopeImpl *parent) {
+ 
     patternBinding->getPattern(0)->forEachVariable([&](VarDecl *vd) {
-      // assume all same as first
+        // assume all same as first
       if (hasCustomAttribute(vd))
         createSubtree<AttachedPropertyWrapperScope>(parent, vd);
     });
@@ -221,7 +216,7 @@ class ScopeCreator final {
   void
   forEachSpecializeAttrInSourceOrder(Decl *declBeingSpecialized,
                                      function_ref<void(SpecializeAttr *)> fn) {
-    llvm::SmallVector<SpecializeAttr *, 8> sortedSpecializeAttrs;
+    SmallVector<SpecializeAttr *, 8> sortedSpecializeAttrs;
     for (auto *attr : declBeingSpecialized->getAttrs()) {
       if (auto *specializeAttr = dyn_cast<SpecializeAttr>(attr)) {
         if (!isDuplicate(specializeAttr))
@@ -584,6 +579,8 @@ CREATES_NEW_INSERTION_POINT(TopLevelCodeScope)
 
 NO_NEW_INSERTION_POINT(AbstractFunctionBodyScope)
 NO_NEW_INSERTION_POINT(AbstractFunctionDeclScope)
+NO_NEW_INSERTION_POINT(AttachedPropertyWrapperScope)
+
 NO_NEW_INSERTION_POINT(CaptureListScope)
 NO_NEW_INSERTION_POINT(CaseStmtScope)
 NO_NEW_INSERTION_POINT(CatchStmtScope)
@@ -606,7 +603,6 @@ NO_EXPANSION(ClosureParametersScope)
 NO_EXPANSION(SpecializeAttributeScope)
 // no accessors, unlike PatternEntryUseScope
 NO_EXPANSION(ConditionalClausePatternUseScope)
-NO_EXPANSION(AttachedPropertyWrapperScope)
 NO_EXPANSION(GuardStmtUseScope)
 
 #undef CREATES_NEW_INSERTION_POINT
@@ -744,8 +740,10 @@ void AbstractFunctionDeclScope::expandAScopeThatDoesNotCreateANewInsertionPoint(
     }
   }
   // Create scope for the body.
-  if (decl->getBody()) {
-    if (decl->getDeclContext()->isTypeContext())
+  // We create body scopes when there is no body for source kit to complete
+  // erroneous code in bodies.
+  if (decl->getBodySourceRange().isValid()) {
+    if (AbstractFunctionBodyScope::isAMethod(decl))
       scopeCreator.createSubtree<MethodBodyScope>(leaf, decl);
     else
       scopeCreator.createSubtree<PureFunctionBodyScope>(leaf, decl);
@@ -754,8 +752,10 @@ void AbstractFunctionDeclScope::expandAScopeThatDoesNotCreateANewInsertionPoint(
 
 void AbstractFunctionBodyScope::expandAScopeThatDoesNotCreateANewInsertionPoint(
     ScopeCreator &scopeCreator) {
-  BraceStmt *braceStmt = decl->getBody();
-  ASTVisitorForScopeCreation().visitBraceStmt(braceStmt, this, scopeCreator);
+  // We create body scopes when there is no body for source kit to complete
+  // erroneous code in bodies.
+  if (BraceStmt *braceStmt = decl->getBody())
+    ASTVisitorForScopeCreation().visitBraceStmt(braceStmt, this, scopeCreator);
 }
 
 void IfStmtScope::expandAScopeThatDoesNotCreateANewInsertionPoint(
@@ -898,6 +898,15 @@ void DefaultArgumentInitializerScope::
   ASTVisitorForScopeCreation().visitExpr(initExpr, this, scopeCreator);
 }
 
+void AttachedPropertyWrapperScope::
+    expandAScopeThatDoesNotCreateANewInsertionPoint(
+        ScopeCreator &scopeCreator) {
+  for (auto *attr : decl->getAttrs().getAttributes<CustomAttr>()) {
+    if (auto *expr = attr->getArg())
+      ASTVisitorForScopeCreation().visitExpr(expr, this, scopeCreator);
+  }
+}
+
 #pragma mark expandScope
 
 ASTScopeImpl *GenericTypeOrExtensionWholePortion::expandScope(
@@ -911,8 +920,7 @@ ASTScopeImpl *GenericTypeOrExtensionWholePortion::expandScope(
       scope->getDecl().get(), scope->getGenericContext()->getGenericParams(),
       scope);
   if (scope->getGenericContext()->getTrailingWhereClause())
-    deepestScope =
-        scope->createTrailingWhereClauseScope(deepestScope, scopeCreator);
+    scope->createTrailingWhereClauseScope(deepestScope, scopeCreator);
   scope->createBodyScope(deepestScope, scopeCreator);
   return scope->getParent().get();
 }