Merge pull request swiftlang#34455 from bnbarham/benb/decl-filter-70704835

Retain invalid decls when filtering using access path and skip
including decls in the completion list if the completion is in
their initializer.

Author: bnbarham

include/swift/AST/NameLookup.h

@@ -414,6 +414,22 @@ class AccessFilteringDeclConsumer final : public VisibleDeclConsumer {
                  DynamicLookupInfo dynamicLookupInfo = {}) override;
 };
 
+/// Filters out decls that are not usable based on their source location, eg.
+/// a decl inside its own initializer or a non-type decl before its definition.
+class UsableFilteringDeclConsumer final : public VisibleDeclConsumer {
+  const SourceManager &SM;
+  SourceLoc Loc;
+  VisibleDeclConsumer &ChainedConsumer;
+
+public:
+  UsableFilteringDeclConsumer(const SourceManager &SM, SourceLoc loc,
+                              VisibleDeclConsumer &consumer)
+      : SM(SM), Loc(loc), ChainedConsumer(consumer) {}
+
+  void foundDecl(ValueDecl *D, DeclVisibilityKind reason,
+                 DynamicLookupInfo dynamicLookupInfo) override;
+};
+
 /// Remove any declarations in the given set that were overridden by
 /// other declarations in that set.
 ///

lib/AST/NameLookup.cpp

@@ -138,14 +138,40 @@ void DebuggerClient::anchor() {}
 void AccessFilteringDeclConsumer::foundDecl(
     ValueDecl *D, DeclVisibilityKind reason,
     DynamicLookupInfo dynamicLookupInfo) {
-  if (D->hasInterfaceType() && D->isInvalid())
-    return;
   if (!D->isAccessibleFrom(DC))
     return;
 
   ChainedConsumer.foundDecl(D, reason, dynamicLookupInfo);
 }
 
+void UsableFilteringDeclConsumer::foundDecl(ValueDecl *D,
+    DeclVisibilityKind reason, DynamicLookupInfo dynamicLookupInfo) {
+  // Skip when Loc is within the decl's own initializer
+  if (auto *VD = dyn_cast<VarDecl>(D)) {
+    if (auto *init = VD->getParentInitializer()) {
+      auto initRange = init->getSourceRange();
+      if (initRange.isValid() && SM.rangeContainsTokenLoc(initRange, Loc))
+        return;
+    }
+  }
+
+  switch (reason) {
+  case DeclVisibilityKind::LocalVariable:
+    // Skip if Loc is before the found decl, unless its a TypeDecl (whose use
+    // before its declaration is still allowed)
+    if (!isa<TypeDecl>(D) && !SM.isBeforeInBuffer(D->getLoc(), Loc))
+      return;
+    break;
+  default:
+    // The rest of the file is currently skipped, so no need to check
+    // decl location for VisibleAtTopLevel. Other visibility kinds are always
+    // usable
+    break;
+  }
+
+  ChainedConsumer.foundDecl(D, reason, dynamicLookupInfo);
+}
+
 void LookupResultEntry::print(llvm::raw_ostream& out) const {
   getValueDecl()->print(out);
   if (auto dc = getBaseDecl()) {

lib/IDE/CodeCompletion.cpp

@@ -2468,9 +2468,7 @@ class CompletionLookup final : public swift::VisibleDeclConsumer {
 
   void addVarDeclRef(const VarDecl *VD, DeclVisibilityKind Reason,
                      DynamicLookupInfo dynamicLookupInfo) {
-    if (!VD->hasName() ||
-        !VD->isAccessibleFrom(CurrDeclContext) ||
-        VD->shouldHideFromEditor())
+    if (!VD->hasName())
       return;
 
     const Identifier Name = VD->getName();
@@ -4288,8 +4286,12 @@ class CompletionLookup final : public swift::VisibleDeclConsumer {
     ExprType = Type();
     Kind = LookupKind::ValueInDeclContext;
     NeedLeadingDot = false;
-    FilteredDeclConsumer Consumer(*this, Filter);
-    lookupVisibleDecls(Consumer, CurrDeclContext,
+
+    AccessFilteringDeclConsumer AccessFilteringConsumer(
+        CurrDeclContext, *this);
+    FilteredDeclConsumer FilteringConsumer(AccessFilteringConsumer, Filter);
+
+    lookupVisibleDecls(FilteringConsumer, CurrDeclContext,
                        /*IncludeTopLevel=*/false, Loc);
     RequestedCachedResults.push_back(RequestedResultsTy::toplevelResults()
                                          .withModuleQualifier(ModuleQualifier));
@@ -4633,7 +4635,10 @@ class CompletionLookup final : public swift::VisibleDeclConsumer {
   void getTypeCompletionsInDeclContext(SourceLoc Loc,
                                        bool ModuleQualifier = true) {
     Kind = LookupKind::TypeInDeclContext;
-    lookupVisibleDecls(*this, CurrDeclContext,
+
+    AccessFilteringDeclConsumer AccessFilteringConsumer(
+        CurrDeclContext, *this);
+    lookupVisibleDecls(AccessFilteringConsumer, CurrDeclContext,
                        /*IncludeTopLevel=*/false, Loc);
 
     RequestedCachedResults.push_back(
@@ -4646,21 +4651,30 @@ class CompletionLookup final : public swift::VisibleDeclConsumer {
     Kind = OnlyTypes ? LookupKind::TypeInDeclContext
                      : LookupKind::ValueInDeclContext;
     NeedLeadingDot = false;
-    AccessFilteringDeclConsumer FilteringConsumer(CurrDeclContext, *this);
-    CurrModule->lookupVisibleDecls({}, FilteringConsumer,
+
+    UsableFilteringDeclConsumer UsableFilteringConsumer(Ctx.SourceMgr,
+        Ctx.SourceMgr.getCodeCompletionLoc(), *this);
+    AccessFilteringDeclConsumer AccessFilteringConsumer(
+        CurrDeclContext, UsableFilteringConsumer);
+
+    CurrModule->lookupVisibleDecls({}, AccessFilteringConsumer,
                                    NLKind::UnqualifiedLookup);
   }
 
-  void getVisibleDeclsOfModule(const ModuleDecl *TheModule,
-                               ArrayRef<std::string> AccessPath,
-                               bool ResultsHaveLeadingDot) {
+  void lookupExternalModuleDecls(const ModuleDecl *TheModule,
+                                 ArrayRef<std::string> AccessPath,
+                                 bool ResultsHaveLeadingDot) {
+    assert(CurrModule != TheModule &&
+           "requested module should be external");
+
     Kind = LookupKind::ImportFromModule;
     NeedLeadingDot = ResultsHaveLeadingDot;
 
     ImportPath::Access::Builder builder;
     for (auto Piece : AccessPath) {
       builder.push_back(Ctx.getIdentifier(Piece));
     }
+
     AccessFilteringDeclConsumer FilteringConsumer(CurrDeclContext, *this);
     TheModule->lookupVisibleDecls(builder.get(), FilteringConsumer,
                                   NLKind::UnqualifiedLookup);
@@ -6758,7 +6772,7 @@ void swift::ide::lookupCodeCompletionResultsFromModule(
     ArrayRef<std::string> accessPath, bool needLeadingDot,
     const DeclContext *currDeclContext) {
   CompletionLookup Lookup(targetSink, module->getASTContext(), currDeclContext);
-  Lookup.getVisibleDeclsOfModule(module, accessPath, needLeadingDot);
+  Lookup.lookupExternalModuleDecls(module, accessPath, needLeadingDot);
 }
 
 void swift::ide::copyCodeCompletionResults(CodeCompletionResultSink &targetSink,

lib/Sema/LookupVisibleDecls.cpp

@@ -1302,55 +1302,9 @@ void swift::lookupVisibleDecls(VisibleDeclConsumer &Consumer,
     lookupVisibleDeclsImpl(Consumer, DC, IncludeTopLevel, Loc);
     return;
   }
-
-  // Filtering out unusable values.
-  class LocalConsumer : public VisibleDeclConsumer {
-    const SourceManager &SM;
-    SourceLoc Loc;
-    VisibleDeclConsumer &Consumer;
-
-    bool isUsableValue(ValueDecl *VD, DeclVisibilityKind Reason) {
-
-      // Check "use within its own initial value" case.
-      if (auto *varD = dyn_cast<VarDecl>(VD)) {
-        if (auto *initExpr = varD->getParentInitializer())
-          if (SM.rangeContainsTokenLoc(initExpr->getSourceRange(), Loc))
-            return false;
-      }
-
-      switch (Reason) {
-      case DeclVisibilityKind::LocalVariable:
-        // Use of 'TypeDecl's before declaration is allowed.
-        if (isa<TypeDecl>(VD))
-          return true;
-
-        return SM.isBeforeInBuffer(VD->getLoc(), Loc);
-
-      case DeclVisibilityKind::VisibleAtTopLevel:
-        // TODO: Implement forward reference rule for script mode? Currently,
-        // it's not needed because the rest of the file hasn't been parsed.
-        // See: https://bugs.swift.org/browse/SR-284 for the rule.
-        return true;
-
-      default:
-        // Other visibility kind are always usable.
-        return true;
-      }
-    }
-
-  public:
-    LocalConsumer(const SourceManager &SM, SourceLoc Loc,
-                  VisibleDeclConsumer &Consumer)
-        : SM(SM), Loc(Loc), Consumer(Consumer) {}
-
-    void foundDecl(ValueDecl *VD, DeclVisibilityKind Reason,
-                   DynamicLookupInfo dynamicLookupInfo) override {
-      if (isUsableValue(VD, Reason))
-        Consumer.foundDecl(VD, Reason, dynamicLookupInfo);
-    }
-  } LocalConsumer(DC->getASTContext().SourceMgr, Loc, Consumer);
-
-  lookupVisibleDeclsImpl(LocalConsumer, DC, IncludeTopLevel, Loc);
+  UsableFilteringDeclConsumer FilteringConsumer(DC->getASTContext().SourceMgr,
+                                                Loc, Consumer);
+  lookupVisibleDeclsImpl(FilteringConsumer, DC, IncludeTopLevel, Loc);
 }
 
 void swift::lookupVisibleMemberDecls(VisibleDeclConsumer &Consumer, Type BaseTy,

test/IDE/complete_at_top_level.swift

@@ -1,3 +1,5 @@
+// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=TOP_LEVEL_VAR_INIT_3 | %FileCheck %s -check-prefix=TOP_LEVEL_VAR_INIT_3_NEGATIVE
+
 // RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=TYPE_CHECKED_EXPR_1 | %FileCheck %s -check-prefix=TYPE_CHECKED_EXPR_1
 // RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=TYPE_CHECKED_EXPR_2 | %FileCheck %s -check-prefix=TYPE_CHECKED_EXPR_2
 // RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=TYPE_CHECKED_EXPR_3 | %FileCheck %s -check-prefix=TYPE_CHECKED_EXPR_3
@@ -153,6 +155,9 @@
 // This test is not meant to test that we can correctly form all kinds of
 // completion results in general; that should be tested elsewhere.
 
+var topLevelVar3 = #^TOP_LEVEL_VAR_INIT_3^#
+// TOP_LEVEL_VAR_INIT_3_NEGATIVE-NOT: topLevelVar3
+
 struct FooStruct {
   var instanceVar = 0
 

test/IDE/complete_sequence_invalid.swift

@@ -0,0 +1,19 @@
+// RUN: %empty-directory(%t)
+// RUN: %target-swift-ide-test -batch-code-completion -source-filename %s -filecheck %raw-FileCheck -completion-output-dir %t
+
+// GLOBAL: Decl[GlobalVar]/CurrModule:         invalidDecl[#<<error type>>#];
+let invalidDecl = INVALID
+
+struct S {
+  // MEMBER: Decl[InstanceMethod]/CurrNominal:   invalidMethod()[#<<error type>>#];
+  func invalidMethod() -> INVALID
+}
+
+func test() {
+  #^GLOBAL_1?check=GLOBAL^#
+  #^GLOBAL_2?check=GLOBAL^#
+}
+
+func testMember(val: S) {
+  val.#^MEMBER_1?check=MEMBER^##^MEMBER_2?check=MEMBER^#
+}