[ConstraintSystem] Use new fix/diagnostic for name shadowing

Stop filtering outer overload choices while trying to pre-check
expression, instead have it always fetch those and use new
fix to only attempt them in diagnostic mode (unless it's min/max
situation with conditional conformances).

Author: xedin

lib/Sema/CSSimplify.cpp

@@ -3273,6 +3273,30 @@ bool ConstraintSystem::repairFailures(
                                 locator))
       break;
 
+    {
+      auto *calleeLocator = getCalleeLocator(loc);
+      if (hasFixFor(calleeLocator, FixKind::AddQualifierToAccessTopLevelName)) {
+        if (auto overload = findSelectedOverloadFor(calleeLocator)) {
+          if (auto choice = overload->choice.getDeclOrNull()) {
+            // If this is an argument of a symetric function/operator let's
+            // not fix any position rather than first because we'd just end
+            // up with ambiguity instead of reporting an actual problem with
+            // mismatched type since each argument can have district bindings.
+            if (auto *AFD = dyn_cast<AbstractFunctionDecl>(choice)) {
+              auto *paramList = AFD->getParameters();
+              auto firstParamType = paramList->get(0)->getInterfaceType();
+              if (elt.castTo<LocatorPathElt::ApplyArgToParam>().getParamIdx() >
+                      0 &&
+                  llvm::all_of(*paramList, [&](const ParamDecl *param) -> bool {
+                    return param->getInterfaceType()->isEqual(firstParamType);
+                  }))
+                return true;
+            }
+          }
+        }
+      }
+    }
+
     conversionsOrFixes.push_back(
         AllowArgumentMismatch::create(*this, lhs, rhs, loc));
     break;
@@ -6201,8 +6225,27 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyMemberConstraint(
       if (candidates.size() == 1)
         candidates.front()->setFavored();
 
-      generateConstraints(candidates, memberTy, outerAlternatives,
-                          useDC, locator);
+      // We *might* include any non-members that we found in outer contexts in
+      // some special cases, for backwards compatibility: first, we have to be
+      // looking for one of the special names ('min' or 'max'), and second, all
+      // of the inner (viable) results need to come from conditional
+      // conformances. The second condition is how the problem here was
+      // encountered: a type ('Range') was made to conditionally conform to a
+      // new protocol ('Sequence'), which introduced some extra methods
+      // ('min' and 'max') that shadowed global functions that people regularly
+      // called within extensions to that type (usually adding 'clamp').
+      bool treatAsViable =
+          (member.isSimpleName("min") || member.isSimpleName("max")) &&
+          allFromConditionalConformances(DC, baseTy, result.ViableCandidates);
+
+      generateConstraints(
+          candidates, memberTy, outerAlternatives, useDC, locator, None,
+          /*requiresFix=*/!treatAsViable,
+          [&](unsigned, const OverloadChoice &) {
+            return treatAsViable ? nullptr
+                                 : AddQualifierToAccessTopLevelName::create(
+                                       *this, locator);
+          });
     }
   }
 
@@ -8852,6 +8895,7 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
   case FixKind::AllowInvalidUseOfTrailingClosure:
   case FixKind::AllowNonClassTypeToConvertToAnyObject:
   case FixKind::SpecifyClosureReturnType:
+  case FixKind::AddQualifierToAccessTopLevelName:
     llvm_unreachable("handled elsewhere");
   }
 
@@ -9315,7 +9359,12 @@ ConstraintSystem::simplifyConstraint(const Constraint &constraint) {
 
   case ConstraintKind::BindOverload:
     if (auto *fix = constraint.getFix()) {
-      if (recordFix(fix))
+      // TODO(diagnostics): Impact should be associated with a fix unless
+      // it's a contextual problem, then only solver can decide what the impact
+      // would be in each particular situation.
+      auto impact =
+          fix->getKind() == FixKind::AddQualifierToAccessTopLevelName ? 10 : 1;
+      if (recordFix(fix, impact))
         return SolutionKind::Error;
     }
 

lib/Sema/TypeCheckConstraints.cpp

@@ -452,20 +452,6 @@ static bool findNonMembers(ArrayRef<LookupResultEntry> lookupResults,
   return AllDeclRefs;
 }
 
-/// Whether we should be looking at the outer results for a function called \c
-/// name.
-///
-/// This is very restrictive because it's a source compatibility issue (see the
-/// if (AllConditionalConformances) { (void)findNonMembers(...); } below).
-static bool shouldConsiderOuterResultsFor(DeclNameRef name) {
-  const StringRef specialNames[] = {"min", "max"};
-  for (auto specialName : specialNames)
-    if (name.isSimpleName(specialName))
-      return true;
-
-  return false;
-}
-
 /// Bind an UnresolvedDeclRefExpr by performing name lookup and
 /// returning the resultant expression. Context is the DeclContext used
 /// for the lookup.
@@ -479,8 +465,11 @@ Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE,
   NameLookupOptions lookupOptions = defaultUnqualifiedLookupOptions;
   if (isa<AbstractFunctionDecl>(DC))
     lookupOptions |= NameLookupFlags::KnownPrivate;
-  if (shouldConsiderOuterResultsFor(Name))
-    lookupOptions |= NameLookupFlags::IncludeOuterResults;
+
+  // TODO: Include all of the possible members to give a solver a
+  //       chance to diagnose name shadowing which requires explicit
+  //       name/module qualifier to access top-level name.
+  lookupOptions |= NameLookupFlags::IncludeOuterResults;
 
   auto Lookup = TypeChecker::lookupUnqualified(DC, Name, Loc, lookupOptions);
 
@@ -625,14 +614,6 @@ Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE,
   // better matching candidates.
   if (localDeclAfterUse) {
     auto innerDecl = localDeclAfterUse;
-
-    // Perform a thorough lookup if outer results was not included before.
-    if (!lookupOptions.contains(NameLookupFlags::IncludeOuterResults)) {
-      auto option = lookupOptions;
-      option |= NameLookupFlags::IncludeOuterResults;
-      Lookup = lookupUnqualified(DC, Name, Loc, option);
-    }
-
     while (localDeclAfterUse) {
       if (Lookup.outerResults().empty()) {
         Context.Diags.diagnose(Loc, diag::use_local_before_declaration, Name);
@@ -649,13 +630,6 @@ Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE,
           findNonMembers(Lookup.innerResults(), UDRE->getRefKind(),
                          /*breakOnMember=*/true, ResultValues, isValid);
     }
-
-    // Drop outer results if they are not supposed to be included.
-    if (!lookupOptions.contains(NameLookupFlags::IncludeOuterResults)) {
-      Lookup.filter([&](LookupResultEntry Result, bool isOuter) {
-          return !isOuter;
-      });
-    }
   }
 
   // If we have an unambiguous reference to a type decl, form a TypeExpr.
@@ -715,7 +689,6 @@ Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE,
 
   ResultValues.clear();
   bool AllMemberRefs = true;
-  bool AllConditionalConformances = true;
   ValueDecl *Base = nullptr;
   DeclContext *BaseDC = nullptr;
   for (auto Result : Lookup) {
@@ -732,26 +705,6 @@ Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE,
 
       Base = ThisBase;
       BaseDC = Result.getDeclContext();
-
-      // Check if this result is derived through a conditional conformance,
-      // meaning it comes from a protocol (or extension) where there's a
-      // conditional conformance for the type with the method in question
-      // (NB. that type may not be the type associated with DC, for tested types
-      // with static methods).
-      if (auto Proto = Value->getDeclContext()->getSelfProtocolDecl()) {
-        auto contextSelfType =
-            BaseDC->getInnermostTypeContext()->getDeclaredInterfaceType();
-        auto conformance = conformsToProtocol(
-            contextSelfType, Proto, DC,
-            ConformanceCheckFlags::InExpression |
-                ConformanceCheckFlags::SkipConditionalRequirements);
-
-        if (conformance.isInvalid() ||
-            conformance.getConditionalRequirements().empty()) {
-          AllConditionalConformances = false;
-        }
-      }
-
       continue;
     }
 
@@ -774,22 +727,12 @@ Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE,
                                            /*Implicit=*/true);
     }
 
-    // We *might* include any non-members that we found in outer contexts in
-    // some special cases, for backwards compatibility: first, we have to be
-    // looking for one of the special names
-    // ('shouldConsiderOuterResultsFor(Name)'), and second, all of the inner
-    // results need to come from conditional conformances. The second condition
-    // is how the problem here was encountered: a type ('Range') was made to
-    // conditionally conform to a new protocol ('Sequence'), which introduced
-    // some extra methods ('min' and 'max') that shadowed global functions that
-    // people regularly called within extensions to that type (usually adding
-    // 'clamp').
     llvm::SmallVector<ValueDecl *, 4> outerAlternatives;
-    if (AllConditionalConformances) {
-      (void)findNonMembers(Lookup.outerResults(), UDRE->getRefKind(),
-                           /*breakOnMember=*/false, outerAlternatives,
-                           /*isValid=*/[&](ValueDecl *) { return true; });
-    }
+    (void)findNonMembers(Lookup.outerResults(), UDRE->getRefKind(),
+                         /*breakOnMember=*/false, outerAlternatives,
+                         /*isValid=*/[](ValueDecl *choice) -> bool {
+                           return !choice->isInvalid();
+                         });
 
     // Otherwise, form an UnresolvedDotExpr and sema will resolve it based on
     // type information.

test/APINotes/versioned-objc.swift

@@ -166,7 +166,7 @@ extension PrintingInterference {
   func testDroppingRenamedPrints() {
     // CHECK-DIAGS-4: [[@LINE+1]]:{{[0-9]+}}: warning: use of 'print' treated as a reference to instance method
     print(self)
-    // CHECK-DIAGS-5: [[@LINE-1]]:{{[0-9]+}}: error: missing argument for parameter 'extra' in call
+    // CHECK-DIAGS-5: [[@LINE-1]]:{{[0-9]+}}: error: use of 'print' refers to instance method rather than global function 'print(_:separator:terminator:)' in module 'Swift'
 
     // CHECK-DIAGS-4-NOT: [[@LINE+1]]:{{[0-9]+}}:
     print(self, extra: self)

test/Constraints/members.swift

@@ -355,7 +355,7 @@ do {
 // rdar://problem/25341015
 extension Sequence {
   func r25341015_1() -> Int {
-    return max(1, 2) // expected-error {{use of 'max' refers to instance method 'max(by:)' rather than global function 'max' in module 'Swift'}} expected-note {{use 'Swift.' to reference the global function in module 'Swift'}}
+    return max(1, 2) // expected-error {{use of 'max' refers to instance method rather than global function 'max' in module 'Swift'}} expected-note {{use 'Swift.' to reference the global function in module 'Swift'}}
   }
 }
 
@@ -381,7 +381,7 @@ func r25341015() {
   class Bar {
     func baz() {}
     func qux() {
-      baz(1, 2) // expected-error {{argument passed to call that takes no arguments}}
+      baz(1, 2) // expected-error {{use of 'baz' refers to instance method rather than local function 'baz'}}
     }
   }
 }
@@ -405,17 +405,17 @@ func bar_32854314() -> Int {
 extension Array where Element == Int {
   func foo() {
     let _ = min(foo_32854314(), bar_32854314()) // expected-note {{use 'Swift.' to reference the global function in module 'Swift'}} {{13-13=Swift.}}
-    // expected-error@-1 {{use of 'min' nearly matches global function 'min' in module 'Swift' rather than instance method 'min(by:)'}}
+    // expected-error@-1 {{use of 'min' refers to instance method rather than global function 'min' in module 'Swift'}}
   }
 
   func foo(_ x: Int, _ y: Double) {
     let _ = min(x, y) // expected-note {{use 'Swift.' to reference the global function in module 'Swift'}} {{13-13=Swift.}}
-    // expected-error@-1 {{use of 'min' nearly matches global function 'min' in module 'Swift' rather than instance method 'min(by:)'}}
+    // expected-error@-1 {{use of 'min' refers to instance method rather than global function 'min' in module 'Swift'}}
   }
 
   func bar() {
     let _ = min(1.0, 2) // expected-note {{use 'Swift.' to reference the global function in module 'Swift'}} {{13-13=Swift.}}
-    // expected-error@-1 {{use of 'min' nearly matches global function 'min' in module 'Swift' rather than instance method 'min(by:)'}}
+    // expected-error@-1 {{use of 'min' refers to instance method rather than global function 'min' in module 'Swift'}}
   }
 }
 

test/NameBinding/name_lookup_min_max_conditional_conformance.swift

@@ -16,7 +16,7 @@ extension ContainsMinMax {
     func min() {}
 }
 
-func foo(_: Int, _: Int) {}
+func foo(_: Int, _: Int) {} // expected-note 2 {{'foo' declared here}}
 
 protocol ContainsFoo {}
 extension ContainsFoo {
@@ -34,15 +34,14 @@ extension NonConditional {
         // expected-error@-1{{use of 'min' refers to instance method}}
         // expected-note@-2{{use 'Swift.' to reference the global function}}
 
-        // FIXME(diagnostics): Better diagnostic in this case would be to suggest to add `name_lookup_min_max_conditional_conformance.`
-        // to call because name `foo` is shadowed by instance method without arguments. Would be fixed by `resolveDeclRefExpr` refactoring.
-        _ = foo(5, 6) // expected-error {{argument passed to call that takes no arguments}}
+        _ = foo(5, 6) // expected-error {{use of 'foo' refers to instance method rather than global function 'foo' in module 'name_lookup_min_max_conditional_conformance'}}
+       // expected-note@-1 {{use 'name_lookup_min_max_conditional_conformance.' to reference the global function in module 'name_lookup_min_max_conditional_conformance'}} {{13-13=name_lookup_min_max_conditional_conformance.}}
     }
 }
 
 struct Conditional<T> {}
 extension Conditional: ContainsMinMax where T: ContainsMinMax {}
-extension Conditional: ContainsFoo where T: ContainsFoo {} // expected-note {{requirement from conditional conformance of 'Conditional<T>' to 'ContainsFoo'}}
+extension Conditional: ContainsFoo where T: ContainsFoo {}
 
 extension Conditional {
     func f() {
@@ -53,9 +52,8 @@ extension Conditional {
         // expected-warning@-1{{use of 'min' as reference to global function in module 'Swift' will change in future versions of Swift to reference instance method in generic struct 'Conditional' which comes via a conditional conformance}}
         // expected-note@-2{{use 'Swift.' to continue to reference the global function}}
 
-        // FIXME(diagnostics): Same as line 39, there should be only one error here about shadowing.
         _ = foo(5, 6)
-        // expected-error@-1 {{referencing instance method 'foo()' on 'Conditional' requires that 'T' conform to 'ContainsFoo'}}
-        // expected-error@-2 {{argument passed to call that takes no arguments}}
+        // expected-error@-1 {{use of 'foo' refers to instance method rather than global function 'foo' in module 'name_lookup_min_max_conditional_conformance'}}
+        // expected-note@-2 {{use 'name_lookup_min_max_conditional_conformance.' to reference the global function in module 'name_lookup_min_max_conditional_conformance'}} {{13-13=name_lookup_min_max_conditional_conformance.}}
     }
 }

test/Sema/circular_decl_checking.swift

@@ -23,9 +23,10 @@ class HasGenericFunc {
   }
 }
 
-class HasProp {
+class HasProp { // expected-note {{'HasProp' declared here}}
   var HasProp: HasProp {
-    return HasProp() // expected-error {{cannot call value of non-function type 'HasProp'}}{{19-21=}}
+    return HasProp() // expected-error {{use of 'HasProp' refers to instance method rather than class 'HasProp' in module 'circular_decl_checking'}}
+    // expected-note@-1 {{use 'circular_decl_checking.' to reference the class in module 'circular_decl_checking'}} {{12-12=circular_decl_checking.}}
   }
   var SomethingElse: SomethingElse? { // expected-error {{use of undeclared type 'SomethingElse'}}
     return nil