Lookup using private discriminators should still find public things. (#4238)

Private and fileprivate declarations have a special "private
discriminator" to keep them from colliding with declarations with the
same signature in another file. The debugger uses this to prefer
declarations in the file you're currently stopped in when running the
expression parser. Unfortunately, the current logic didn't just prefer
declarations in the current file---it /only/ took declarations in the
current file, as long as there weren't zero. The fixed version will
include any declarations in the current file plus any declarations
with 'internal' or broader access.

(Really we shouldn't do this at the lookup level at all; it should
just be a tie-breaker in solution ranking in the constraint
solver. But that would be a more intrusive change.)

rdar://problem/27015195

Author: jrose-apple

lib/AST/NameLookup.cpp

@@ -327,23 +327,34 @@ bool swift::removeShadowedDecls(SmallVectorImpl<ValueDecl*> &decls,
   return anyRemoved;
 }
 
-static bool matchesDiscriminator(Identifier discriminator,
-                                 const ValueDecl *value) {
+namespace {
+enum class DiscriminatorMatch {
+  NoDiscriminator,
+  Matches,
+  Different
+};
+}
+
+static DiscriminatorMatch matchDiscriminator(Identifier discriminator,
+                                             const ValueDecl *value) {
   if (value->getFormalAccess() > Accessibility::FilePrivate)
-    return false;
+    return DiscriminatorMatch::NoDiscriminator;
 
   auto containingFile =
     dyn_cast<FileUnit>(value->getDeclContext()->getModuleScopeContext());
   if (!containingFile)
-    return false;
+    return DiscriminatorMatch::Different;
 
-  return
-    discriminator == containingFile->getDiscriminatorForPrivateValue(value);
+  if (discriminator == containingFile->getDiscriminatorForPrivateValue(value))
+    return DiscriminatorMatch::Matches;
+
+  return DiscriminatorMatch::Different;
 }
 
-static bool matchesDiscriminator(Identifier discriminator,
-                                 UnqualifiedLookupResult lookupResult) {
-  return matchesDiscriminator(discriminator, lookupResult.getValueDecl());
+static DiscriminatorMatch
+matchDiscriminator(Identifier discriminator,
+                   UnqualifiedLookupResult lookupResult) {
+  return matchDiscriminator(discriminator, lookupResult.getValueDecl());
 }
 
 template <typename Result>
@@ -353,19 +364,21 @@ static void filterForDiscriminator(SmallVectorImpl<Result> &results,
   if (discriminator.empty())
     return;
 
-  auto doesNotMatch = [discriminator](Result next) -> bool {
-    return !matchesDiscriminator(discriminator, next);
-  };
-
-  auto lastMatchIter = std::find_if_not(results.rbegin(), results.rend(),
-                                        doesNotMatch);
+  auto lastMatchIter = std::find_if(results.rbegin(), results.rend(),
+                                    [discriminator](Result next) -> bool {
+    return
+      matchDiscriminator(discriminator, next) == DiscriminatorMatch::Matches;
+  });
   if (lastMatchIter == results.rend())
     return;
 
   Result lastMatch = *lastMatchIter;
 
   auto newEnd = std::remove_if(results.begin(), lastMatchIter.base()-1,
-                               doesNotMatch);
+                               [discriminator](Result next) -> bool {
+    return
+      matchDiscriminator(discriminator, next) == DiscriminatorMatch::Different;
+  });
   results.erase(newEnd, results.end());
   results.push_back(lastMatch);
 }

test/NameBinding/Inputs/HasPrivateAccess1.swift

@@ -3,3 +3,10 @@ private var global: Int = 1
 public struct MyStruct {
   private static func method() -> Int? { return nil }
 }
+
+// Note: These are deliberately 'internal' here and 'private' in the other file.
+// They're testing that we don't filter out non-discriminated decls in lookup.
+internal func process(_ x: Int) -> Int { return x }
+extension MyStruct {
+  internal static func process(_ x: Int) -> Int { return x }
+}

test/NameBinding/Inputs/HasPrivateAccess2.swift

@@ -3,3 +3,10 @@ private var global: String = "abc"
 extension MyStruct {
   private static func method() -> String? { return nil }
 }
+
+// Note: These are deliberately 'private' here and 'internal' in the other file.
+// They're testing that we don't filter out non-discriminated decls in lookup.
+private func process(_ x: String) -> String { return x }
+extension MyStruct {
+  private static func process(_ x: String) -> String { return x }
+}

test/NameBinding/debug-client-discriminator.swift

@@ -21,3 +21,23 @@ let qualified = HasPrivateAccess.global
 // CHECK-INT: let result: Int?
 // CHECK-STRING: let result: String?
 let result = HasPrivateAccess.MyStruct.method()
+
+// CHECK-ERROR: let processedInt: Int
+// CHECK-INT: let processedInt: Int
+// CHECK-STRING: let processedInt: Int
+let processedInt = process(1)
+
+// CHECK-ERROR: let processedIntQualified: Int
+// CHECK-INT: let processedIntQualified: Int
+// CHECK-STRING: let processedIntQualified: Int
+let processedIntQualified = MyStruct.process(1)
+
+// CHECK-ERROR: let processedString: String
+// CHECK-INT: let processedString: String
+// CHECK-STRING: let processedString: String
+let processedString = process("abc")
+
+// CHECK-ERROR: let processedStringQualified: String
+// CHECK-INT: let processedStringQualified: String
+// CHECK-STRING: let processedStringQualified: String
+let processedStringQualified = MyStruct.process("abc")