Add an optional string argument to DeprecatedAttr for Fix-It.

[manmanren]

Summary

This can be used to display Fix-Its.
We only add this to GNU attributes. Since it now has two optional arguments, and the common parsing does not handle "replacement = ", we use custom parsing for DeprecatedAttr.

[manmanren]

Ping

[aaron.ballman]

I like the idea of a potential fixit being provided by the attribute, but I do not agree with the way the feature is surfaced in this patch.

For the GNU-style attribute, the named argument functionality is sufficiently novel syntax that I would strongly like to avoid it, especially since attribute((deprecated)) is supported by multiple compilers.

We should not be adding this functionality to the C++-style attribute in the gnu namespace; that's not our vendor namespace to modify. I wouldn't be opposed to adding a clang namespace where we support this feature, however.

We definitely need to make sure we are not modifying the functionality for the C++ standards-based attribute, or the __declspec based attribute (for pretty printing, as well as parsing, etc).

So this change is missing a lot of tests to make sure we do not support this feature in places where it doesn't belong.

include/clang/Basic/Attr.td

716 ↗

(On Diff #49768)

Would you mind adding some documentation to the attribute since we're changing it?

lib/Parse/ParseDecl.cpp

1048 ↗

(On Diff #49768)

This also gets called for attributes parsed in the gnu:: namespace, which means that you are modifying the behavior of gnu::deprecated(), which we should not do unless GCC also supports this feature.

1068 ↗

(On Diff #49768)

I assume this was meant to do something useful?

1070 ↗

(On Diff #49768)

This now makes the attribute behave differently than __attribute__((deprecated)) because you can have this odd named-argument behavior. I would strongly prefer this be an actual optional parameter instead of using this approach. e.g., __attribute__((deprecated("", "fixit"))).

lib/Sema/SemaDeclAttr.cpp

5074 ↗

(On Diff #49768)

This check should be moved above the extension warning above -- we only want to diagnose the extension if the attribute is actually applied.

test/SemaCXX/cxx11-attr-print.cpp

16 ↗

(On Diff #49768)

This is a bug; we should not be hijacking the gnu attribute space.

18 ↗

(On Diff #49768)

I would like a test that verifies we don't print an empty fixit literal when using the C++14 [[deprecated]] attribute. Same for __declspec(deprecated).

[manmanren]

Hi Aaron,

Thanks for the review!

I like the idea of a potential fixit being provided by the attribute, but I do not agree with the way the feature is surfaced in this patch.

For the GNU-style attribute, the named argument functionality is sufficiently novel syntax that I would strongly like to avoid it, especially since attribute((deprecated)) is supported by multiple compilers.

I suggested replacement = "" mostly for its explicitness, especially when we already have an optional message argument "".
If a user only want to specify replacement without a message, we can say deprecated(replacement = "xxx")
With 'replacement =`, the user will need to specify an empty message.
But your concern makes sense too. We can support the following:
deprecated("xxx") --> a message
deprecated("xxx, "fixit") --> a message and a fixit

We should not be adding this functionality to the C++-style attribute in the gnu namespace; that's not our vendor namespace to modify.

I believe the patch currently does not change the C++-style attribute. Of course, I should add testing cases to verify.

I wouldn't be opposed to adding a clang namespace where we support this feature, however.

We definitely need to make sure we are not modifying the functionality for the C++ standards-based attribute

Is this ("the C++ standards-based attribute") the same as "the C++-style attribute" you mentioned above?

, or the declspec based attribute (for pretty printing, as well as parsing, etc).
Yes, I should add testing cases to make sure declspec is not changed at all with this patch.

So this change is missing a lot of tests to make sure we do not support this feature in places where it doesn't belong.

include/clang/Basic/Attr.td

716 ↗

(On Diff #49768)

Yes, I should.

lib/Parse/ParseDecl.cpp

1048 ↗

(On Diff #49768)

Yes, I will fix this.

1068 ↗

(On Diff #49768)

Yes, I should emit a diagnostic.

lib/Sema/SemaDeclAttr.cpp

5074 ↗

(On Diff #49768)

Agree.

test/SemaCXX/cxx11-attr-print.cpp

16 ↗

(On Diff #49768)

Yes, I will fix this.

18 ↗

(On Diff #49768)

Will do.

[aaron.ballman]

In D17865#370912, @manmanren wrote:

Hi Aaron,

Thanks for the review!

I like the idea of a potential fixit being provided by the attribute, but I do not agree with the way the feature is surfaced in this patch.

For the GNU-style attribute, the named argument functionality is sufficiently novel syntax that I would strongly like to avoid it, especially since attribute((deprecated)) is supported by multiple compilers.

I suggested replacement = "" mostly for its explicitness, especially when we already have an optional message argument "".
If a user only want to specify replacement without a message, we can say deprecated(replacement = "xxx")
With 'replacement =`, the user will need to specify an empty message.

I do like the explicit nature of this approach, but I'm worried about it being too novel. For instance, would this compel GCC to implement a similar parsing feature since they also support the deprecated attribute, that sort of thing. Also, it feels like a bit of an odd design in C and C++ since nothing else in the language supports argument passing by name like that (and I would really hate to see us use = blah only to find out that C++ someday standardizes on something different).

But your concern makes sense too. We can support the following:
deprecated("xxx") --> a message
deprecated("xxx, "fixit") --> a message and a fixit

I think this is the correct approach, even with multiple optional arguments.

We should not be adding this functionality to the C++-style attribute in the gnu namespace; that's not our vendor namespace to modify.

I believe the patch currently does not change the C++-style attribute. Of course, I should add testing cases to verify.

I believe the patch does change the C++-style attribute in the gnu namespace, as best I can tell. e.g., it would now allow: `[[gnu::deprecated("something", "a fixit")]]`. We need to be careful not to allow this unless GCC allows the same syntax, otherwise we are hijacking their vendor attribute namespace with something incompatible.

I wouldn't be opposed to adding a clang namespace where we support this feature, however.

We definitely need to make sure we are not modifying the functionality for the C++ standards-based attribute

Is this ("the C++ standards-based attribute") the same as "the C++-style attribute" you mentioned above?

This is talking about the C++14 [[deprecated]] attribute. We want to make sure we don't allow [[deprecated("foo", "fixit")]], which I don't think your patch does (but we should have tests ensuring it).

, or the declspec based attribute (for pretty printing, as well as parsing, etc).
Yes, I should add testing cases to make sure declspec is not changed at all with this patch.

So this change is missing a lot of tests to make sure we do not support this feature in places where it doesn't belong.

[bob.wilson]

I do like the explicit nature of this approach, but I'm worried about it being too novel. For instance, would this compel GCC to implement a similar parsing feature since they also support the deprecated attribute, that sort of thing. Also, it feels like a bit of an odd design in C and C++ since nothing else in the language supports argument passing by name like that (and I would really hate to see us use = blah only to find out that C++ someday standardizes on something different).

This makes sense to me. We also want to add this to the "availability" attribute, and since that already uses named arguments, I think we should use the "replacement=" syntax there.

[manmanren]

Uploading a new patch addressing Aaron's comments:
1> Add an optional string argument for fix-it, instead of the original proposal of named argument Replacement = "xxx"
This actually simplifies the patch a lot
2> Add testing cases to make sure we don't accept a fix-it argument for declspec, c++11 attribute, gnu namespace attribute
3> Change AST printing to make sure we only dump the fix-it argument for gnu style attribute

Cheers,
Manman

[manmanren]

utils/TableGen/ClangAttrEmitter.cpp

1234 ↗

(On Diff #50374)

This does not look pretty. Maybe we can implement a function writeDeprecatedValue that can skip the second argument if it is empty?

[aaron.ballman]

include/clang/Basic/Attr.td

726 ↗

(On Diff #50374)

The formatting here is a bit strange.

lib/Sema/SemaDeclAttr.cpp

5143 ↗

(On Diff #50374)

This should move back down below the extension warning (the effect is the same, but logically we want to warn before attaching).

utils/TableGen/ClangAttrEmitter.cpp

1234 ↗

(On Diff #50374)

I wouldn't be opposed to that approach; we already do it for writeAvailabilityValue(), so another one for deprecated wouldn't be that horrible.

[manmanren]

Upload patch to address review comments.

Thanks,
Manman

[aaron.ballman]

Aside from the missing attribute documentation, looks good to me.

[manmanren]

In D17865#374659, @aaron.ballman wrote:

Aside from the missing attribute documentation, looks good to me.

Hi Aaron,

Which document are you referring to? "include/clang/Basic/AttrDocs.td"?
We don't have a section for DeprecatedAttr, should I start one?

Cheers,
Manman

[aaron.ballman]

In D17865#374939, @manmanren wrote:

In D17865#374659, @aaron.ballman wrote:

Aside from the missing attribute documentation, looks good to me.

Hi Aaron,

Which document are you referring to? "include/clang/Basic/AttrDocs.td"?

Yes, AttrDocs.td that you then reference in to Attr.td.

We don't have a section for DeprecatedAttr, should I start one?

Yes, please! We used to get away with relying on GCC to document the attribute, so now that we're adding Clang-specific behavior, we should bite the bullet and document the attribute properly. :-)

Thanks!

[aaron.ballman]

One small documentation nit, but I'm fine with you fixing it then committing (no need for another round of review). Thanks!

include/clang/Basic/AttrDocs.td

2189 ↗

(On Diff #50682)

"When spelled as `__attribute__((deprecated))`, the deprecated attribute can..."

"Otherwise, when spelled as `[[gnu::deprecated]] or [[deprecated]]`, the attribute can have one optional string argument which is the message to display when emitting the warning."

cfe/trunk/include/clang/Basic/Attr.td

 def OpenCLGenericAddressSpace : TypeAttr {
   let Spellings = [Keyword<"__generic">, Keyword<"generic">];
   let Documentation = [OpenCLAddressSpaceGenericDocs];
 }
 
 def Deprecated : InheritableAttr {
   let Spellings = [GCC<"deprecated">, Declspec<"deprecated">,
                    CXX11<"","deprecated", 201309>];
-  let Args = [StringArgument<"Message", 1>];
-  let Documentation = [Undocumented];
+  let Args = [StringArgument<"Message", 1>,
+              // An optional string argument that enables us to provide a
+              // Fix-It.
+              StringArgument<"Replacement", 1>];
+  let Documentation = [DeprecatedDocs];
 }
 
 def Destructor : InheritableAttr {
   let Spellings = [GCC<"destructor">];
   let Args = [DefaultIntArgument<"Priority", 65535>];
   let Subjects = SubjectList<[Function]>;
   let Documentation = [Undocumented];
 }

cfe/trunk/include/clang/Basic/AttrDocs.td

 that don't need to call out to any other functions.
 
 This calling convention, like the ``preserve_most`` calling convention, will be
 used by a future version of the Objective-C runtime and should be considered
 experimental at this time.
   }];
 }
 
+def DeprecatedDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
+The ``deprecated`` attribute can be applied to a function, a variable, or a
+type. This is useful when identifying functions, variables, or types that are
+expected to be removed in a future version of a program.
+
+Consider the function declaration for a hypothetical function ``f``:
+
+.. code-block:: c++
+
+  void f(void) __attribute__((deprecated("message", "replacement")));
+
+When spelled as `__attribute__((deprecated))`, the deprecated attribute can have
+two optional string arguments. The first one is the message to display when
+emitting the warning; the second one enables the compiler to provide a Fix-It
+to replace the deprecated name with a new name. Otherwise, when spelled as
+`[[gnu::deprecated]] or [[deprecated]]`, the attribute can have one optional
+string argument which is the message to display when emitting the warning.
+  }];
+}

cfe/trunk/lib/Lex/PPMacroExpansion.cpp

return llvm::StringSwitch<bool>(Feature)
       .Case("attribute_availability_tvos", true)
       .Case("attribute_availability_watchos", true)
       .Case("attribute_availability_with_strict", true)
       .Case("attribute_availability_in_templates", true)
       .Case("attribute_cf_returns_not_retained", true)
       .Case("attribute_cf_returns_retained", true)
       .Case("attribute_cf_returns_on_parameters", true)
       .Case("attribute_deprecated_with_message", true)
+      .Case("attribute_deprecated_with_replacement", true)
       .Case("attribute_ext_vector_type", true)
       .Case("attribute_ns_returns_not_retained", true)
       .Case("attribute_ns_returns_retained", true)
       .Case("attribute_ns_consumes_self", true)
       .Case("attribute_ns_consumed", true)
       .Case("attribute_cf_consumed", true)
       .Case("attribute_objc_ivar_unused", true)
       .Case("attribute_objc_method_family", true)

cfe/trunk/lib/Sema/SemaDeclAttr.cpp

if (NSD->isAnonymousNamespace()) {
       S.Diag(Attr.getLoc(), diag::warn_deprecated_anonymous_namespace);
       // Do not want to attach the attribute to the namespace because that will
       // cause confusing diagnostic reports for uses of declarations within the
       // namespace.
       return;
     }
   }
 
+  // Handle the cases where the attribute has a text message.
+  StringRef Str, Replacement;
+  if (Attr.isArgExpr(0) && Attr.getArgAsExpr(0) &&
+      !S.checkStringLiteralArgumentAttr(Attr, 0, Str))
+    return;
+
+  // Only support a single optional message for Declspec and CXX11.
+  if (Attr.isDeclspecAttribute() || Attr.isCXX11Attribute())
+    checkAttributeAtMostNumArgs(S, Attr, 1);
+  else if (Attr.isArgExpr(1) && Attr.getArgAsExpr(1) &&
+           !S.checkStringLiteralArgumentAttr(Attr, 1, Replacement))
+    return;
+
   if (!S.getLangOpts().CPlusPlus14)
     if (Attr.isCXX11Attribute() &&
         !(Attr.hasScope() && Attr.getScopeName()->isStr("gnu")))
       S.Diag(Attr.getLoc(), diag::ext_cxx14_attr) << Attr.getName();
 
-  handleAttrWithMessage<DeprecatedAttr>(S, D, Attr);
+  D->addAttr(::new (S.Context) DeprecatedAttr(Attr.getRange(), S.Context, Str,
+                                   Replacement,
+                                   Attr.getAttributeSpellingListIndex()));
 }
 
 static void handleNoSanitizeAttr(Sema &S, Decl *D, const AttributeList &Attr) {
   if (!checkAttributeAtLeastNumArgs(S, Attr, 1))
     return;
 
   std::vector<StringRef> Sanitizers;
 
case Sema::AD_Partial:
     diag = diag::warn_partial_availability;
     diag_message = diag::warn_partial_message;
     diag_fwdclass_message = diag::warn_partial_fwdclass_message;
     property_note_select = /* partial */ 2;
     available_here_select_kind = /* partial */ 3;
     break;
   }
 
+  CharSourceRange UseRange;
+  StringRef Replacement;
+  if (K == Sema::AD_Deprecation) {
+    if (auto attr = D->getAttr<DeprecatedAttr>())
+      Replacement = attr->getReplacement();
+
+    if (!Replacement.empty())
+      UseRange =
+          CharSourceRange::getCharRange(Loc, S.getLocForEndOfToken(Loc));
+  }
+
   if (!Message.empty()) {
-    S.Diag(Loc, diag_message) << D << Message;
+    S.Diag(Loc, diag_message) << D << Message
+      << (UseRange.isValid() ?
+          FixItHint::CreateReplacement(UseRange, Replacement) : FixItHint());
     if (ObjCProperty)
       S.Diag(ObjCProperty->getLocation(), diag::note_property_attribute)
           << ObjCProperty->getDeclName() << property_note_select;
   } else if (!UnknownObjCClass) {
-    S.Diag(Loc, diag) << D;
+    S.Diag(Loc, diag) << D
+      << (UseRange.isValid() ?
+          FixItHint::CreateReplacement(UseRange, Replacement) : FixItHint());
     if (ObjCProperty)
       S.Diag(ObjCProperty->getLocation(), diag::note_property_attribute)
           << ObjCProperty->getDeclName() << property_note_select;
   } else {
-    S.Diag(Loc, diag_fwdclass_message) << D;
+    S.Diag(Loc, diag_fwdclass_message) << D
+      << (UseRange.isValid() ?
+          FixItHint::CreateReplacement(UseRange, Replacement) : FixItHint());
     S.Diag(UnknownObjCClass->getLocation(), diag::note_forward_class);
   }
 
   S.Diag(D->getLocation(), diag_available_here)
       << D << available_here_select_kind;
   if (K == Sema::AD_Partial)
     S.Diag(Loc, diag::note_partial_availability_silence) << D;
 }

cfe/trunk/test/SemaCXX/attr-deprecated-replacement-error.cpp

+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -verify -fsyntax-only -std=c++11 -fms-extensions %s
+
+#if !__has_feature(attribute_deprecated_with_replacement)
+#error "Missing __has_feature"
+#endif
+
+int a1 [[deprecated("warning", "fixit")]]; // expected-error{{'deprecated' attribute takes no more than 1 argument}}
+int a2 [[deprecated("warning", 1)]]; // expected-error{{'deprecated' attribute takes no more than 1 argument}}
+
+int b1 [[gnu::deprecated("warning", "fixit")]]; // expected-error{{'deprecated' attribute takes no more than 1 argument}}
+int b2 [[gnu::deprecated("warning", 1)]]; // expected-error{{'deprecated' attribute takes no more than 1 argument}}
+
+__declspec(deprecated("warning", "fixit")) int c1; // expected-error{{'deprecated' attribute takes no more than 1 argument}}
+__declspec(deprecated("warning", 1)) int c2; // expected-error{{'deprecated' attribute takes no more than 1 argument}}
+
+int d1 __attribute__((deprecated("warning", "fixit")));
+int d2 __attribute__((deprecated("warning", 1))); // expected-error{{'deprecated' attribute requires a string}}

cfe/trunk/test/SemaCXX/attr-deprecated-replacement-fixit.cpp

+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -verify -fsyntax-only %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: cp %s %t
+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -fixit %t
+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -Werror %t
+
+#if !__has_feature(attribute_deprecated_with_replacement)
+#error "Missing __has_feature"
+#endif
+
+void f_8(int) __attribute__((deprecated("message", "new8"))); // expected-note {{'f_8' has been explicitly marked deprecated here}}
+void new8(int);
+void test() {
+  f_8(0); // expected-warning{{'f_8' is deprecated}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:6}:"new8"
+}

cfe/trunk/test/SemaCXX/cxx11-attr-print.cpp

 int z [[gnu::aligned(4)]];
 
 // CHECK: __attribute__((deprecated("warning")));
 int a __attribute__((deprecated("warning")));
 
 // CHECK: int b {{\[}}[gnu::deprecated("warning")]];
 int b [[gnu::deprecated("warning")]];
 
+// CHECK: __declspec(deprecated("warning"))
+__declspec(deprecated("warning")) int c;
+
+// CHECK: int d {{\[}}[deprecated("warning")]];
+int d [[deprecated("warning")]];
+
+// CHECK: __attribute__((deprecated("warning", "fixit")));
+int e __attribute__((deprecated("warning", "fixit")));
+
 // CHECK: int cxx11_alignas alignas(4);
 alignas(4) int cxx11_alignas;
 
 // CHECK: int c11_alignas _Alignas(alignof(int));
 _Alignas(int) int c11_alignas;
 
 // CHECK: void foo() __attribute__((const));
 void foo() __attribute__((const));

cfe/trunk/utils/TableGen/ClangAttrEmitter.cpp

OS << "\" << getPlatform()->getName();\n"
      << "  if (getStrict()) OS << \", strict\";\n"
      << "  if (!getIntroduced().empty()) OS << \", introduced=\" << getIntroduced();\n"
      << "  if (!getDeprecated().empty()) OS << \", deprecated=\" << getDeprecated();\n"
      << "  if (!getObsoleted().empty()) OS << \", obsoleted=\" << getObsoleted();\n"
      << "  if (getUnavailable()) OS << \", unavailable\";\n"
      << "  OS << \"";
 }
 
+static void writeDeprecatedAttrValue(raw_ostream &OS, std::string &Variety) {
+  OS << "\\\"\" << getMessage() << \"\\\"\";\n";
+  // Only GNU deprecated has an optional fixit argument at the second position.
+  if (Variety == "GNU")
+     OS << "    if (!getReplacement().empty()) OS << \", \\\"\""
+           " << getReplacement() << \"\\\"\";\n";
+  OS << "    OS << \"";
+}
+
 static void writeGetSpellingFunction(Record &R, raw_ostream &OS) {
   std::vector<FlattenedSpelling> Spellings = GetFlattenedSpellings(R);
 
   OS << "const char *" << R.getName() << "Attr::getSpelling() const {\n";
   if (Spellings.empty()) {
     OS << "  return \"(No spelling)\";\n}\n\n";
     return;
   }
for (unsigned I = 0; I < Spellings.size(); ++ I) {
     // attributes which have optional arguments that were not provided. For
     // instance: __attribute__((aligned)) will be pretty printed as
     // __attribute__((aligned())). The logic should check whether there is only
     // a single argument, and if it is optional, whether it has been provided.
     if (hasNonFakeArgs)
       OS << "(";
     if (Spelling == "availability") {
       writeAvailabilityValue(OS);
+    } else if (Spelling == "deprecated" || Spelling == "gnu::deprecated") {
+        writeDeprecatedAttrValue(OS, Variety);
     } else {
       unsigned index = 0;
       for (const auto &arg : Args) {
         if (arg->isFake()) continue;
         if (index++) OS << ", ";
         arg->writeValue(OS);
       }
     }