Merge pull request #29341 from brentdax/path-dependence

beccadax · web-flow · commit 7df141cdad45 · 2020-01-21T20:45:47.000-08:00
[MiscDiagnostics] Warn if magic identifiers don’t match
diff --git a/include/swift/AST/Decl.h b/include/swift/AST/Decl.h
@@ -7405,10 +7405,12 @@ inline EnumElementDecl *EnumDecl::getUniqueElement(bool hasValue) const {
   return result;
 }
 
-/// Retrieve the parameter list for a given declaration.
+/// Retrieve the parameter list for a given declaration, or nullputr if there
+/// is none.
 ParameterList *getParameterList(ValueDecl *source);
 
-/// Retrieve parameter declaration from the given source at given index.
+/// Retrieve parameter declaration from the given source at given index, or
+/// nullptr if the source does not have a parameter list.
 const ParamDecl *getParameterAt(const ValueDecl *source, unsigned index);
 
 /// Display Decl subclasses.
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -4678,6 +4678,17 @@ WARNING(observe_keypath_property_not_objc_dynamic, none,
         "passing reference to non-'@objc dynamic' property %0 to KVO method %1 "
         "may lead to unexpected behavior or runtime trap", (DeclName, DeclName))
 
+WARNING(default_magic_identifier_mismatch, none,
+        "parameter %0 with default argument '%1' passed to parameter %2, whose "
+        "default argument is '%3'",
+        (Identifier, StringRef, Identifier, StringRef))
+NOTE(change_caller_default_to_match_callee, none,
+     "did you mean for parameter %0 to default to '%1'?",
+     (Identifier, StringRef))
+NOTE(silence_default_magic_identifier_mismatch, none,
+     "add parentheses to silence this warning", ())
+
+
 //------------------------------------------------------------------------------
 // MARK: Debug diagnostics
 //------------------------------------------------------------------------------
diff --git a/include/swift/AST/Expr.h b/include/swift/AST/Expr.h
@@ -503,7 +503,7 @@ class alignas(8) Expr {
 
   /// Retrieves the declaration that is being referenced by this
   /// expression, if any.
-  ConcreteDeclRef getReferencedDecl() const;
+  ConcreteDeclRef getReferencedDecl(bool stopAtParenExpr = false) const;
 
   /// Determine whether this expression is 'super', possibly converted to
   /// a base class.
@@ -1049,6 +1049,20 @@ class MagicIdentifierLiteralExpr : public LiteralExpr {
   enum Kind : unsigned {
     File, FilePath, Line, Column, Function, DSOHandle
   };
+
+  static StringRef getKindString(MagicIdentifierLiteralExpr::Kind value) {
+    switch (value) {
+      case File: return "#file";
+      case FilePath: return "#filePath";
+      case Function: return "#function";
+      case Line: return "#line";
+      case Column: return "#column";
+      case DSOHandle: return "#dsohandle";
+    }
+
+    llvm_unreachable("Unhandled MagicIdentifierLiteralExpr in getKindString.");
+  }
+
 private:
   SourceLoc Loc;
   ConcreteDeclRef BuiltinInitializer;
diff --git a/lib/AST/ASTDumper.cpp b/lib/AST/ASTDumper.cpp
@@ -314,19 +314,6 @@ static StringRef getDefaultArgumentKindString(DefaultArgumentKind value) {
   llvm_unreachable("Unhandled DefaultArgumentKind in switch.");
 }
 static StringRef
-getMagicIdentifierLiteralExprKindString(MagicIdentifierLiteralExpr::Kind value) {
-  switch (value) {
-    case MagicIdentifierLiteralExpr::File: return "#file";
-    case MagicIdentifierLiteralExpr::FilePath: return "#filePath";
-    case MagicIdentifierLiteralExpr::Function: return "#function";
-    case MagicIdentifierLiteralExpr::Line: return "#line";
-    case MagicIdentifierLiteralExpr::Column: return "#column";
-    case MagicIdentifierLiteralExpr::DSOHandle: return "#dsohandle";
-  }
-
-  llvm_unreachable("Unhandled MagicIdentifierLiteralExpr in switch.");
-}
-static StringRef
 getObjCSelectorExprKindString(ObjCSelectorExpr::ObjCSelectorKind value) {
   switch (value) {
     case ObjCSelectorExpr::Method: return "method";
@@ -1957,7 +1944,7 @@ class PrintExpr : public ExprVisitor<PrintExpr> {
   }
   void visitMagicIdentifierLiteralExpr(MagicIdentifierLiteralExpr *E) {
     printCommon(E, "magic_identifier_literal_expr")
-      << " kind=" << getMagicIdentifierLiteralExprKindString(E->getKind());
+      << " kind=" << MagicIdentifierLiteralExpr::getKindString(E->getKind());
 
     if (E->isString()) {
       OS << " encoding="
diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp
@@ -6623,14 +6623,19 @@ ParameterList *swift::getParameterList(ValueDecl *source) {
     return AFD->getParameters();
   } else if (auto *EED = dyn_cast<EnumElementDecl>(source)) {
     return EED->getParameterList();
-  } else {
-    return cast<SubscriptDecl>(source)->getIndices();
+  } else if (auto *SD = dyn_cast<SubscriptDecl>(source)) {
+    return SD->getIndices();
   }
+
+  return nullptr;
 }
 
 const ParamDecl *swift::getParameterAt(const ValueDecl *source,
                                        unsigned index) {
-  return getParameterList(const_cast<ValueDecl *>(source))->get(index);
+  if (auto *params = getParameterList(const_cast<ValueDecl *>(source))) {
+    return params->get(index);
+  }
+  return nullptr;
 }
 
 Type AbstractFunctionDecl::getMethodInterfaceType() const {
diff --git a/lib/AST/Expr.cpp b/lib/AST/Expr.cpp
@@ -228,7 +228,7 @@ DeclRefExpr *Expr::getMemberOperatorRef() {
   return operatorRef;
 }
 
-ConcreteDeclRef Expr::getReferencedDecl() const {
+ConcreteDeclRef Expr::getReferencedDecl(bool stopAtParenExpr) const {
   switch (getKind()) {
   // No declaration reference.
   #define NO_REFERENCE(Id) case ExprKind::Id: return ConcreteDeclRef()
@@ -237,7 +237,8 @@ ConcreteDeclRef Expr::getReferencedDecl() const {
       return cast<Id##Expr>(this)->Getter()
   #define PASS_THROUGH_REFERENCE(Id, GetSubExpr)                      \
     case ExprKind::Id:                                                \
-      return cast<Id##Expr>(this)->GetSubExpr()->getReferencedDecl()
+      return cast<Id##Expr>(this)                                     \
+                 ->GetSubExpr()->getReferencedDecl(stopAtParenExpr)
 
   NO_REFERENCE(Error);
   NO_REFERENCE(NilLiteral);
@@ -275,7 +276,12 @@ ConcreteDeclRef Expr::getReferencedDecl() const {
   NO_REFERENCE(UnresolvedMember);
   NO_REFERENCE(UnresolvedDot);
   NO_REFERENCE(Sequence);
-  PASS_THROUGH_REFERENCE(Paren, getSubExpr);
+
+  case ExprKind::Paren:
+    if (stopAtParenExpr) return ConcreteDeclRef();
+    return cast<ParenExpr>(this)
+               ->getSubExpr()->getReferencedDecl(stopAtParenExpr);
+
   PASS_THROUGH_REFERENCE(DotSelf, getSubExpr);
   PASS_THROUGH_REFERENCE(Try, getSubExpr);
   PASS_THROUGH_REFERENCE(ForceTry, getSubExpr);
diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp
@@ -194,28 +194,30 @@ static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,
           callee = dynamicMRE->getMember();
         }
 
-        visitArguments(Call, [&](unsigned argIndex, Expr *arg) {
-          // InOutExprs can be wrapped in some implicit casts.
-          Expr *unwrapped = arg;
-          if (auto *IIO = dyn_cast<InjectIntoOptionalExpr>(arg))
-            unwrapped = IIO->getSubExpr();
-
-          if (isa<InOutToPointerExpr>(unwrapped) ||
-              isa<ArrayToPointerExpr>(unwrapped) ||
-              isa<ErasureExpr>(unwrapped)) {
-            auto operand =
-              cast<ImplicitConversionExpr>(unwrapped)->getSubExpr();
-            if (auto *IOE = dyn_cast<InOutExpr>(operand))
-              operand = IOE->getSubExpr();
-
-            // Also do some additional work based on how the function uses
-            // the argument.
-            if (callee) {
+        if (callee) {
+          visitArguments(Call, [&](unsigned argIndex, Expr *arg) {
+            checkMagicIdentifierMismatch(callee, uncurryLevel, argIndex, arg);
+
+            // InOutExprs can be wrapped in some implicit casts.
+            Expr *unwrapped = arg;
+            if (auto *IIO = dyn_cast<InjectIntoOptionalExpr>(arg))
+              unwrapped = IIO->getSubExpr();
+
+            if (isa<InOutToPointerExpr>(unwrapped) ||
+                isa<ArrayToPointerExpr>(unwrapped) ||
+                isa<ErasureExpr>(unwrapped)) {
+              auto operand =
+                cast<ImplicitConversionExpr>(unwrapped)->getSubExpr();
+              if (auto *IOE = dyn_cast<InOutExpr>(operand))
+                operand = IOE->getSubExpr();
+
+              // Also do some additional work based on how the function uses
+              // the argument.
               checkConvertedPointerArgument(callee, uncurryLevel, argIndex,
                                             unwrapped, operand);
             }
-          }
-        });
+          });
+        }
       }
       
       // If we have an assignment expression, scout ahead for acceptable _'s.
@@ -421,6 +423,91 @@ static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,
       // Otherwise, we can't support this.
     }
 
+    void checkMagicIdentifierMismatch(ConcreteDeclRef callee,
+                                      unsigned uncurryLevel,
+                                      unsigned argIndex,
+                                      Expr *arg) {
+      // We only care about args in the arg list.
+      if (uncurryLevel != (callee.getDecl()->hasCurriedSelf() ? 1 : 0))
+        return;
+
+      // Get underlying params for both callee and caller, if declared.
+      auto *calleeParam = getParameterAt(callee.getDecl(), argIndex);
+      auto *callerParam = dyn_cast_or_null<ParamDecl>(
+          arg->getReferencedDecl(/*stopAtParenExpr=*/true).getDecl()
+      );
+
+      // (Otherwise, we don't need to do anything.)
+      if (!calleeParam || !callerParam)
+        return;
+
+      auto calleeDefaultArg = getMagicIdentifierDefaultArgKind(calleeParam);
+      auto callerDefaultArg = getMagicIdentifierDefaultArgKind(callerParam);
+
+      // If one of the parameters doesn't have a default arg, or they both have
+      // the same one, everything's fine.
+      if (!calleeDefaultArg || !callerDefaultArg ||
+          *calleeDefaultArg == *callerDefaultArg)
+        return;
+
+      StringRef calleeDefaultArgString =
+          MagicIdentifierLiteralExpr::getKindString(*calleeDefaultArg);
+      StringRef callerDefaultArgString =
+          MagicIdentifierLiteralExpr::getKindString(*callerDefaultArg);
+
+      // Emit main warning
+      Ctx.Diags.diagnose(arg->getLoc(), diag::default_magic_identifier_mismatch,
+                         callerParam->getName(), callerDefaultArgString,
+                         calleeParam->getName(), calleeDefaultArgString);
+
+      // Add "change caller default arg" fixit
+      SourceLoc callerDefaultArgLoc =
+          callerParam->getStructuralDefaultExpr()->getLoc();
+      Ctx.Diags.diagnose(callerDefaultArgLoc,
+                         diag::change_caller_default_to_match_callee,
+                         callerParam->getName(), calleeDefaultArgString)
+        .fixItReplace(callerDefaultArgLoc, calleeDefaultArgString);
+
+      // Add "silence with parens" fixit
+      Ctx.Diags.diagnose(arg->getLoc(),
+                         diag::silence_default_magic_identifier_mismatch)
+        .fixItInsert(arg->getStartLoc(), "(")
+        .fixItInsertAfter(arg->getEndLoc(), ")");
+
+      // Point to callee parameter
+      Ctx.Diags.diagnose(calleeParam, diag::decl_declared_here,
+                         calleeParam->getFullName());
+    }
+
+    Optional<MagicIdentifierLiteralExpr::Kind>
+    getMagicIdentifierDefaultArgKind(const ParamDecl *param) {
+      switch (param->getDefaultArgumentKind()) {
+      case DefaultArgumentKind::Column:
+        return MagicIdentifierLiteralExpr::Kind::Column;
+      case DefaultArgumentKind::DSOHandle:
+        return MagicIdentifierLiteralExpr::Kind::DSOHandle;
+      case DefaultArgumentKind::File:
+        return MagicIdentifierLiteralExpr::Kind::File;
+      case DefaultArgumentKind::FilePath:
+        return MagicIdentifierLiteralExpr::Kind::FilePath;
+      case DefaultArgumentKind::Function:
+        return MagicIdentifierLiteralExpr::Kind::Function;
+      case DefaultArgumentKind::Line:
+        return MagicIdentifierLiteralExpr::Kind::Line;
+
+      case DefaultArgumentKind::None:
+      case DefaultArgumentKind::Normal:
+      case DefaultArgumentKind::Inherited:
+      case DefaultArgumentKind::NilLiteral:
+      case DefaultArgumentKind::EmptyArray:
+      case DefaultArgumentKind::EmptyDictionary:
+      case DefaultArgumentKind::StoredProperty:
+        return None;
+      }
+
+      llvm_unreachable("Unhandled DefaultArgumentKind in "
+                       "getMagicIdentifierDefaultArgKind");
+    }
 
     void checkUseOfModule(DeclRefExpr *E) {
       // Allow module values as a part of:
diff --git a/lib/Sema/TypeCheckStmt.cpp b/lib/Sema/TypeCheckStmt.cpp
@@ -1390,14 +1390,8 @@ static void diagnoseIgnoredLiteral(ASTContext &Ctx, LiteralExpr *LE) {
     case ExprKind::StringLiteral: return "string";
     case ExprKind::InterpolatedStringLiteral: return "string";
     case ExprKind::MagicIdentifierLiteral:
-      switch (cast<MagicIdentifierLiteralExpr>(LE)->getKind()) {
-      case MagicIdentifierLiteralExpr::Kind::File: return "#file";
-      case MagicIdentifierLiteralExpr::Kind::FilePath: return "#filePath";
-      case MagicIdentifierLiteralExpr::Kind::Line: return "#line";
-      case MagicIdentifierLiteralExpr::Kind::Column: return "#column";
-      case MagicIdentifierLiteralExpr::Kind::Function: return "#function";
-      case MagicIdentifierLiteralExpr::Kind::DSOHandle: return "#dsohandle";
-      }
+      return MagicIdentifierLiteralExpr::getKindString(
+          cast<MagicIdentifierLiteralExpr>(LE)->getKind());
     case ExprKind::NilLiteral: return "nil";
     case ExprKind::ObjectLiteral: return "object";
 
diff --git a/test/Sema/diag_mismatched_magic_literals.swift b/test/Sema/diag_mismatched_magic_literals.swift
diff --git a/test/multifile/default-arguments/one-module/Inputs/mismatched-magic-literals-other.swift b/test/multifile/default-arguments/one-module/Inputs/mismatched-magic-literals-other.swift
diff --git a/test/multifile/default-arguments/one-module/mismatched-magic-literals.swift b/test/multifile/default-arguments/one-module/mismatched-magic-literals.swift
