Adding diagnostic for wrong usage of std::make_[shared/unique]

Given this code:

#include <memory>

struct AImpl {
    AImpl(int, int);

using A = std::shared_ptr<AImpl>;

// .......

int f() {
    A x = std::make_shared<A>(1, 2);

This should have been std::make_shared<AImpl> instead, but it’s pretty easy to accidentally write the wrong version when people are aliasing away std::shared_ptr. The compiler emits 100 lines of dense warnings about all the (13) constructors of shared_ptr instead of a useful error message.

Does Clang have similar diagnostics about wrong usage of the STL? I’m not sure how to implement something like this and whether Clang or the stdlib is more appropriate.

The very first diagnostic seems relatively helpful.

error: no viable conversion from 'shared_ptr<std::shared_ptr<AImpl>>' to 'shared_ptr<AImpl>'

What error message would you ideally want clang to provide?

Nit: I think there’s a typo in the snippet.

struct AImpl {
    AImpl(int, int);
// ...

I don’t think it’s helpful enough - it’s buried inside 100 lines of diagnostics and if AImpl would be some kind of a template type with long enough arguments you won’t necessarily pay attention to the exact error.

Ideally I would want clang to just suggest the fix when it would be legal.

(Thanks for spotting the typo!)

I do agree that the diagnostic can be better. (Although that’s possibly true for all C++ compilers.)

There’s a FixIt feature in diagnostics - it sounds like you’d like it to support this particular case.

You can find its command line options here:
Related tests:

The canonical problem is - how to make a good enough guess (in clang, based on the AST) that it would be almost never incorrect?

I can imagine at least two different ways how to fix the snippet (with different semantical consequences):

A x = std::make_shared<A>(1, 2); -> A x = std::make_shared<AImpl>(1, 2);
A x = std::make_shared<A>(1, 2); -> auto x = std::make_shared<A>(1, 2);

While I am willing to agree that std::make_shared<std::make_shared<T>> is “pretty uncommon” I don’t think it’s nonsensical. And if there are valid uses of this construct then trying to come up with a universal diagnostic for it is tricky.

Well, you can’t fix this as auto x = std::make_shared<A>(1, 2); because it’s no std::shared_ptr constructor receives two integers. So checking that the arguments are valid for a constructor of AImpl shouldn’t have too many false positives I think…