Bug 1662652 - New non-standard move checker. r=andi

Differential Revision: https://phabricator.services.mozilla.com/D60955

build/clang-plugin/alpha/AlphaChecks.inc

@@ -6,4 +6,5 @@
 // to be in alpha stage development.
 
 // CHECK(AlphaChecker, "alpha-checker")
+CHECK(NonStdMoveChecker, "non-std-move")
 CHECK(TempRefPtrChecker, "performance-temp-refptr")

build/clang-plugin/alpha/AlphaIncludes.inc

@@ -1 +1,2 @@
+#include "NonStdMoveChecker.h"
 #include "TempRefPtrChecker.h"

build/clang-plugin/alpha/NonStdMoveChecker.cpp

@@ -0,0 +1,111 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#include "NonStdMoveChecker.h"
+#include "CustomMatchers.h"
+#include "clang/Lex/Lexer.h"
+
+constexpr const char *kConstructExpr = "construct";
+constexpr const char *kOperatorCallExpr = "operator-call";
+constexpr const char *kSourceExpr = "source-expr";
+constexpr const char *kMaterializeExpr = "materialize-expr";
+
+void NonStdMoveChecker::registerMatchers(MatchFinder *AstMatcher) {
+
+  // Assignment through forget
+  AstMatcher->addMatcher(
+      cxxOperatorCallExpr(
+          hasOverloadedOperatorName("="),
+          hasAnyArgument(materializeTemporaryExpr(
+                             has(cxxBindTemporaryExpr(has(cxxMemberCallExpr(
+                                 has(memberExpr(member(hasName("forget")))),
+                                 on(expr().bind(kSourceExpr)))))))
+                             .bind(kMaterializeExpr)))
+          .bind(kOperatorCallExpr),
+      this);
+
+  // Construction through forget
+
+  AstMatcher->addMatcher(
+      cxxConstructExpr(has(materializeTemporaryExpr(
+                               has(cxxBindTemporaryExpr(has(cxxMemberCallExpr(
+                                   has(memberExpr(member(hasName("forget")))),
+                                   on(expr().bind(kSourceExpr)))))))
+                               .bind(kMaterializeExpr)))
+          .bind(kConstructExpr),
+      this);
+}
+
+Optional<FixItHint>
+NonStdMoveChecker::makeFixItHint(const MatchFinder::MatchResult &Result,
+                                 const Expr *const TargetExpr) {
+  const auto *MaterializeExpr = Result.Nodes.getNodeAs<Expr>(kMaterializeExpr);
+
+  // TODO: In principle, we should check here if TargetExpr if
+  // assignable/constructible from std::move(SourceExpr). Not sure how to do
+  // this. Currently, we only filter out the case where the targetTypeTemplate
+  // is already_AddRefed, where this is known to fail.
+
+  const auto *targetTypeTemplate = getNonTemplateSpecializedCXXRecordDecl(
+      TargetExpr->getType().getCanonicalType());
+  const auto *sourceTypeTemplate = getNonTemplateSpecializedCXXRecordDecl(
+      MaterializeExpr->getType().getCanonicalType());
+
+  if (targetTypeTemplate && sourceTypeTemplate) {
+    // TODO is there a better way to check this than by name? otherwise, the
+    // names probably are necessarily unique in the scope
+    if (targetTypeTemplate->getName() == sourceTypeTemplate->getName() &&
+        targetTypeTemplate->getName() == "already_AddRefed") {
+      return {};
+    }
+  }
+
+  const auto *SourceExpr = Result.Nodes.getNodeAs<Expr>(kSourceExpr);
+
+  const auto sourceText = Lexer::getSourceText(
+      CharSourceRange::getTokenRange(SourceExpr->getSourceRange()),
+      Result.Context->getSourceManager(), Result.Context->getLangOpts());
+
+  return FixItHint::CreateReplacement(MaterializeExpr->getSourceRange(),
+                                      ("std::move(" + sourceText + ")").str());
+}
+
+void NonStdMoveChecker::check(const MatchFinder::MatchResult &Result) {
+  // TODO: Include source and target type name in messages.
+
+  const auto *OCE =
+      Result.Nodes.getNodeAs<CXXOperatorCallExpr>(kOperatorCallExpr);
+
+  if (OCE) {
+    const auto *refPtrDecl =
+        dyn_cast<const CXXRecordDecl>(OCE->getCalleeDecl()->getDeclContext());
+
+    const auto XFixItHint = makeFixItHint(Result, OCE);
+    // TODO: produce diagnostic but no FixItHint in this case?
+    if (XFixItHint) {
+      diag(OCE->getBeginLoc(), "non-standard move assignment to %0 obscures "
+                               "move, use std::move instead")
+          << refPtrDecl << *XFixItHint;
+    }
+  }
+
+  const auto *CoE = Result.Nodes.getNodeAs<CXXConstructExpr>(kConstructExpr);
+
+  if (CoE) {
+    const auto *refPtrDecl =
+        dyn_cast<const CXXRecordDecl>(CoE->getConstructor()->getDeclContext());
+
+    const auto XFixItHint = makeFixItHint(Result, CoE);
+    // TODO: produce diagnostic but no FixItHint in this case?
+    if (XFixItHint) {
+      diag(CoE->getBeginLoc(), "non-standard move construction of %0 obscures "
+                               "move, use std::move instead")
+          << refPtrDecl << *XFixItHint;
+    }
+  }
+
+  // TODO: What about swap calls immediately after default-construction? These
+  // can also be replaced by move-construction, but this may require
+  // control-flow analysis.
+}

build/clang-plugin/alpha/NonStdMoveChecker.h

@@ -0,0 +1,24 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#ifndef NonStdMoveChecker_h__
+#define NonStdMoveChecker_h__
+
+#include "plugin.h"
+
+class NonStdMoveChecker final : public BaseCheck {
+public:
+  NonStdMoveChecker(StringRef CheckName, ContextType *Context = nullptr)
+      : BaseCheck(CheckName, Context) {}
+  void registerMatchers(MatchFinder *AstMatcher) override;
+  void check(const MatchFinder::MatchResult &Result) override;
+
+private:
+  CompilerInstance *CI;
+
+  static Optional<FixItHint>
+  makeFixItHint(const MatchFinder::MatchResult &Result, const Expr *TargetExpr);
+};
+
+#endif

build/clang-plugin/alpha/sources.mozbuild

@@ -6,5 +6,6 @@
 
 HOST_SOURCES += [
     # 'AlphaChecker.cpp',
+    "NonStdMoveChecker.cpp",
     'TempRefPtrChecker.cpp',
-]
+]

build/clang-plugin/alpha/tests/TestNonStdMove.cpp

@@ -0,0 +1,125 @@
+#include <mozilla/RefPtr.h>
+
+// we can't include nsCOMPtr.h here, so let's redefine a basic version
+template<typename T>
+struct nsCOMPtr {
+  nsCOMPtr() = default;
+
+  template<typename U>
+  MOZ_IMPLICIT nsCOMPtr(already_AddRefed<U>&& aSrc);
+
+  template<typename U>
+  nsCOMPtr& operator=(already_AddRefed<U>&& aSrc);
+};
+
+
+using namespace mozilla;
+
+struct RefCountedBase {
+  void AddRef();
+  void Release();
+
+  void method_test();
+};
+
+struct RefCountedDerived : RefCountedBase {};
+
+struct RefCountedBaseHolder {
+  RefPtr<RefCountedBase> GetRefCountedBase() const {
+    return mRefCountedBase;
+  }
+
+private:
+  RefPtr<RefCountedBase> mRefCountedBase = MakeRefPtr<RefCountedBase>();
+};
+
+
+void test_assign_same_type() {
+  RefPtr<RefCountedBase> a = MakeRefPtr<RefCountedBase>();
+  RefPtr<RefCountedBase> b;
+
+  b = a.forget(); // expected-warning {{non-standard move assignment}}
+}
+
+void test_assign_implicit_cast() {
+  RefPtr<RefCountedDerived> a = MakeRefPtr<RefCountedDerived>();
+  RefPtr<RefCountedBase> b;
+
+  b = a.forget(); // expected-warning {{non-standard move assignment}}
+}
+
+void test_assign_different_template() {
+  RefPtr<RefCountedDerived> a = MakeRefPtr<RefCountedDerived>();
+  nsCOMPtr<RefCountedBase> b;
+
+  b = a.forget(); // expected-warning {{non-standard move assignment}}
+}
+
+void test_construct_different_template() {
+  RefPtr<RefCountedDerived> a = MakeRefPtr<RefCountedDerived>();
+  nsCOMPtr<RefCountedBase> b = a.forget(); // expected-warning {{non-standard move construction}}
+}
+
+void test_assign_already_addrefed() {
+  RefPtr<RefCountedDerived> a = MakeRefPtr<RefCountedDerived>();
+  already_AddRefed<RefCountedDerived> b;
+
+  b = a.forget();
+}
+
+void test_construct_already_addrefed() {
+  RefPtr<RefCountedDerived> a = MakeRefPtr<RefCountedDerived>();
+  already_AddRefed<RefCountedDerived> b = a.forget();
+}
+
+void test_construct_same_type() {
+  RefPtr<RefCountedBase> a = MakeRefPtr<RefCountedBase>();
+  RefPtr<RefCountedBase> b = a.forget(); // expected-warning {{non-standard move construction}}
+}
+
+void test_construct_implicit_cast() {
+  RefPtr<RefCountedDerived> a = MakeRefPtr<RefCountedDerived>();
+  RefPtr<RefCountedBase> b = a.forget(); // expected-warning {{non-standard move construction}}
+}
+
+void test_construct_brace_same_type() {
+  RefPtr<RefCountedBase> a = MakeRefPtr<RefCountedBase>();
+  auto b = RefPtr<RefCountedBase>{a.forget()}; // expected-warning {{non-standard move construction}}
+}
+
+void test_construct_brace_implicit_cast() {
+  RefPtr<RefCountedDerived> a = MakeRefPtr<RefCountedDerived>();
+  auto b = RefPtr<RefCountedBase>{a.forget()}; // expected-warning {{non-standard move construction}}
+}
+
+void test_construct_function_style_same_type() {
+  RefPtr<RefCountedBase> a = MakeRefPtr<RefCountedBase>();
+  auto b = RefPtr<RefCountedBase>(a.forget()); // expected-warning {{non-standard move construction}}
+}
+
+void test_construct_function_style_implicit_cast() {
+  RefPtr<RefCountedDerived> a = MakeRefPtr<RefCountedDerived>();
+  auto b = RefPtr<RefCountedBase>(a.forget());  // expected-warning {{non-standard move construction}}
+}
+
+void test_construct_result_type() {
+  RefPtr<RefCountedBase> a = MakeRefPtr<RefCountedBase>();
+  already_AddRefed<RefCountedBase> b = a.forget();
+}
+
+void test_construct_implicitly_cast_result_type() {
+  RefPtr<RefCountedDerived> a = MakeRefPtr<RefCountedDerived>();
+  already_AddRefed<RefCountedBase> b = a.forget();
+}
+
+void foo(already_AddRefed<RefCountedBase>&& aArg);
+
+void test_call_with_result_type() {
+  RefPtr<RefCountedBase> a = MakeRefPtr<RefCountedBase>();
+  foo(a.forget());
+}
+
+void test_call_with_implicitly_cast_result_type() {
+  RefPtr<RefCountedDerived> a = MakeRefPtr<RefCountedDerived>();
+  foo(a.forget());
+}

build/clang-plugin/alpha/tests/sources.mozbuild

@@ -6,5 +6,6 @@
 
 SOURCES += [
     # 'AlphaTest.cpp',
+    "TestNonStdMove.cpp",
     'TestTempRefPtr.cpp',
-]
+]