Use clang plugin to check Maybe variables are used

.github/workflows/test.yml

@@ -92,6 +92,52 @@ jobs:
         run: |
           exit 1
 
+  static_analysis_with_clang_plug_in:
+    name: Static analysis with Clang plug-in
+    runs-on: ubuntu-20.04
+    if: github.event.pull_request.draft == false && contains(github.event.pull_request.requested_reviewers.*.login, 'oneflow-ci-bot')
+    steps:
+      - name: Check out OneFlow
+        uses: actions/checkout@v2
+        with:
+          ref: ${{ github.event.pull_request.head.ref }}
+          repository: ${{github.event.pull_request.head.repo.full_name}}
+      - name: Install dependencies
+        run: |
+          wget -O - https://apt.llvm.org/llvm-snapshot.gpg.key | sudo apt-key add -
+          sudo apt-add-repository "deb http://apt.llvm.org/focal/ llvm-toolchain-focal-12 main"
+          sudo apt-get update
+          sudo apt-get install -y llvm-12 llvm-12-dev llvm-12-tools clang-12 libclang-common-12-dev libclang-12-dev \
+            libopenblas-dev nasm python3-pip ninja-build
+
+      - name: Build Clang plug-in
+        run: |
+          export CC=clang-12
+          export CXX=clang++-12
+          cd tools/clang-plugin
+          mkdir build
+          cd build
+          cmake .. -DCP_LLVM_INSTALL_DIR=/usr/lib/llvm-12
+          make -j$(nproc)
+          clang_plugin_path="${PWD}/lib/libCheckUnusedMaybe.so"
+          ldd ${clang_plugin_path}
+          echo "clang_plugin_path=${clang_plugin_path}" >> $GITHUB_ENV
+      - name: Check unused Maybe (by building OneFlow with Clang plug-in)
+        run: |
+          export PROJECT_DIR=${PWD}
+          mkdir build
+          cd build
+          export CC=clang-12
+          export CXX=clang++-12
+          export ONEFLOW_MAYBE_CHECK_ONLY_FN=${PROJECT_DIR}/oneflow/
+          export ONEFLOW_MAYBE_CHECK_SKIP_FN=.cfg.cpp
+          cmake .. -C ../cmake/caches/international/cpu.cmake \
+            -DCMAKE_BUILD_TYPE=Release \
+            -DBUILD_TESTING=ON \
+            -DCMAKE_CXX_FLAGS="-fplugin=${{ env.clang_plugin_path }}"
+          cmake --build . -j$(nproc) --target oneflow_deps
+          cmake --build . -j$(nproc)
+
   wait_for_gpu_slot:
     name: Wait for GPU slots
     runs-on: [self-hosted, scheduler]

oneflow/core/framework/consistent_tensor_infer_cache.cpp

@@ -152,7 +152,7 @@ Maybe<Operator> MakeOp(const UserOpExpr& user_op_expr, const AttrMap& attrs,
         [&](int32_t i) { return output_mut_metas.at(i).mut_tensor_meta(); }));
   }
   const auto& op = JUST(MakeOp(user_op_expr, infer_args.attrs(), parallel_desc->device_tag()));
-  op->FillOpParallelDesc(parallel_desc.shared_from_symbol());
+  JUST(op->FillOpParallelDesc(parallel_desc.shared_from_symbol()));
   {
     // Infer parallel distribution.
     cfg::ParallelDistributionSignature parallel_distribution_constraints;

oneflow/core/framework/instructions_builder.cpp

@@ -145,12 +145,18 @@ Maybe<compatible_py::BlobObject> MakeNewBlobObjectLike(
 }
 
 Maybe<void> _ReleaseLogicalObject(compatible_py::Object* obj) {
-  JUST(LogicalRun([&obj](InstructionsBuilder* build) { build->DeleteObject(obj); }));
+  JUST(LogicalRun([&obj](InstructionsBuilder* build) -> Maybe<void> {
+    JUST(build->DeleteObject(obj));
+    return Maybe<void>::Ok();
+  }));
   return Maybe<void>::Ok();
 }
 
 Maybe<void> _ReleasePhysicalObject(compatible_py::Object* obj) {
-  JUST(PhysicalRun([&obj](InstructionsBuilder* build) { build->DeleteObject(obj); }));
+  JUST(PhysicalRun([&obj](InstructionsBuilder* build) -> Maybe<void> {
+    JUST(build->DeleteObject(obj));
+    return Maybe<void>::Ok();
+  }));
   return Maybe<void>::Ok();
 }
 

tools/clang-plugin/.gitignore

@@ -0,0 +1,4 @@
+build/
+build-*/
+.cache/
+compile_commands.json

tools/clang-plugin/CMakeLists.txt

@@ -0,0 +1,66 @@
+cmake_minimum_required(VERSION 3.10)
+project(clang-plugins CXX)
+
+set(CP_LLVM_INSTALL_DIR "" CACHE PATH "LLVM installation directory")
+
+# A bit of a sanity checking
+set(CP_LLVM_INCLUDE_DIR "${CP_LLVM_INSTALL_DIR}/include/llvm")
+if(NOT EXISTS "${CP_LLVM_INCLUDE_DIR}")
+message(FATAL_ERROR
+  "${CP_LLVM_INCLUDE_DIR} does not exist, CP_LLVM_INSTALL_DIR (${CP_LLVM_INSTALL_DIR}) is invalid")
+endif()
+
+set(CP_LLVM_CMAKE_FILE "${CP_LLVM_INSTALL_DIR}/lib/cmake/clang/ClangConfig.cmake")
+if(NOT EXISTS "${CP_LLVM_CMAKE_FILE}")
+message(FATAL_ERROR
+  " CP_LLVM_CMAKE_FILE (${CP_LLVM_CMAKE_FILE}) is invalid.")
+endif()
+
+#===============================================================================
+# 2. LOAD CLANG CONFIGURATION
+#    For more: http://llvm.org/docs/CMake.html#embedding-llvm-in-your-project
+#===============================================================================
+list(APPEND CMAKE_PREFIX_PATH "${CP_LLVM_INSTALL_DIR}/lib/cmake/llvm/")
+list(APPEND CMAKE_PREFIX_PATH "${CP_LLVM_INSTALL_DIR}/lib/cmake/clang/")
+
+find_package(Clang REQUIRED CONFIG)
+
+# Sanity check. As Clang does not expose e.g. `CLANG_VERSION_MAJOR` through
+# AddClang.cmake, we have to use LLVM_VERSION_MAJOR instead.
+# TODO: Revisit when next version is released.
+if(NOT "12" VERSION_EQUAL "${LLVM_VERSION_MAJOR}")
+  message(FATAL_ERROR "Found LLVM ${LLVM_VERSION_MAJOR}, but need LLVM 12")
+endif()
+
+message(STATUS "Found Clang ${LLVM_PACKAGE_VERSION}")
+message(STATUS "Using ClangConfig.cmake in: ${CP_LLVM_INSTALL_DIR}")
+
+message("CLANG STATUS:
+  Includes (clang)    ${CLANG_INCLUDE_DIRS}
+  Includes (llvm)     ${LLVM_INCLUDE_DIRS}"
+)
+
+# Set the LLVM and Clang header and library paths
+include_directories(SYSTEM "${LLVM_INCLUDE_DIRS};${CLANG_INCLUDE_DIRS}")
+
+#===============================================================================
+# 3. BUILD CONFIGURATION
+#===============================================================================
+# Use the same C++ standard as LLVM does
+set(CMAKE_CXX_STANDARD 14 CACHE STRING "")
+
+# -fvisibility-inlines-hidden is set when building LLVM and on Darwin warnings
+# are triggered if llvm-tutor is built without this flag (though otherwise it
+# builds fine). For consistency, add it here too.
+include(CheckCXXCompilerFlag)
+check_cxx_compiler_flag("-fvisibility-inlines-hidden" SUPPORTS_FVISIBILITY_INLINES_HIDDEN_FLAG)
+if (${SUPPORTS_FVISIBILITY_INLINES_HIDDEN_FLAG} EQUAL "1")
+  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fvisibility-inlines-hidden")
+endif()
+
+# Set the build directories
+set(CMAKE_RUNTIME_OUTPUT_DIRECTORY "${PROJECT_BINARY_DIR}/bin")
+set(CMAKE_LIBRARY_OUTPUT_DIRECTORY "${PROJECT_BINARY_DIR}/lib")
+
+add_subdirectory(CheckUnusedMaybe)
+

tools/clang-plugin/CheckUnusedMaybe/CMakeLists.txt

@@ -0,0 +1,18 @@
+add_library(CheckUnusedMaybe SHARED CheckUnusedMaybe.cpp)
+
+# On Darwin (unlike on Linux), undefined symbols in shared objects are not
+# allowed at the end of the link-edit. The plugins defined here:
+#  - _are_ shared objects
+#  - reference symbols from LLVM shared libraries, i.e. symbols which are
+#    undefined until those shared objects are loaded in memory (and hence
+#    _undefined_ during static linking)
+# The build will fail with errors like this:
+#    "Undefined symbols for architecture x86_64"
+# with various LLVM symbols being undefined. Since those symbols are later
+# loaded and resolved at runtime, these errors are false positives.
+# This behaviour can be modified via the '-undefined' OS X linker flag as
+# follows.
+target_link_libraries(
+  CheckUnusedMaybe
+  "$<$<PLATFORM_ID:Darwin>:-undefined dynamic_lookup>"
+  )

tools/clang-plugin/CheckUnusedMaybe/CheckUnusedMaybe.cpp

@@ -0,0 +1,142 @@
+#include "clang/AST/ASTConsumer.h"
+#include "clang/AST/ExprCXX.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Stmt.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Frontend/FrontendPluginRegistry.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/CrashRecoveryContext.h"
+
+using namespace clang;
+
+class CheckUnusedMaybeVisitor
+    : public RecursiveASTVisitor<CheckUnusedMaybeVisitor> {
+public:
+  explicit CheckUnusedMaybeVisitor(ASTContext *Context) : Context(Context) {
+    {
+      auto skip_filenames_env =
+          llvm::StringRef(std::getenv("ONEFLOW_MAYBE_CHECK_SKIP_FN"));
+      if (!skip_filenames_env.empty()) {
+        skip_filenames_env.split(skip_filenames, ";");
+        for (const auto &x : skip_filenames) {
+          llvm::outs() << "skip: " << x << "\n";
+        }
+      }
+    }
+    {
+      auto only_filenames_env =
+          llvm::StringRef(std::getenv("ONEFLOW_MAYBE_CHECK_ONLY_FN"));
+      if (!only_filenames_env.empty()) {
+        only_filenames_env.split(only_filenames, ";");
+        for (const auto &x : only_filenames) {
+          llvm::outs() << "only: " << x << "\n";
+        }
+      }
+    }
+  }
+
+  virtual bool VisitCompoundStmt(CompoundStmt *stmt) {
+    if (!only_filenames.empty()) {
+      bool skip = true;
+      for (const auto &x : only_filenames) {
+        if (Context->getSourceManager()
+                .getFilename(stmt->getBeginLoc())
+                .contains(x)) {
+          skip = false;
+        }
+      }
+      if (skip) {
+        return true;
+      }
+    }
+
+    for (const auto &x : skip_filenames) {
+      if (Context->getSourceManager()
+              .getFilename(stmt->getBeginLoc())
+              .contains(x)) {
+        return true;
+      }
+    }
+
+    for (const auto &x : stmt->children()) {
+      if (ExprWithCleanups *expr = dyn_cast<ExprWithCleanups>(x)) {
+        std::string type = expr->getType().getAsString();
+        if (type.substr(0, 12) == "class Maybe<" ||
+            type.substr(0, 6) == "Maybe<") {
+          DiagnosticsEngine &DE = Context->getDiagnostics();
+          unsigned DiagID =
+              DE.getCustomDiagID(DiagnosticsEngine::Error,
+                                 "This function returns Maybe but the return "
+                                 "value is ignored. Wrap it with JUST(..)?");
+          auto DB = DE.Report(expr->getBeginLoc(), DiagID);
+          DB.AddSourceRange(
+              clang::CharSourceRange::getCharRange(expr->getSourceRange()));
+        }
+      }
+      if (CallExpr *call = dyn_cast<CallExpr>(x)) {
+        std::string returnType;
+        llvm::CrashRecoveryContext CRC;
+        CRC.RunSafely([&call, &returnType, this]() {
+          if (auto *callee = call->getDirectCallee()) {
+            returnType = callee->getReturnType().getAsString();
+          } else {
+            returnType = call->getCallReturnType(*this->Context).getAsString();
+          }
+        });
+        if (returnType.substr(0, 12) == "class Maybe<" ||
+            returnType.substr(0, 6) == "Maybe<") {
+          DiagnosticsEngine &DE = Context->getDiagnostics();
+          unsigned DiagID =
+              DE.getCustomDiagID(DiagnosticsEngine::Error,
+                                 "This function returns Maybe but the return "
+                                 "value is ignored. Wrap it with JUST(..)?");
+          auto DB = DE.Report(call->getBeginLoc(), DiagID);
+          DB.AddSourceRange(
+              clang::CharSourceRange::getCharRange(call->getSourceRange()));
+        }
+      }
+    }
+    return true;
+  }
+
+private:
+  ASTContext *Context;
+  llvm::SmallVector<llvm::StringRef, 10> skip_filenames;
+  llvm::SmallVector<llvm::StringRef, 10> only_filenames;
+};
+
+class CheckUnusedMaybeConsumer : public clang::ASTConsumer {
+public:
+  explicit CheckUnusedMaybeConsumer(ASTContext *Context) : Visitor(Context) {}
+
+  void HandleTranslationUnit(clang::ASTContext &Context) override {
+    Visitor.TraverseDecl(Context.getTranslationUnitDecl());
+  }
+
+private:
+  CheckUnusedMaybeVisitor Visitor;
+};
+
+class CheckUnusedMaybeAction : public clang::PluginASTAction {
+public:
+  virtual std::unique_ptr<clang::ASTConsumer>
+  CreateASTConsumer(clang::CompilerInstance &Compiler,
+                    llvm::StringRef InFile) override {
+    return std::make_unique<CheckUnusedMaybeConsumer>(
+        &Compiler.getASTContext());
+  }
+
+  bool ParseArgs(const CompilerInstance &CI,
+                 const std::vector<std::string> &args) override {
+    return true;
+  }
+
+  // Automatically run the plugin after the main AST action
+  ActionType getActionType() override { return AddAfterMainAction; }
+};
+
+static FrontendPluginRegistry::Add<CheckUnusedMaybeAction>
+    X("check-unused-maybe", "Check unused maybe");
+

tools/clang-plugin/CheckUnusedMaybe/unused_maybe_example.cpp

@@ -0,0 +1,8 @@
+template <class T> class Maybe {};
+
+Maybe<void> a() { return Maybe<void>(); }
+
+int main() {
+  a();
+  return 0;
+}