[Diagnostics|SR-5789] Fixits for applying subscripts by keyword (#16431)

* [Diagnostics|SR-5789] Added fixits for referring to subscripts by keyword

If there is a subscript member, the error message changes to [type -bash has no member property or method named 'subscript']
The fix-it replaces parentheses with brackets, removes '.subscript'
If the apply expression is incomplete, e.g. subscript(..., the fix-it adds a bracket at the end.

* tests updated & logic for compatible arg types (except generics)

* ignore generic types & switch to returning

* avoid explicitly using sting literals

* handle implicit conversion of tuples & encapsulate it

* isolate subscript misusage

* return bool instead of void

* move function to FailureDiagnosis, diagnose independently & update error message

* Update CSDiag.cpp

Author: AnthonyLatsis

include/swift/AST/DiagnosticsSema.def

@@ -75,6 +75,13 @@ ERROR(could_not_find_type_member_corrected,none,
       "type %0 has no member %1; did you mean %2?",
       (Type, DeclName, DeclName))
 
+ERROR(could_not_find_subscript_member_did_you_mean,none,
+      "value of type %0 has no property or method named 'subscript'; "
+      "did you mean to use the subscript operator?",
+      (Type))
+NOTE(subscript_declared_here,none,
+      "declared here", ())
+
 ERROR(could_not_find_enum_case,none,
       "enum type %0 has no case %1; did you mean %2", (Type, DeclName, DeclName))
 

lib/Sema/CSDiag.cpp

@@ -1002,6 +1002,10 @@ class FailureDiagnosis :public ASTVisitor<FailureDiagnosis, /*exprresult*/bool>{
 
   bool diagnoseSubscriptErrors(SubscriptExpr *SE, bool performingSet);
 
+  /// Diagnose the usage of 'subscript' instead of the operator when calling
+  /// a subscript and offer a fixit if the inputs are compatible.
+  bool diagnoseSubscriptMisuse(ApplyExpr *callExpr);
+
   bool visitExpr(Expr *E);
   bool visitIdentityExpr(IdentityExpr *E);
   bool visitTryExpr(TryExpr *E);
@@ -5434,6 +5438,141 @@ bool FailureDiagnosis::diagnoseCallContextualConversionErrors(
   return false;
 }
 
+bool FailureDiagnosis::diagnoseSubscriptMisuse(ApplyExpr *callExpr) {
+  auto UDE = dyn_cast<UnresolvedDotExpr>(callExpr->getFn());
+  if (!UDE)
+    return false;
+
+  auto baseExpr = UDE->getBase();
+  if (!baseExpr || UDE->getName().getBaseName() != getTokenText(tok::kw_subscript))
+    return false;
+
+  auto baseType = CS.getType(baseExpr);
+  // Look up subscript declarations.
+  auto lookup = CS.lookupMember(baseType->getRValueType(),
+                                DeclName(DeclBaseName::createSubscript()));
+  auto nonSubscrLookup = CS.lookupMember(baseType->getRValueType(),
+                                         UDE->getName());
+  // Make sure we only found subscript declarations. If not, the problem
+  // is different - return.
+  if (lookup.empty() || !nonSubscrLookup.empty())
+    return false;
+  // Try to resolve a type of the argument expression.
+  auto argExpr = typeCheckChildIndependently(callExpr->getArg(),
+                                             Type(), CTP_CallArgument);
+  if (!argExpr)
+    return CS.TC.Diags.hadAnyError();
+
+  SmallVector<Identifier, 2> scratch;
+  ArrayRef<Identifier> argLabels = callExpr->getArgumentLabels(scratch);
+  SmallVector<OverloadChoice, 2> choices;
+
+  for (auto candidate : lookup)
+    choices.push_back(OverloadChoice(baseType, candidate.getValueDecl(),
+                                     UDE->getFunctionRefKind()));
+  CalleeCandidateInfo candidateInfo(baseType, choices,
+                                    callArgHasTrailingClosure(argExpr),
+                                    CS, true);
+  struct TypeConvertibleChecker {
+    ConstraintSystem &CS;
+
+    TypeConvertibleChecker(ConstraintSystem &CS) : CS(CS) {}
+
+    // Checks whether type1 is implicitly convertible to type2
+    bool isImplicitlyConvertible(Type type1, Type type2) {
+      if (!type1 || !type2)
+        return false;
+
+      if (auto tup1 = type1->getAs<TupleType>()) {
+        if (auto tup2 = type2->getAs<TupleType>())
+          return isImplicitlyConvertibleTuple(tup1, tup2);
+        else
+          return false;
+      }
+      if (type1->isEqual(type2) || type2->is<GenericTypeParamType>() ||
+          CS.TC.isSubtypeOf(type1, type2, CS.DC))
+        return true;
+      return false;
+    }
+
+    bool isImplicitlyConvertibleTuple(TupleType *tup1, TupleType *tup2) {
+      if (tup1->getNumElements() != tup2->getNumElements())
+        return false;
+
+      for (unsigned i = 0, e = tup1->getNumElements(); i < e; i ++) {
+        auto element1 = tup1->getElement(i);
+        auto element2 = tup2->getElement(i);
+        if (element1.hasName()) {
+          if (!element2.hasName() || element1.getName() !=
+              element2.getName())
+            return false;
+        }
+        if (!isImplicitlyConvertible(element1.getType(),
+                                     element2.getType()))
+          return false;
+      }
+      return true;
+    }
+  } checker(CS);
+  auto params = swift::decomposeArgType(CS.getType(argExpr), argLabels);
+  using ClosenessPair = CalleeCandidateInfo::ClosenessResultTy;
+
+  candidateInfo.filterList([&](UncurriedCandidate cand) -> ClosenessPair {
+    auto candFuncType = cand.getUncurriedFunctionType();
+    if (!candFuncType)
+      return {CC_GeneralMismatch, {}};
+
+    auto candParams = candFuncType->getParams();
+    if (params.size() != candParams.size())
+      return {CC_GeneralMismatch, {}};
+
+    for (unsigned i = 0, e = params.size(); i < e; i ++) {
+      if (checker.isImplicitlyConvertible(params[i].getType(),
+                                          candParams[i].getType()))
+        continue;
+      return {CC_GeneralMismatch, {}};
+    }
+    return {CC_ExactMatch, {}};
+  });
+
+  auto *locator = CS.getConstraintLocator(UDE, ConstraintLocator::Member);
+  auto memberRange = baseExpr->getSourceRange();
+  if (locator)
+    locator = simplifyLocator(CS, locator, memberRange);
+  auto nameLoc = DeclNameLoc(memberRange.Start);
+
+  auto diag = diagnose(baseExpr->getLoc(),
+                       diag::could_not_find_subscript_member_did_you_mean,
+                       baseType);
+  diag.highlight(memberRange).highlight(nameLoc.getSourceRange());
+
+  if (candidateInfo.closeness != CC_ExactMatch)
+    return true;
+
+  auto toCharSourceRange = Lexer::getCharSourceRangeFromSourceRange;
+  auto lastArgSymbol = toCharSourceRange(CS.TC.Context.SourceMgr,
+                                         argExpr->getEndLoc());
+
+  diag.fixItReplace(SourceRange(argExpr->getStartLoc()),
+                    getTokenText(tok::l_square));
+  diag.fixItRemove(nameLoc.getSourceRange());
+  diag.fixItRemove(SourceRange(UDE->getDotLoc()));
+  if (CS.TC.Context.SourceMgr.extractText(lastArgSymbol) ==
+      getTokenText(tok::r_paren))
+    diag.fixItReplace(SourceRange(argExpr->getEndLoc()),
+                      getTokenText(tok::r_square));
+  else
+    diag.fixItInsertAfter(argExpr->getEndLoc(),
+                          getTokenText(tok::r_square));
+  diag.flush();
+
+  if (candidateInfo.size() == 1)
+    diagnose(candidateInfo.candidates.front().getDecl(),
+             diag::subscript_declared_here);
+
+  return true;
+}
+
 // Check if there is a structural problem in the function expression
 // by performing type checking with the option to allow unresolved
 // type variables. If that is going to produce a function type with
@@ -5480,11 +5619,18 @@ bool FailureDiagnosis::visitApplyExpr(ApplyExpr *callExpr) {
   auto originalFnType = CS.getType(callExpr->getFn());
 
   if (shouldTypeCheckFunctionExpr(CS.TC, CS.DC, fnExpr)) {
+
+    // If we are misusing a subscript, diagnose that and provide a fixit.
+    // We diagnose this here to have enough context to offer an appropriate fixit.
+    if (diagnoseSubscriptMisuse(callExpr)) {
+      return CS.TC.Diags.hadAnyError();
+    }
     // Type check the function subexpression to resolve a type for it if
     // possible.
     fnExpr = typeCheckChildIndependently(callExpr->getFn());
-    if (!fnExpr)
-      return true;
+    if (!fnExpr) {
+      return CS.TC.Diags.hadAnyError();
+    }
   }
 
   SWIFT_DEFER {

lib/Sema/CalleeCandidateInfo.cpp

@@ -835,7 +835,7 @@ CalleeCandidateInfo::CalleeCandidateInfo(Type baseType,
     // the uncurry level is 1 if self has already been applied.
     unsigned uncurryLevel = 0;
     if (decl->getDeclContext()->isTypeContext() &&
-        selfAlreadyApplied)
+        selfAlreadyApplied && !isa<SubscriptDecl>(decl))
       uncurryLevel = 1;
 
     candidates.push_back({ decl, uncurryLevel });

test/decl/subscript/subscripting.swift

@@ -246,33 +246,82 @@ struct MutableSubscriptInGetter {
   }
 }
 
+protocol Protocol {}
+protocol RefinedProtocol: Protocol {}
+class SuperClass {}
+class SubClass: SuperClass {}
+class SubSubClass: SubClass {}
+class ClassConformingToProtocol: Protocol {}
+class ClassConformingToRefinedProtocol: RefinedProtocol {}
+
+struct GenSubscriptFixitTest {
+  subscript<T>(_ arg: T) -> Bool { return true } // expected-note {{declared here}}
+}
+
+func testGenSubscriptFixit(_ s0: GenSubscriptFixitTest) {
+
+  _ = s0.subscript("hello")
+  // expected-error@-1 {{value of type 'GenSubscriptFixitTest' has no property or method named 'subscript'; did you mean to use the subscript operator?}} {{9-10=}} {{10-19=}} {{19-20=[}} {{27-28=]}}
+}
+
 struct SubscriptTest1 {
   subscript(keyword:String) -> Bool { return true }  // expected-note 2 {{found this candidate}}
   subscript(keyword:String) -> String? {return nil }  // expected-note 2 {{found this candidate}}
+
+  subscript(arg: SubClass) -> Bool { return true } // expected-note {{declared here}}
+  subscript(arg: Protocol) -> Bool { return true } // expected-note 2 {{declared here}}
+
+  subscript(arg: (foo: Bool, bar: (Int, baz: SubClass)), arg2: String) -> Bool { return true }
+  // expected-note@-1 2 {{declared here}}
 }
 
 func testSubscript1(_ s1 : SubscriptTest1) {
   let _ : Int = s1["hello"]  // expected-error {{ambiguous subscript with base type 'SubscriptTest1' and index type 'String'}}
-  
+
   if s1["hello"] {}
-  
-  
-  let _ = s1["hello"]  // expected-error {{ambiguous use of 'subscript'}}
+
+  _ = s1.subscript((true, (5, SubClass())), "hello")
+  // expected-error@-1 {{value of type 'SubscriptTest1' has no property or method named 'subscript'; did you mean to use the subscript operator?}} {{9-10=}} {{10-19=}} {{19-20=[}} {{52-53=]}}
+  _ = s1.subscript((true, (5, baz: SubSubClass())), "hello")
+  // expected-error@-1 {{value of type 'SubscriptTest1' has no property or method named 'subscript'; did you mean to use the subscript operator?}} {{9-10=}} {{10-19=}} {{19-20=[}} {{60-61=]}}
+  _ = s1.subscript((fo: true, (5, baz: SubClass())), "hello")
+  // expected-error@-1 {{value of type 'SubscriptTest1' has no property or method named 'subscript'; did you mean to use the subscript operator?}}
+  _ = s1.subscript(SubSubClass())
+  // expected-error@-1 {{value of type 'SubscriptTest1' has no property or method named 'subscript'; did you mean to use the subscript operator?}} {{9-10=}} {{10-19=}} {{19-20=[}} {{33-34=]}}
+  _ = s1.subscript(ClassConformingToProtocol())
+  // expected-error@-1 {{value of type 'SubscriptTest1' has no property or method named 'subscript'; did you mean to use the subscript operator?}} {{9-10=}} {{10-19=}} {{19-20=[}} {{47-48=]}}
+  _ = s1.subscript(ClassConformingToRefinedProtocol())
+  // expected-error@-1 {{value of type 'SubscriptTest1' has no property or method named 'subscript'; did you mean to use the subscript operator?}} {{9-10=}} {{10-19=}} {{19-20=[}} {{54-55=]}}
+  _ = s1.subscript(true)
+  // expected-error@-1 {{value of type 'SubscriptTest1' has no property or method named 'subscript'; did you mean to use the subscript operator?}}
+  _ = s1.subscript(SuperClass())
+  // expected-error@-1 {{value of type 'SubscriptTest1' has no property or method named 'subscript'; did you mean to use the subscript operator?}}
+  _ = s1.subscript("hello")
+  // expected-error@-1 {{value of type 'SubscriptTest1' has no property or method named 'subscript'; did you mean to use the subscript operator?}} {{9-10=}} {{10-19=}} {{19-20=[}} {{27-28=]}}
+  _ = s1.subscript("hello"
+  // expected-error@-1 {{value of type 'SubscriptTest1' has no property or method named 'subscript'; did you mean to use the subscript operator?}} {{9-10=}} {{10-19=}} {{19-20=[}} {{27-27=]}}
+  // expected-note@-2 {{to match this opening '('}}
+
+  let _ = s1["hello"]
+  // expected-error@-1 {{ambiguous use of 'subscript'}}
+  // expected-error@-2 {{expected ')' in expression list}}
 }
 
 struct SubscriptTest2 {
   subscript(a : String, b : Int) -> Int { return 0 }
+  // expected-note@-1 {{declared here}}
   subscript(a : String, b : String) -> Int { return 0 }
 }
 
 func testSubscript1(_ s2 : SubscriptTest2) {
   _ = s2["foo"] // expected-error {{cannot subscript a value of type 'SubscriptTest2' with an index of type 'String'}}
   // expected-note @-1 {{overloads for 'subscript' exist with these partially matching parameter lists: (String, Int), (String, String)}}
-  
+
   let a = s2["foo", 1.0] // expected-error {{cannot subscript a value of type 'SubscriptTest2' with an index of type '(String, Double)'}}
   // expected-note @-1 {{overloads for 'subscript' exist with these partially matching parameter lists: (String, Int), (String, String)}}
-  
-  
+
+  _ = s2.subscript("hello", 6)
+  // expected-error@-1 {{value of type 'SubscriptTest2' has no property or method named 'subscript'; did you mean to use the subscript operator?}} {{9-10=}} {{10-19=}} {{19-20=[}} {{30-31=]}}
   let b = s2[1, "foo"] // expected-error {{cannot convert value of type 'Int' to expected argument type 'String'}}
 
   // rdar://problem/27449208
@@ -313,9 +362,10 @@ struct S4 {
 
 // SR-2575
 struct SR2575 {
-  subscript() -> Int {
+  subscript() -> Int { // expected-note {{declared here}}
     return 1
   }
 }
 
-SR2575().subscript() // expected-error{{type 'SR2575' has no member 'subscript'}}
+SR2575().subscript()
+// expected-error@-1 {{value of type 'SR2575' has no property or method named 'subscript'; did you mean to use the subscript operator?}} {{9-10=}} {{10-19=}} {{19-20=[}} {{20-21=]}}