[clang-tools-extra] [clang-tidy] Unsafe CRTP check (PR #82403)

[via cfe-commits]

================
@@ -0,0 +1,62 @@
+.. title:: clang-tidy - bugprone-unsafe-crtp
+
+bugprone-unsafe-crtp
+====================
+
+Finds CRTP used in an error-prone way.
+
+If the constructor of a class intended to be used in a CRTP is public, then
+it allows users to construct that class on its own.
+
+Example:
+
+.. code-block:: c++
+
+  template <typename T> class CRTP {
+  public:
+    CRTP() = default;
+  };
+
+  class Good : CRTP<Good> {};
+  Good GoodInstance;
+
+  CRTP<int> BadInstance;
+
+If the constructor is protected, the possibility of an accidental instantiation
+is prevented, however it can fade an error, when a different class is used as
+the template parameter instead of the derived one.
+
+Example:
+
+.. code-block:: c++
+
+  template <typename T> class CRTP {
+  protected:
+    CRTP() = default;
+  };
+
+  class Good : CRTP<Good> {};
+  Good GoodInstance;
+
+  class Bad : CRTP<Good> {};
+  Bad BadInstance;
+
+To ensure that no accidental instantiation happens, the best practice is to make
+the constructor private and declare the derived class as friend.
+
+Example:
+
+.. code-block:: c++
+
+  template <typename T> class CRTP {
+    CRTP() = default;
+    friend T;
+  };
+
+  class Good : CRTP<Good> {};
+  Good GoodInstance;
+
+  class Bad : CRTP<Good> {};
+  Bad CompileTimeError;
+
+  CRTP<int> AlsoCompileTimeError;
----------------
isuckatcs wrote:

If the constructor is protected as in your example, the following will also compile, which is not desired.
```c++
struct Decl {};

struct Stmt : ASTNode<Decl> {
  protected:
    Stmt() : ASTNode() {  }
};
```
This is exactly the kind of usage, this check wants to catch. 

Also note that with the suggested fix, your example still compiles.
```c++
template <class NodeFamily>
struct ASTNode {
  private:
    ASTNode() {  }
    friend NodeFamily;
};

struct Stmt : ASTNode<Stmt> {
  protected:
    Stmt() : ASTNode() {  }
};

struct IfStmt : Stmt {
  IfStmt() : Stmt() {  }
};
```

`IfStmt` calls the constructor of `Stmt`, and not the constructor of `ASTNode<>`. The latter constructor is called by `Stmt`, which has access to it either way.

For the record, AFAIK the CRTP is supposed to describe `has a` relationships and not `is a` relationships, so making ASTNode a CRTP might not be an ideal design choice. 

https://github.com/llvm/llvm-project/pull/82403