[clang-tidy] Add check for use of types/classes/functions from <functional> header which are deprecated and removed in C++17

[massberg]

Summary

In C++17 several classes, types, and functions from the <functional> header are no longer available.
In this change we add first clang-tidy checks for occurrences of

• std::unary_function
• std::binary_function
• std::ptr_fun
• std::mem_fun

As there are several more deprecated types in <functional>, this check will be extended in the future.

[alexfh]

Thank you for this new check! A few minor comments (mostly style issues).

clang-tidy/modernize/AvoidFunctionalCheck.cpp

37 ↗

(On Diff #132131)

Please use llvm naming conventions for locals (CamelCase).

38 ↗

(On Diff #132131)

clang:: is not needed here (the code is inside namespace clang).

39 ↗

(On Diff #132131)

const auto *

42 ↗

(On Diff #132131)

const auto *

52 ↗

(On Diff #132131)

Maybe expand a bit more: "deprecated in C++11 and removed in C++17" (or "from C++17", whichever sounds better in English)?

docs/ReleaseNotes.rst

63

".h" is an anachronism, I'd write "from the <functional> header" or something like that. Same in other places.

[massberg]

Implemented all suggestions from the reviewer.

[aaron.ballman]

clang-tidy/modernize/AvoidFunctionalCheck.h

19 ↗

(On Diff #132137)

Missing full stop at the end of the sentence.

Why should this modernize check be limited to <functional>? Just like we have a "deprecated headers" check, perhaps this should be a "deprecated APIs" check?

[aaron.ballman]

clang-tidy/modernize/AvoidFunctionalCheck.cpp

21 ↗

(On Diff #132137)

I don't think this matcher catches other uses of these types. e.g.,

typedef std::binary_function<int, int, bool> awesome_sauce;
struct S : awesome_sauce {};

template <typename Ty, typename = void>
struct S {};

template <typename Ty>          
struct S<Ty, typename std::enable_if<std::is_base_of<std::binary_function<int, int, bool>, Ty>::value>::type> {
  typedef int value;
};

22 ↗

(On Diff #132137)

These should all be ::std:: instead of std:: to cover pathological code that puts std inside of another namespace.

37 ↗

(On Diff #132137)

Formatting (here and elsewhere); you should run the patch through clang-format to handle this sort of thing.

43 ↗

(On Diff #132137)

Should elide the braces here.

47–51 ↗

(On Diff #132137)

Shouldn't this be an assert instead?

docs/clang-tidy/checks/modernize-avoid-functional.rst

6 ↗

(On Diff #132137)

"In C++17, several classes, types, and functions" (commas).

[massberg]

Addressed comments of reviewer.

[massberg]

Alex and Aaron, thanks for the comments!

clang-tidy/modernize/AvoidFunctionalCheck.cpp

21 ↗

(On Diff #132137)

True. This matcher covers most cases in code I have looked into. I can extend this matcher in a follow-up change.

47–51 ↗

(On Diff #132137)

Due to multiple inheritance, there might be several bases and we have to iterate them to find the one we are interested in.
I added an additional test case covering multiple inheritance.

clang-tidy/modernize/AvoidFunctionalCheck.h

19 ↗

(On Diff #132137)

Added full stop.

I'm not sure if this check should be limited to <functional> or be extended to a full 'deprecated API' check.
This change is just a start, several more types and classes which are removed from <functional> will follow, e.g:

• std::ptr_fun, std::mem_fun, std::mem_fun_ref
• std::bind1st, std::bind2nd
• std::unary_function, std::binary_function
• std::pointer_to_unary_function, std::pointer_to_binary_function, std::mem_fun_t, std::mem_fun1_t, std::const_mem_fun_t,
• std::const_mem_fun1_t, std::mem_fun_ref_t, std::mem_fun1_ref_t, std::const_mem_fun_ref_t, std::const_mem_fun1_ref_t
• std::binder1st, std::binder2nd

As these are a bunch of functions and types, in my eyes a check just for <functional> is fine. But I'm also fine with a general 'deprecated API' check.
Alex, can you comment on this?

[massberg]

clang-tidy/modernize/AvoidFunctionalCheck.h

19 ↗

(On Diff #132137)

There are already other checks for functions which are removed in C++17 like modernize-replace-random-shuffle.
So I think having an separate check for functions and types removed from <functional> would be OK.

[aaron.ballman]

clang-tidy/modernize/AvoidFunctionalCheck.h

19 ↗

(On Diff #132137)

You've hit the nail on the head for what I'm trying to avoid -- we shouldn't have multiple checks unless they do drastically different things. Having a deprecated check like this really only makes sense for APIs that are deprecated but aren't uniformly marked as [[deprecated]] by the library. As such, I think we really only need one check for this rather than splitting it out over multiple checks -- the existing check functionality could be rolled into this one and its check become an alias.

[alexfh]

clang-tidy/modernize/AvoidFunctionalCheck.h

19 ↗

(On Diff #132137)

I'm not sure if this check should be limited to <functional> or be extended to a full 'deprecated API' check.

IIUC, it should be possible to implement fixits at least for some use cases here. My impression was that Jens was at least considering to work on fixits. The other check mentioned here - modernize-replace-random-shuffle already implements fixits. Fixits are specific to the API and some codebases may have better replacement APIs than what the standard suggests, so different users may want to apply different set of the fixes. Given all that, I wouldn't just merge all of the checks dealing with deprecated APIs. Splitting them at least by header seems like a good start, maybe even finer granularity may be needed in some cases.

[alexfh]

clang-tidy/modernize/AvoidFunctionalCheck.h

19 ↗

(On Diff #132137)

TL;DR "they do drastically different things" is the case for this check and modernize-replace-random-shuffle.

[aaron.ballman]

clang-tidy/modernize/AvoidFunctionalCheck.h

19 ↗

(On Diff #132137)

I disagree that they do drastically different things or that fix-its are a problem. Some of these APIs have replacements, others do not. At the end of the day, the basics are the same: the functionality is deprecated and you should consider a replacement. Sometimes we know that replacement up front, other times we don't. I don't think we should make users reach for a per-header file answer to that problem unless it provides them some benefit. I don't see users caring to update <functional> but not other headers.

I can see benefit to splitting the *implementations* of the checks along arbitrary lines, but how we structure the implementation is orthogonal to how we surface the functionality.

[alexfh]

test/clang-tidy/modernize-avoid-functional.cpp

30 ↗

(On Diff #132169)

Template parameters in this message are not useful. binary_function is removed in C++17 regardless of the template parameters used. I played with your patch a bit and came up with a way to remove the template parameters in a not overly hacky way (and also pulled out the common part of the message to avoid duplication):

void AvoidFunctionalCheck::registerMatchers(MatchFinder *Finder) {
  Finder->addMatcher(
      cxxRecordDecl(allOf(isDerivedFrom(classTemplateSpecializationDecl(
                                            hasAnyName("std::binary_function",
                                                       "std::unary_function"))
                                            .bind("base")),
                          anyOf(isClass(), ast_matchers::isStruct()),
                          ast_matchers::isDefinition()))
          .bind("un_or_binary_derived"),
      this);
  Finder->addMatcher(callExpr(callee(functionDecl(hasName("std::ptr_fun"))))
                         .bind("ptr_fun_call"),
                     this);
  Finder->addMatcher(callExpr(callee(functionDecl(hasName("std::mem_fun"))))
                         .bind("mem_fun_call"),
                     this);
}

void AvoidFunctionalCheck::check(const MatchFinder::MatchResult &Result) {
  const StringRef Message = "%0 is deprecated in C++11 and removed in C++17";
  if (const auto *const Decl =
          Result.Nodes.getNodeAs<CXXRecordDecl>("un_or_binary_derived")) {
    const auto *SpecDecl =
        Result.Nodes.getNodeAs<ClassTemplateSpecializationDecl>("base");
    for (CXXBaseSpecifier Base : Decl->bases()) {
      if (SpecDecl == Base.getType()->getAsCXXRecordDecl())
        diag(Base.getLocStart(), Message) << SpecDecl->getSpecializedTemplate();
    }
  } else if (const auto *const Call =
                 Result.Nodes.getNodeAs<CallExpr>("ptr_fun_call")) {
    diag(Call->getLocStart(), Message) << "'std::ptr_fun'";
  } else if (const auto *const Call =
                 Result.Nodes.getNodeAs<CallExpr>("mem_fun_call")) {
    diag(Call->getLocStart(), Message) << "'std::mem_fun'";
  }
}

This could be done differently, of course, depending on which way this check is going to evolutionize (e.g. if you're going to find usages of deprecated entities on a lower level - say, TypeLocs, or if you're going to provide fixits for narrower usage patterns).

[• Quuxplusone]

clang-tidy/modernize/AvoidFunctionalCheck.h

19 ↗

(On Diff #132137)

This sounds like clang-tidy ought to have an umbrella option here, analogous to how -Wformat turns on -Wformat-security, -Wformat-truncation, -Wformat-overflow, etc. (Well, in GCC it doesn't, but that's the general idea.)
So there could be a 'modernize-avoid-deprecated-in-c++11' umbrella option that turns on both 'modernize-replace-random-shuffle' and 'modernize-avoid-functional'; a 'modernize-avoid-removed-in-c++17' umbrella option that turns on those two plus some other options; and so on.
Just a thought. If such a structure is anathema to how clang-tidy does things, then never mind. :)

[Eugene.Zelenko]

docs/ReleaseNotes.rst

60

Please move it to new checks section in alphabetical order.

63

Please enclose <functional> in ``.

docs/clang-tidy/checks/modernize-avoid-functional.rst

6 ↗

(On Diff #132169)

Please make this sentence same as in Release Notes.

10 ↗

(On Diff #132169)

Please enclose names in ``.

[alexfh]

clang-tidy/modernize/AvoidFunctionalCheck.h

19 ↗

(On Diff #132137)

I don't see users caring to update <functional> but not other headers.

This is our use case: we have preferred local alternative for the deprecated APIs from <random>, so the upstream modernize-replace-random-shuffle check is out of question. However this check will be useful.

If the two checks are combined, we'll need to add flags to enable warnings/fixes for each subset of the APIs, which is IMO worse than having two separate checks.

[aaron.ballman]

clang-tidy/modernize/AvoidFunctionalCheck.h

19 ↗

(On Diff #132137)

Why is the upstream check out of the question -- the fix-its aren't useful for your case, but it seems the check would be?

Would you be opposed to the approach @Quuxplusone suggested?

[alexfh]

clang-tidy/modernize/AvoidFunctionalCheck.h

19 ↗

(On Diff #132137)

We have a local check that suggests the more appropriate internal APIs. The upstream check would generate duplicate warnings (and suggest conflicting fixes).

Would you be opposed to the approach @Quuxplusone suggested?

It might be nice, but I'm not sure yet which real problem this will address. Maybe I don't see the problem due to the way I'm using clang-tidy. If the problem is unclear relationships between some checks, some of the benefits could be achieved by using more levels of hierarchy in the check names (e.g. modernize-c++11-deprecated-random-shuffle, modernize-c++11-deprecated-functional, which can be turned on together using modernize-c++11-deprecated-*).

[aaron.ballman]

clang-tidy/modernize/AvoidFunctionalCheck.h

19 ↗

(On Diff #132137)

If the upstream check had the ability to customize the fix-it suggestions, would you still need the local check?

The problem I'd like to see addressed is that some users tend to think in terms of grouping other than header files, like "complain about everything removed in C++xy because I plan to upgrade to that at some point soon." I don't want to have those folks specify a per-header file check because the library deprecations span multiple header files with functionality that may have been deprecated or removed at different times. It seems more user friendly to have the granularity be: all deprecations, all deprecations up to this version of the standard, just deprecations for this grouping, and just this deprecation.

[massberg]

Addressed comments of reviewers.

[massberg]

test/clang-tidy/modernize-avoid-functional.cpp

30 ↗

(On Diff #132169)

Thanks for figuring this out! I updated to your suggested version.

[alexfh]

clang-tidy/modernize/AvoidFunctionalCheck.cpp

22 ↗

(On Diff #132137)

Could you change the names to "::std" again, since my suggested code didn't take this comment into account?

clang-tidy/modernize/AvoidFunctionalCheck.h

19 ↗

(On Diff #132137)

Looking at the code of the two versions of the random-shuffle check, I don't see how we could make the checks configurable enough to accommodate both use cases. So yes, we'd need a local check at least for this reason.

As for providing different facets of functionality (address everything deprecated in C++11/14/17/... as opposed to addressing everything in <random>, <functional>, etc.), they don't have to be in a single check each. Instead, we could use configuration facilities for this. One possibility that comes to mind is to introduce the concept of including configuration files (we have this internally, it's built on top of the FileOptionsProvider). Then we could provide a number of "presets" (e.g. for migration to C++17) that would enable all relevant checks and set appropriate options. Alternatively (or until we have the inclusion implemented) we could provide this kind of a configuration snippet in documentation.

WDYT?

[aaron.ballman]

clang-tidy/modernize/AvoidFunctionalCheck.h

19 ↗

(On Diff #132137)

Looking at the code of the two versions of the random-shuffle check, I don't see how we could make the checks configurable enough to accommodate both use cases. So yes, we'd need a local check at least for this reason.

Good to know -- thank you for sharing your usage experience!

WDYT?

I might be misunderstanding how the user will interact with configuration files to get this behavior, but... why should this require configuration files rather than use what users are already used to in warning groups? My gut feeling is that we should be surfacing features in ways the user is already familiar with, and this seems somewhat novel for warnings compared to the grouping approach. This approach means the user experience isn't always going to be uniform like with warning groups -- different configurations provide different behaviors. Also, there's the increased cognitive expense of users finding out about these configuration files and editing them manually to get the behavior they need.

We already have the notion of check aliases that can be reconfigured as-needed, so what I was envisioning were pre-built "checks" that are simply an alias to other checks, appropriately configured. To the user, it looks and acts just like any other check, including allowing the user to specify "warn me about all the C++17 deprecations except this one".

[alexfh]

LG

clang-tidy/modernize/AvoidFunctionalCheck.h

19 ↗

(On Diff #132137)

I'm happy to continue the discussion, but I'd like to unblock this patch. Whichever mechanism we choose or come up with, it may take some time. But I'd like to be able to use this check sooner.

I hope, this is fine by you.

docs/clang-tidy/checks/modernize-avoid-functional.rst

7 ↗

(On Diff #132592)

nit: Could you remove the leading spaces?

[aaron.ballman]

A few comments were not applied, and I'd like a more descriptive name for the check (especially if we plan to generalize this).

clang-tidy/modernize/AvoidFunctionalCheck.h

19 ↗

(On Diff #132137)

I'm happy to continue the discussion, but I'd like to unblock this patch. Whichever mechanism we choose or come up with, it may take some time. But I'd like to be able to use this check sooner.

I hope, this is fine by you.

Yup, fine by me to unblock this patch.

clang-tidy/modernize/ModernizeTidyModule.cpp

50

I'm not keen on this name -- it suggests the user should avoid functional, which is certainly not the advice we want to give them. Also, it makes no mention of deprecations, which is what the check is all about.

How about: modernize-functional-deprecations and we can use modernize-deprecations as the eventual catch-all umbrella, modernize-<header name>-deprecations for other headers, and modernize-<header name>-deprecations-<api> if we want to add API-level granularity?

docs/ReleaseNotes.rst

101

type, classes and functions -> types, classes, and functions

docs/clang-tidy/checks/modernize-avoid-functional.rst

10 ↗

(On Diff #132169)

This comment was not properly applied -- it was calling for backticks, not a single quote.

6 ↗

(On Diff #132137)

This comment was not applied but is marked as done. Missing the Oxford comma still.

[alexfh]

clang-tidy/modernize/ModernizeTidyModule.cpp

50

I'd suggest to put "deprecations" first: modernize-deprecations-functional. Or maybe modernize-deprecated-functional?

[aaron.ballman]

clang-tidy/modernize/ModernizeTidyModule.cpp

50

That works for me -- I have a slight preference for modernize-deprecated-functional.

[massberg]

Addressed comments and renamed test to modernize-deprecated-functional

[massberg]

Thanks for the comments!
I double-checked that the renaming went well and hope that I haven't missed anything this time ...

[aaron.ballman]

clang-tidy/modernize/DeprecatedFunctionalCheck.cpp

27

You can drop the ast_matchers:: here because of the using declaration above.

Actually, do we need the isClass() and isStruct() check at all? What else would match isDerivedFrom()?

31

::std::ptr_fun

34

::std::mem_fun

Why not ::std::mem_fun_ref? Or the _t variants of these APIs?

49–55

I think that this code should be generalized (same with the matchers) so that you match on hasAnyName() for the function calls and use CallExpr::getCalleeDecl() to get the declaration. e.g.,

if (const auto *Call = Result.Nodes.getNodeAs<CallExpr>("blech")) {
  if (const Decl *Callee = Call->getCalleeDecl())
    diag(Call->getLocStart(), Message) << Calleee;
}

This way you can add more named without having to add extra logic for the diagnostics.

docs/ReleaseNotes.rst

101

Please use backticks for <functional>. Also, should say "and functions from the <functional> header".

docs/clang-tidy/checks/modernize-deprecated-functional.rst

7

Same comments here as above.

[massberg]

Thanks for the comments!

clang-tidy/modernize/DeprecatedFunctionalCheck.cpp

27

Done.

You are right, as we use isDerivedFrom() we do not need isClass() and isStruct() anymore.

34

There are several other types and functions in <functional> which are removed in C++17. I will add them in a follow-up change.
This change just introduces the check with a limited number of cases and helps me to get used to the clang-tidy coding style. ;)

49–55

I generalized the code and the matcher.
When we use

<< cast<NamedDecl>(Callee);

we get the template arguments in the name , e.g. ptr_fun<int, int>, so I chose to use getQualifiedNameAsString.
If there is there a better way to get the function name without template arguments I appreciate any suggestions.

[aaron.ballman]

clang-tidy/modernize/DeprecatedFunctionalCheck.cpp

34

Okay, that's reasonable.

49–55

Nope, in that case, your code is correct. However, we generally provide the template arguments in diagnostics. I see @alexfh was asking for them to be removed as not being useful, but I'm not certain I agree with his rationale. Yes, all instances are deprecated and thus the template arguments don't discern between good and bad cases, but showing the template arguments is also consistent with the other diagnostics we emit. For instance, other "deprecated" diagnostics also emit the template arguments. I'm not certain we should be inconsistent with the way we produce diagnostics, but I'll defer to Alex if he still feels strongly about leaving them off here.

[alexfh]

clang-tidy/modernize/DeprecatedFunctionalCheck.cpp

49–55

Indeed, -Wdeprecated-declarations warnings print template arguments. Moreover, they seem to be issued only on instantiations, see https://godbolt.org/g/W563gw.

But I have a number of concerns with template arguments in the deprecation warnings:

1. The note attached to the warning lies. Consider a warning from the test above: ... <source>:11:1: warning: 'B<int>' is deprecated: bbb [-Wdeprecated-declarations] B<int> e; ^ <source>:7:10: note: 'B<int>' has been explicitly marked deprecated here struct [[deprecated("bbb")]] B {};
   
   But B<int> hasn't been explicitly marked deprecated, only the template definition of B has been. Template arguments are important in the case of the explicit template specialization A<int> in the same example, but not in cases where the template definition was marked deprecated, since template arguments only add noise and no useful information there.
2. Warnings can easily become unreadable: https://godbolt.org/g/AFdznH
3. Certain coding patterns can result in numerous deprecation warnings differing only in template arguments: https://godbolt.org/g/U2JCbG. Clang-tidy can deduplicate warnings, if they have identical text and location, but adding template arguments to the message will prevent deduplication. I've seen a case where thousands of deprecation warnings were generated for a single line in a widely used header.

So yes, I feel strongly about leaving off template arguments in case the whole template was marked deprecated. I think it would be the right thing to do for the -Wdeprecated-declarations diagnostic as well.

[alexfh]

clang-tidy/modernize/DeprecatedFunctionalCheck.cpp

49–55

s/leaving off/leaving out/

[aaron.ballman]

clang-tidy/modernize/DeprecatedFunctionalCheck.cpp

49–55

The note attached to the warning lies.

No it doesn't? The attribute is inherited from the primary template unless your explicit specialization *removes* the attribute. https://godbolt.org/g/ZuXZds

Warnings can easily become unreadable

This is true of all template diagnostics and isn't specific to clang-tidy's treatment of them.

I've seen a case where thousands of deprecation warnings were generated for a single line in a widely used header.

This sounds more worrying, but at the same time, your link behaving by design and doing what I would want it to do. The presence of the deprecated primary template isn't what gets diagnosed, it's the *uses* of the deprecated entity. This is called out explicitly in [dcl.attr.deprecated]p4.

So yes, I feel strongly about leaving off template arguments in case the whole template was marked deprecated. I think it would be the right thing to do for the -Wdeprecated-declarations diagnostic as well.

I would be strongly opposed to that change to -Wdeprecated-declarations.

We may be at an impasse here, but my viewpoint is unchanged -- I think removing the template arguments is inconsistent with other diagnostics. I'll defer to you on this, but I think it's a mistake and definitely do not want to see it used as precedent.

[alexfh]

clang-tidy/modernize/DeprecatedFunctionalCheck.cpp

49–55

Let's try to look at this from a different angle: are there benefits (apart from consistency) of including template arguments to deprecation warnings where the primary template is deprecated rather than a specialization? Could you provide an example of a case where template arguments are making the warning easier to understand or to act upon?

The presence of the deprecated primary template isn't what gets diagnosed, it's the *uses* of the deprecated entity. This is called out explicitly in [dcl.attr.deprecated]p4.

Sure, I'm not proposing to change _where_ the warnings are produced, I just want the warnings to be free of unnecessary information that unnecessarily makes the warning messages different. In the example I provided (https://godbolt.org/g/U2JCbG) the program only refers to the deprecated entity (class Q) once after it's declared (Q<T> in class S : Q<T> {};). IMO knowing the specific value of T doesn't give the user any useful information in this case. This only makes the message less readable and gets in the way of any efforts to deduplicate the warnings. Am I missing something?

[aaron.ballman]

clang-tidy/modernize/DeprecatedFunctionalCheck.cpp

49–55

Let's try to look at this from a different angle: are there benefits (apart from consistency) of including template arguments to deprecation warnings where the primary template is deprecated rather than a specialization? Could you provide an example of a case where template arguments are making the warning easier to understand or to act upon?

My concern is that we elide the template args in *all* cases, not just primary vs specialization. Knowing the template args is quite important in these cases:

// Primary template isn't deprecated.
template<typename T>
struct A {};

// Specialization is deprecated.
template<>
struct [[deprecated("aaa")]] A<int> {};

// Primary template is deprecated.
template<typename T>
struct [[deprecated("bbb")]] B {};

// Specialization is not deprecated.
template<>
struct B<int> {};

However, I agree that in the case where the primary template is deprecated and no specializations differ, the template args don't help all that much.

Also, I don't think we should be so quick to write off consistency. I've seen projects parse compiler output before; consistency turns out to be important in weird ways. ;-)

Am I missing something?

I think our definition of "unnecessary" is what differs. I consider the template arguments of an instantiation to be necessary as they are part of the type definition. Some types in a template set may be deprecated while others may not be -- losing the template arguments in *all* cases means important information is not conveyed to the user.

If we decide we want to change the way we output template diagnostics in the presence of *no* specializations, that's a different discussion. However, the code (as it is) is stripping the template arguments in all cases.

[alexfh]

clang-tidy/modernize/DeprecatedFunctionalCheck.cpp

49–55

My concern is that we elide the template args in *all* cases, not just primary vs specialization.
...
However, I agree that in the case where the primary template is deprecated and no specializations differ, the template args don't help all that much.

IIUC, this is the case for all types this check deals with. If it ever touches template types that are only deprecated for some sets of template arguments, we should make sure it outputs template arguments, since they become important.

Also, I don't think we should be so quick to write off consistency. I've seen projects parse compiler output before;

I'm pretty sure removing template arguments in this check won't break any existing projects that parse compiler output ;)

As for consistency with the -Wdeprecated-declarations diagnostic, I could have a look at the feasibility of removing template arguments for the cases where there's no explicit template specialization.

[aaron.ballman]

clang-tidy/modernize/DeprecatedFunctionalCheck.cpp

49–55

IIUC, this is the case for all types this check deals with. If it ever touches template types that are only deprecated for some sets of template arguments, we should make sure it outputs template arguments, since they become important.

That's why I was pushing for this to be a diagnostics engine-level decision -- then we don't have to "remember" to add functionality back in sometime in the future and all checks (including clang) behave consistently without extra intervention.

I'm pretty sure removing template arguments in this check won't break any existing projects that parse compiler output ;)

I'm not as confident as you on this point. ;-) However, I don't think that use case should be an overly strong consideration here, either.

[alexfh]

clang-tidy/modernize/DeprecatedFunctionalCheck.cpp

49–55

That's why I was pushing for this to be a diagnostics engine-level decision

Diagnostic engine doesn't have enough information on how to distinguish cases where the template parameters in the message are useful and where they aren't. Specifically, for the deprecation warnings this depends on what is explicitly marked with the [[deprecated]] attribute (primary template vs. explicit template instantiation - if this even makes sense to do at all). For other warnings the logic will be different. For this check there don't seem to be any cases where the template arguments would be useful in the message (and I would be surprised, if one of the future C++ standards declared only certain instantiations of template classes in STL deprecated, but it it does, we can reconsider this decision).

[aaron.ballman]

clang-tidy/modernize/DeprecatedFunctionalCheck.cpp

49–55

Diagnostic engine doesn't have enough information on how to distinguish cases where the template parameters in the message are useful and where they aren't. Specifically, for the deprecation warnings this depends on what is explicitly marked with the [[deprecated]] attribute (primary template vs. explicit template instantiation - if this even makes sense to do at all).

By "explicit template instantiation", do you mean explicit specialization? If so, that definitely makes sense (it's even called out in the standard).

For other warnings the logic will be different. For this check there don't seem to be any cases where the template arguments would be useful in the message (and I would be surprised, if one of the future C++ standards declared only certain instantiations of template classes in STL deprecated, but it it does, we can reconsider this decision).

std::vector<bool> immediately comes to mind where it's reasonable to want to deprecate the specialization and not the primary template (and the C++ committee might someday decide to do exactly that).

[alexfh]

clang-tidy/modernize/DeprecatedFunctionalCheck.cpp

49–55

By "explicit template instantiation", do you mean explicit specialization?

Yes, I meant explicit specialization.

If so, that definitely makes sense (it's even called out in the standard).

Could you clarify? What does the standard say about this?

std::vector<bool> immediately comes to mind where it's reasonable to want to deprecate the specialization and not the primary template (and the C++ committee might someday decide to do exactly that).

Thanks for the example. vector<bool> is probably the most likely candidate I know so far, though I still find this not extremely likely to happen. And I still don't see how we could accommodate this use case in a generic enough way (in the diagnostic engine, for example). Even this check and the -Wdeprecated-declarations would have to take the decision (of whether the template arguments should be displayed) differently. The diagnostic can rely on the [[deprecated]] attribute, while this check would have to do this differently to be able to support language standard(s) older than the one where the deprecation hypothetically took place.

[aaron.ballman]

clang-tidy/modernize/DeprecatedFunctionalCheck.cpp

49–55

By "explicit template instantiation", do you mean explicit specialization?

Yes, I meant explicit specialization.

Thanks; I kind of thought that, but didn't want to assume for too long. :-)

If so, that definitely makes sense (it's even called out in the standard).

Could you clarify? What does the standard say about this?

I was responding to the comment "if this even makes sense to do at all" with a rationale as to why I think it makes sense:

[dcl.attr.deprecated]p2: "The attribute may be applied to the declaration of a class, a typedef-name, a variable, a non-static data member, a function, a namespace, an enumeration, an enumerator, or a template specialization."

The key bit is that it may be applied to a template specialization and differently than the primary template.

Thanks for the example. vector<bool> is probably the most likely candidate I know so far, though I still find this not extremely likely to happen.

I'm less worried about what WG21 does there (though I do think we may someday get to the point where we can deprecated vector<bool>) and more worried about people who want to flag it *locally*, but I see now that there's no configuration options yet, so perhaps that's moot.

And I still don't see how we could accommodate this use case in a generic enough way (in the diagnostic engine, for example).

I was thinking of using different qualifiers on the diagnostic placeholder, kind of like what we do with %q to force it to always print a qualified name. I was thinking we could use a separate modifier to say "this particular check allows you to do some other heuristic for printing template argument lists", such as only providing the template arguments in the presence of explicit specializations.

If we plan to never allow this to be customizable, then I suppose we can address the template args issue when it arises with standards-based classes. However, I remain skeptical that this is a good precedent to set.

clang-tidy/modernize/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyModernizeModule
   AvoidBindCheck.cpp
+  DeprecatedFunctionalCheck.cpp
   DeprecatedHeadersCheck.cpp
   LoopConvertCheck.cpp
   LoopConvertUtils.cpp
   MakeSmartPtrCheck.cpp
   MakeSharedCheck.cpp
   MakeUniqueCheck.cpp
   ModernizeTidyModule.cpp
   PassByValueCheck.cpp

clang-tidy/modernize/DeprecatedFunctionalCheck.h

+//===--- DeprecatedFunctionalCheck.h - clang-tidy---------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_AVOIDFUNCTIONALCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_AVOIDFUNCTIONALCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+/// Check for several deprecated types and classes from <functional> header.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-deprecated-functional.html
+class DeprecatedFunctionalCheck : public ClangTidyCheck {
+public:
+  DeprecatedFunctionalCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_AVOIDFUNCTIONALCHECK_H

clang-tidy/modernize/DeprecatedFunctionalCheck.cpp

+//===--- DeprecatedFunctionalCheck.cpp - clang-tidy------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "DeprecatedFunctionalCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+void DeprecatedFunctionalCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      cxxRecordDecl(allOf(isDerivedFrom(classTemplateSpecializationDecl(
+                                            hasAnyName("::std::binary_function",
+                                                       "::std::unary_function"))
+                                            .bind("base")),
+                          isDefinition()))
+          .bind("un_or_binary_derived"),
+      this);
+  Finder->addMatcher(callExpr(callee(functionDecl(hasAnyName("::std::ptr_fun",
+                                                             "::std::mem_fun"))
+                                         .bind("fun_decl")))
+                         .bind("fun_call"),
+                     this);
+}
+
+void DeprecatedFunctionalCheck::check(const MatchFinder::MatchResult &Result) {
+  const StringRef Message = "%0 is deprecated in C++11 and removed in C++17";
+  if (const auto *Decl =
+          Result.Nodes.getNodeAs<CXXRecordDecl>("un_or_binary_derived")) {
+    const auto *SpecDecl =
+        Result.Nodes.getNodeAs<ClassTemplateSpecializationDecl>("base");
+    for (CXXBaseSpecifier Base : Decl->bases()) {
+      if (SpecDecl == Base.getType()->getAsCXXRecordDecl())
+        diag(Base.getLocStart(), Message) << SpecDecl->getSpecializedTemplate();
+    }
+  } else if (const auto *Call = Result.Nodes.getNodeAs<CallExpr>("fun_call")) {
+    if (const auto *FuncDecl = Result.Nodes.getNodeAs<FunctionDecl>("fun_decl"))
+      diag(Call->getLocStart(), Message)
+          << "'" + FuncDecl->getQualifiedNameAsString() + "'";
+  }
+}
+
+}  // namespace modernize
+}  // namespace tidy
+}  // namespace clang

clang-tidy/modernize/ModernizeTidyModule.cpp

 //===--- ModernizeTidyModule.cpp - clang-tidy -----------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "AvoidBindCheck.h"
+#include "DeprecatedFunctionalCheck.h"
 #include "DeprecatedHeadersCheck.h"
 #include "LoopConvertCheck.h"
 #include "MakeSharedCheck.h"
 #include "MakeUniqueCheck.h"
 #include "PassByValueCheck.h"
 #include "RawStringLiteralCheck.h"
 #include "RedundantVoidArgCheck.h"
 #include "ReplaceAutoPtrCheck.h"

 namespace clang {
 namespace tidy {
 namespace modernize {
 
 class ModernizeModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<AvoidBindCheck>("modernize-avoid-bind");
+    CheckFactories.registerCheck<DeprecatedFunctionalCheck>(
+        "modernize-deprecated-functional");
     CheckFactories.registerCheck<DeprecatedHeadersCheck>(
         "modernize-deprecated-headers");
     CheckFactories.registerCheck<LoopConvertCheck>("modernize-loop-convert");
     CheckFactories.registerCheck<MakeSharedCheck>("modernize-make-shared");
     CheckFactories.registerCheck<MakeUniqueCheck>("modernize-make-unique");
     CheckFactories.registerCheck<PassByValueCheck>("modernize-pass-by-value");
     CheckFactories.registerCheck<RawStringLiteralCheck>(
         "modernize-raw-string-literal");

docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
 - The 'misc-incorrect-roundings' check was renamed to `bugprone-incorrect-roundings
   <http://clang.llvm.org/extra/clang-tidy/checks/bugprone-incorrect-roundings.html>`_
 
 - The 'misc-string-compare' check was renamed to `readability-string-compare
   <http://clang.llvm.org/extra/clang-tidy/checks/readability-string-compare.html>`_
 
 - New `cppcoreguidelines-avoid-goto
   <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-avoid-goto.html>`_ check
 
   The usage of ``goto`` for control flow is error prone and should be replaced
   with looping constructs. Every backward jump is rejected. Forward jumps are
   only allowed in nested loops.
- New `fuchsia-trailing-return
   return types.
 
 - New `readability-simd-intrinsics
   <http://clang.llvm.org/extra/clang-tidy/checks/readability-simd-intrinsics.html>`_ check
 
   Warns if SIMD intrinsics are used which can be replaced by
   ``std::experimental::simd`` operations.
     
+- New `modernize-deprecated-functional
+  <http://clang.llvm.org/extra/clang-tidy/checks/modernize-deprecated-functional.html>`_ check
+
+  Warns if types, classes, and functions from the `<functional>` header which
+  are deprecated in C++11 and removed in C++17 are used.
+
 - New alias `hicpp-avoid-goto
   <http://clang.llvm.org/extra/clang-tidy/checks/hicpp-avoid-goto.html>`_ to 
   `cppcoreguidelines-avoid-goto <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-avoid-goto.html>`_
   added.
 
 - New `bugprone-throw-keyword-missing
   <http://clang.llvm.org/extra/clang-tidy/checks/bugprone-throw-keyword-missing.html>`_ check
 

docs/clang-tidy/checks/list.rst

.. toctree::
    misc-unconventional-assign-operator
    misc-undelegated-constructor
    misc-uniqueptr-reset-release
    misc-unused-alias-decls
    misc-unused-parameters
    misc-unused-raii
    misc-unused-using-decls
    modernize-avoid-bind
+   modernize-deprecated-functional
    modernize-deprecated-headers
    modernize-loop-convert
    modernize-make-shared
    modernize-make-unique
    modernize-pass-by-value
    modernize-raw-string-literal
    modernize-redundant-void-arg
    modernize-replace-auto-ptr

docs/clang-tidy/checks/modernize-deprecated-functional.rst

+.. title:: clang-tidy - modernize-deprecated-functional
+
+modernize-deprecated-functional
+===============================
+
+Warns if types, classes, and functions from the `<functional>` header which are deprecated in C++11 and removed in C++17 are used.
+In particular, this check warns if one of the following deprecated objects is
+used:
+
+-  `std::unary_function`
+-  `std::binary_function`
+-  `std::ptr_fun`
+-  `std::mem_fun`

test/clang-tidy/modernize-deprecated-functional.cpp

+// RUN: %check_clang_tidy %s modernize-deprecated-functional %t
+
+namespace std {
+
+template <class FirstArg, class SecondArg, class ReturnValue>
+class binary_function {};
+
+template <class FirstArg, class ReturnValue>
+class unary_function {};
+
+template <class Arg, class Result>
+class pointer_to_unary_function
+    : public std::unary_function<Arg, Result> {  // NOLINT
+};
+
+template <class Arg, class Result>
+pointer_to_unary_function<Arg, Result> ptr_fun(Result (*f)(Arg)) {  // NOLINT
+  pointer_to_unary_function<Arg, Result> Nothing;
+  return Nothing;
+}
+
+template <class Res, class T>
+class mem_fun_t {};
+
+template <class Res, class T>
+mem_fun_t<Res, T> mem_fun(Res (T::*f)()) {}  // NOLINT
+
+}  // namespace std
+
+// CHECK-MESSAGES: [[@LINE+1]]:25: warning: 'binary_function' is deprecated in C++11 and removed in C++17 [modernize-deprecated-functional]
+class BinaryTestClass : public std::binary_function<int, int, bool> {
+ public:
+  BinaryTestClass();
+};
+
+// CHECK-MESSAGES: [[@LINE+1]]:24: warning: 'unary_function' is deprecated in C++11 and removed in C++17 [modernize-deprecated-functional]
+class UnaryTestClass : public std::unary_function<int, bool> {
+ public:
+  UnaryTestClass();
+};
+
+class TestClass {
+ public:
+  int test() { return 1; }
+};
+
+// CHECK-MESSAGES: [[@LINE+2]]:31: warning: 'unary_function' is deprecated in C++11 and removed in C++17 [modernize-deprecated-functional]
+class AnotherUnaryTestClass : public TestClass,
+                              public std::unary_function<int, bool> {
+ public:
+  AnotherUnaryTestClass();
+};
+
+
+int simpleFunc(int X) { return X + 1; };
+
+void foo(void) {
+// CHECK-MESSAGES: [[@LINE+1]]:3: warning: 'std::ptr_fun' is deprecated in C++11 and removed in C++17 [modernize-deprecated-functional]
+  std::ptr_fun(simpleFunc);
+
+// CHECK-MESSAGES: [[@LINE+1]]:3: warning: 'std::mem_fun' is deprecated in C++11 and removed in C++17 [modernize-deprecated-functional]
+  std::mem_fun(&TestClass::test);
+}