Made use of @warn_unused_result a warning with a fixit (#2760)

https://bugs.swift.org/browse/SR-1052

Author: tanadeau

include/swift/AST/Attr.def

@@ -233,9 +233,6 @@ DECL_ATTR(_alignment, Alignment, OnStruct | OnEnum | UserInaccessible, 56)
 SIMPLE_DECL_ATTR(rethrows, Rethrows,
                  OnFunc | OnConstructor | RejectByParser, 57)
 
-DECL_ATTR(warn_unused_result, WarnUnusedResult,
-          OnFunc | OnConstructor | LongAttribute, 58)
-
 DECL_ATTR(_swift_native_objc_runtime_base, SwiftNativeObjCRuntimeBase,
           OnClass | UserInaccessible, 59)
 

include/swift/AST/Attr.h

@@ -1079,59 +1079,6 @@ class SynthesizedProtocolAttr : public DeclAttribute {
   }
 };
 
-/// The @warn_unused_result attribute, which specifies that we should
-/// receive a warning if a function is called but its result is
-/// unused.
-///
-/// The @warn_unused_result attribute can optionally be provided with
-/// a message and a mutable variant. For example:
-///
-/// \code
-/// struct X {
-///   @warn_unused_result(message: "this string affects your health")
-///   func methodA() -> String { ... }
-///
-///   @warn_unused_result(mutable_variant: "jumpInPlace")
-///   func jump() -> X { ... }
-///
-///   mutating func jumpInPlace() { ... }
-/// }
-/// \endcode
-class WarnUnusedResultAttr : public DeclAttribute {
-  /// The optional message.
-  const StringRef Message;
-
-  /// An optional name of the mutable variant of the given method,
-  /// which will be suggested as a replacement if it can be called.
-  const StringRef MutableVariant;
-
-public:
-  /// A @warn_unused_result attribute with no options.
-  WarnUnusedResultAttr(SourceLoc atLoc, SourceLoc attrLoc, bool implicit)
-    : DeclAttribute(DAK_WarnUnusedResult, atLoc, SourceRange(atLoc, attrLoc),
-                    implicit),
-      Message(""), MutableVariant("") { }
-
-  /// A @warn_unused_result attribute with a message or mutable variant.
-  WarnUnusedResultAttr(SourceLoc atLoc, SourceLoc attrLoc, SourceLoc lParenLoc,
-                       StringRef message, StringRef mutableVariant,
-                       SourceLoc rParenLoc, bool implicit)
-    : DeclAttribute(DAK_WarnUnusedResult, attrLoc,
-                    SourceRange(atLoc, rParenLoc),
-                    implicit),
-      Message(message), MutableVariant(mutableVariant) { }
-
-  /// Retrieve the message associated with this attribute.
-  StringRef getMessage() const { return Message; }
-
-  /// Retrieve the name of the mutable variant of this method.
-  StringRef getMutableVariant() const { return MutableVariant; }
-
-  static bool classof(const DeclAttribute *DA) {
-    return DA->getKind() == DAK_WarnUnusedResult;
-  }
-};
-
 /// The @swift3_migration attribute which describes the transformations
 /// required to migrate the given Swift 2.x API to Swift 3.
 class Swift3MigrationAttr : public DeclAttribute {

include/swift/AST/DiagnosticsParse.def

@@ -1258,16 +1258,8 @@ ERROR(attr_unowned_expected_rparen,none,
       "expected ')' after specifier for 'unowned'", ())
 
 // warn_unused_result
-WARNING(attr_warn_unused_result_expected_name,none,
-        "expected parameter 'message' or 'mutable_variant'", ())
-WARNING(attr_warn_unused_result_duplicate_parameter,none,
-      "duplicate '%0' parameter; previous value will be ignored", (StringRef))
-ERROR(attr_warn_unused_result_expected_colon,none,
-      "expected ':' following '%0' parameter", (StringRef))
-ERROR(attr_warn_unused_result_expected_string,none,
-      "expected a string following ':' for '%0' parameter", (StringRef))
-WARNING(attr_warn_unused_result_unknown_parameter,none,
-        "unknown parameter '%0' in 'warn_unused_result' attribute", (StringRef))
+WARNING(attr_warn_unused_result_removed,none,
+        "'warn_unused_result' attribute behavior is now the default", ())
 ERROR(attr_warn_unused_result_expected_rparen,none,
       "expected ')' after 'warn_unused_result' attribute", ())
 

include/swift/AST/PrintOptions.h

@@ -346,7 +346,6 @@ struct PrintOptions {
     result.SkipPrivateStdlibDecls = true;
     result.SkipUnderscoredStdlibProtocols = true;
     result.SkipDeinit = true;
-    result.ExcludeAttrList.push_back(DAK_WarnUnusedResult);
     result.ExcludeAttrList.push_back(DAK_DiscardableResult);
     result.EmptyLineBetweenMembers = true;
     result.ElevateDocCommentFromConformance = true;
@@ -432,7 +431,6 @@ struct PrintOptions {
     PO.PrintDocumentationComments = false;
     PO.ExcludeAttrList.push_back(DAK_Available);
     PO.ExcludeAttrList.push_back(DAK_Swift3Migration);
-    PO.ExcludeAttrList.push_back(DAK_WarnUnusedResult);
     PO.SkipPrivateStdlibDecls = true;
     PO.ExplodeEnumCaseDecls = true;
     return PO;

include/swift/Serialization/ModuleFormat.h

@@ -1376,14 +1376,6 @@ namespace decls_block {
     BCBlob      // rename, followed by message
   >;
 
-  using WarnUnusedResultDeclAttrLayout = BCRecordLayout<
-    WarnUnusedResult_DECL_ATTR,
-    BCFixed<1>, // implicit flag
-    BCVBR<6>,  // index at the end of the message,
-    BCBlob     // blob contains the message and mutating-version
-               // strings, separated by the prior index
-  >;
-
   using SpecializeDeclAttrLayout = BCRecordLayout<
     Specialize_DECL_ATTR,
     BCArray<TypeIDField> // concrete types

lib/AST/Attr.cpp

@@ -446,27 +446,6 @@ bool DeclAttribute::printImpl(ASTPrinter &Printer, const PrintOptions &Options)
     break;
   }
 
-  case DAK_WarnUnusedResult: {
-    Printer.printAttrName("@warn_unused_result");
-    auto *attr = cast<WarnUnusedResultAttr>(this);
-    bool printedParens = false;
-    if (!attr->getMessage().empty()) {
-      Printer << "(message: \"" << attr->getMessage() << "\"";
-      printedParens = true;
-    }
-    if (!attr->getMutableVariant().empty()) {
-      if (printedParens)
-        Printer << ", ";
-      else
-        Printer << "(";
-      Printer << "mutable_variant: \"" << attr->getMutableVariant() << "\"";
-      printedParens = true;
-    }
-    if (printedParens)
-      Printer << ")";
-    break;
-  }
-
   case DAK_Specialize: {
     Printer << "@" << getAttrName() << "(";
     auto *attr = cast<SpecializeAttr>(this);
@@ -585,8 +564,6 @@ StringRef DeclAttribute::getAttrName() const {
     return "<<synthesized protocol>>";
   case DAK_Swift3Migration:
     return "swift3_migration";
-  case DAK_WarnUnusedResult:
-    return "warn_unused_result";
   case DAK_Specialize:
     return "_specialize";
   }

lib/ClangImporter/ImportDecl.cpp

@@ -6314,11 +6314,7 @@ void ClangImporter::Implementation::importAttributes(
   }
 
   // Map __attribute__((warn_unused_result)).
-  if (ClangDecl->hasAttr<clang::WarnUnusedResultAttr>()) {
-    MappedDecl->getAttrs().add(new (C) WarnUnusedResultAttr(SourceLoc(),
-                                                            SourceLoc(),
-                                                            false));
-  } else {
+  if (!ClangDecl->hasAttr<clang::WarnUnusedResultAttr>()) {
     if (auto MD = dyn_cast<FuncDecl>(MappedDecl)) {
       if (!MD->getResultType()->isVoid()) {
         MD->getAttrs().add(new (C) DiscardableResultAttr(/*implicit*/true));

lib/Parse/ParseDecl.cpp

@@ -1087,119 +1087,6 @@ bool Parser::parseNewDeclAttribute(DeclAttributes &Attributes, SourceLoc AtLoc,
     break;
   }
 
-  case DAK_WarnUnusedResult: {
-    // @warn_unused_result with no arguments.
-    if (Tok.isNot(tok::l_paren)) {
-      auto attr = new (Context) WarnUnusedResultAttr(AtLoc, Loc, false);
-      Attributes.add(attr);
-      break;
-    }
-
-    // @warn_unused_result with arguments.
-    StringRef message;
-    StringRef mutableVariant;
-    SourceLoc lParenLoc = consumeToken();
-    bool invalid = false;
-    do {
-      // If we see a closing parenthesis,
-      if (Tok.is(tok::r_paren))
-        break;
-
-      if (Tok.isNot(tok::identifier)) {
-        diagnose(Tok, diag::attr_warn_unused_result_expected_name);
-        if (Tok.isNot(tok::r_paren))
-          skipUntil(tok::r_paren);
-        invalid = true;
-        break;
-      }
-
-      // Consume the identifier.
-      StringRef name = Tok.getText();
-      SourceLoc nameLoc = consumeToken();
-
-      // Figure out which parameter it is.
-      enum class KnownParameter {
-        Message,
-        MutableVariant
-       };
-
-      Optional<KnownParameter> known
-        = llvm::StringSwitch<Optional<KnownParameter>>(name)
-            .Case("message", KnownParameter::Message)
-            .Case("mutable_variant", KnownParameter::MutableVariant)
-            .Default(None);
-
-      // If we don't have the ':', try the deprecated '='. If that doesn't
-      // exist neither, complain.
-      if (findAttrValueDelimiter()) {
-        consumeToken();
-      } else {
-        if (known)
-          diagnose(Tok, diag::attr_warn_unused_result_expected_colon, name);
-        else
-          diagnose(Tok, diag::attr_warn_unused_result_unknown_parameter, name);
-        continue;
-      }
-
-      // If we don't have a string, complain.
-      if (Tok.isNot(tok::string_literal)) {
-        diagnose(Tok, diag::attr_warn_unused_result_expected_string, name);
-        if (Tok.isNot(tok::r_paren))
-          skipUntil(tok::r_paren);
-        invalid = true;
-        break;
-      }
-
-      // Dig out the string.
-      auto string = getStringLiteralIfNotInterpolated(*this, nameLoc, Tok,
-                                                      name.str());
-      consumeToken(tok::string_literal);
-      if (!string) {
-        continue;
-      }
-
-      if (!known) {
-        diagnose(nameLoc, diag::attr_warn_unused_result_unknown_parameter,
-                 name);
-        continue;
-      }
-
-      StringRef *whichParam;
-      switch (*known) {
-      case KnownParameter::Message:
-        whichParam = &message;
-        break; 
-
-      case KnownParameter::MutableVariant:
-        whichParam = &mutableVariant;
-        break;
-      }
-
-      if (!whichParam->empty()) {
-        diagnose(nameLoc, diag::attr_warn_unused_result_duplicate_parameter, 
-                 name);
-      }
-
-      *whichParam = *string;
-    } while (consumeIf(tok::comma));
-
-    // Parse the closing ')'.
-    SourceLoc rParenLoc;
-    if (Tok.isNot(tok::r_paren)) {
-      parseMatchingToken(tok::r_paren, rParenLoc,
-                         diag::attr_warn_unused_result_expected_rparen,
-                         lParenLoc);
-    }
-    if (Tok.is(tok::r_paren)) {
-      rParenLoc = consumeToken();
-    }
-
-    Attributes.add(new (Context) WarnUnusedResultAttr(AtLoc, Loc, lParenLoc,
-                                                      message, mutableVariant,
-                                                      rParenLoc, false));
-    break;
-    }
-
   case DAK_Swift3Migration: {
     if (Tok.isNot(tok::l_paren)) {
       diagnose(Loc, diag::attr_expected_lparen, AttrName,
@@ -1460,6 +1347,41 @@ bool Parser::parseDeclAttribute(DeclAttributes &Attributes, SourceLoc AtLoc) {
         .fixItReplace(Tok.getLoc(), "available");
   }
 
+
+  if (DK == DAK_Count && Tok.getText() == "warn_unused_result") {
+    // The behavior created by @warn_unused_result is now the default. Emit a
+    // Fix-It to remove.
+    SourceLoc attrLoc = consumeToken();
+
+    // @warn_unused_result with no arguments.
+    if (Tok.isNot(tok::l_paren)) {
+      diagnose(AtLoc, diag::attr_warn_unused_result_removed)
+        .fixItRemove(SourceRange(AtLoc, attrLoc));
+
+      return true;
+    }
+
+    // @warn_unused_result with arguments.
+    SourceLoc lParenLoc = consumeToken();
+    skipUntil(tok::r_paren);
+
+    // Parse the closing ')'.
+    SourceLoc rParenLoc;
+    if (Tok.isNot(tok::r_paren)) {
+      parseMatchingToken(tok::r_paren, rParenLoc,
+                         diag::attr_warn_unused_result_expected_rparen,
+                         lParenLoc);
+    }
+    if (Tok.is(tok::r_paren)) {
+      rParenLoc = consumeToken();
+    }
+
+    diagnose(AtLoc, diag::attr_warn_unused_result_removed)
+      .fixItRemove(SourceRange(AtLoc, rParenLoc));
+
+    return true;
+  }
+
   if (DK != DAK_Count && !DeclAttribute::shouldBeRejectedByParser(DK))
     return parseNewDeclAttribute(Attributes, AtLoc, DK);
 

lib/Sema/TypeCheckAttr.cpp

@@ -80,7 +80,6 @@ class AttributeEarlyChecker : public AttributeVisitor<AttributeEarlyChecker> {
   IGNORED_ATTR(UIApplicationMain)
   IGNORED_ATTR(UnsafeNoObjCTaggedPointer)
   IGNORED_ATTR(Versioned)
-  IGNORED_ATTR(WarnUnusedResult)
   IGNORED_ATTR(ShowInInterface)
   IGNORED_ATTR(DiscardableResult)
 #undef IGNORED_ATTR
@@ -692,7 +691,6 @@ class AttributeChecker : public AttributeVisitor<AttributeChecker> {
   void visitPostfixAttr(PostfixAttr *attr) { checkOperatorAttribute(attr); }
   void visitPrefixAttr(PrefixAttr *attr) { checkOperatorAttribute(attr); }
 
-  void visitWarnUnusedResultAttr(WarnUnusedResultAttr *attr);
   void visitSpecializeAttr(SpecializeAttr *attr);
 };
 } // end anonymous namespace
@@ -1344,43 +1342,6 @@ AttributeChecker::visitSetterAccessibilityAttr(SetterAccessibilityAttr *attr) {
   visitAbstractAccessibilityAttr(attr);
 }
 
-void AttributeChecker::visitWarnUnusedResultAttr(WarnUnusedResultAttr *attr) {
-  if (!attr->getMutableVariant().empty()) {
-    // mutable_variant only makes sense for non-mutating methods where
-    // it's possible to have a mutating variant.
-    auto func = dyn_cast<FuncDecl>(D);
-    if (!func) {
-      TC.diagnose(attr->getLocation(),
-                  diag::attr_warn_unused_result_mutable_variable, 0);
-      return;
-    }
-
-    if (!func->getDeclContext()->isTypeContext()) {
-      TC.diagnose(attr->getLocation(),
-                  diag::attr_warn_unused_result_mutable_variable, 1);
-      return;
-    }
-
-    if (!func->isInstanceMember()) {
-      TC.diagnose(attr->getLocation(),
-                  diag::attr_warn_unused_result_mutable_variable, 2);
-      return;
-    }
-
-    if (func->getExtensionType()->getClassOrBoundGenericClass()) {
-      TC.diagnose(attr->getLocation(),
-                  diag::attr_warn_unused_result_mutable_variable, 3);
-      return;      
-    }
-
-    if (func->isMutating()) {
-      TC.diagnose(attr->getLocation(),
-                  diag::attr_warn_unused_result_mutable_variable, 4);
-      return;      
-    }
-  }
-}
-
 /// Check that the @_specialize type list has the correct number of entries.
 /// Resolve each type in the list to a concrete type.
 /// Create a Substitution list mapping each nested archetype to a concrete

lib/Sema/TypeCheckDecl.cpp

@@ -5376,7 +5376,6 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
     UNINTERESTING_ATTR(SILStored)
     UNINTERESTING_ATTR(Testable)
 
-    UNINTERESTING_ATTR(WarnUnusedResult)
     UNINTERESTING_ATTR(WarnUnqualifiedAccess)
     UNINTERESTING_ATTR(DiscardableResult)