Enable compiler errors for implicit switch case fallthroughs.

Also unify warning options across Clang and GCC and fix minor issues that was surfaced by this.

PiperOrigin-RevId: 508619655
Change-Id: I59b777bf2dfea4422485670e2427c7d3f5cf6405

.github/workflows/build.yml

@@ -10,11 +10,9 @@ name: build
 
 on:
   push:
-    branches: [main]
     paths-ignore:
       - "doc/**"
   pull_request:
-    branches: [main]
     paths-ignore:
       - "doc/**"
 
@@ -115,9 +113,9 @@ jobs:
     - name: Prepare macOS
       if: ${{ runner.os == 'macOS' }}
       run: brew install ninja
-    - uses: actions/setup-python@v3
+    - uses: actions/setup-python@v4
       with:
-        python-version: "3.10"
+        python-version: "3.11"
     - name: Prepare Python
       shell: bash
       run: |

cmake/MujocoOptions.cmake

@@ -85,15 +85,19 @@ include(MujocoLinkOptions)
 get_mujoco_extra_link_options(EXTRA_LINK_OPTIONS)
 
 if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU" OR (CMAKE_CXX_COMPILER_ID MATCHES "Clang" AND NOT MSVC))
-  set(EXTRA_COMPILE_OPTIONS -Wall -Werror)
+  set(EXTRA_COMPILE_OPTIONS
+      -Werror
+      -Wall
+      -Wimplicit-fallthrough
+      -Wunused
+      -Wno-int-in-bool-context
+      -Wno-sign-compare
+      -Wno-unknown-pragmas
+  )
   if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
-    # TODO: This should add to EXTRA_COMPILE_OPTIONS rather than overwrite it.
+    # Set -Wimplicit-fallthrough=5 to only allow fallthrough annotation via __attribute__.
-    set(EXTRA_COMPILE_OPTIONS
+    set(EXTRA_COMPILE_OPTIONS ${EXTRA_COMPILE_OPTIONS} -Wimplicit-fallthrough=5
-        -Wno-int-in-bool-context
+                              -Wno-maybe-uninitialized
-        -Wno-maybe-uninitialized
-        -Wno-sign-compare
-        -Wno-stringop-overflow
-        -Wno-stringop-truncation
     )
   endif()
 endif()

plugin/elasticity/CMakeLists.txt

@@ -12,9 +12,8 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-set(MUJOCO_ELASTICITY_INCLUDE
+set(MUJOCO_ELASTICITY_INCLUDE ${CMAKE_CURRENT_SOURCE_DIR}/../..
-    ${CMAKE_CURRENT_SOURCE_DIR}/../..
+                              ${CMAKE_CURRENT_SOURCE_DIR}/../../src
-    ${CMAKE_CURRENT_SOURCE_DIR}/../../src
 )
 
 set(MUJOCO_ELASTICITY_SRCS

python/mujoco/CMakeLists.txt

@@ -67,7 +67,12 @@ include(FindOrFetch)
 
 # ==================== MUJOCO LIBRARY ==========================================
 if(NOT TARGET mujoco)
-  add_library(mujoco SHARED IMPORTED GLOBAL)
+  add_library(
+    mujoco
+    SHARED
+    IMPORTED
+    GLOBAL
+  )
   if(APPLE)
     # On macOS, check if we are using mujoco.framework first.
     # Framework headers are searched differently from normal headers.
@@ -75,13 +80,17 @@ if(NOT TARGET mujoco)
     find_path(MUJOCO_FRAMEWORK mujoco.Framework HINTS ${MUJOCO_FRAMEWORK_DIR})
     if(MUJOCO_FRAMEWORK)
       message("MuJoCo framework is at ${MUJOCO_FRAMEWORK}/mujoco.framework")
-      set(MUJOCO_LIBRARY ${MUJOCO_FRAMEWORK}/mujoco.framework/Versions/A/libmujoco.2.3.2.dylib)
+      set(MUJOCO_LIBRARY
+          ${MUJOCO_FRAMEWORK}/mujoco.framework/Versions/A/libmujoco.2.3.2.dylib
+      )
       target_compile_options(mujoco INTERFACE -F${MUJOCO_FRAMEWORK})
     endif()
   endif()
 
   if(NOT MUJOCO_FRAMEWORK)
-    find_library(MUJOCO_LIBRARY mujoco mujoco.2.3.2 HINTS ${MUJOCO_LIBRARY_DIR} REQUIRED)
+    find_library(
+      MUJOCO_LIBRARY mujoco mujoco.2.3.2 HINTS ${MUJOCO_LIBRARY_DIR} REQUIRED
+    )
     find_path(MUJOCO_INCLUDE mujoco/mujoco.h HINTS ${MUJOCO_INCLUDE_DIR} REQUIRED)
     message("MuJoCo is at ${MUJOCO_LIBRARY}")
     message("MuJoCo headers are at ${MUJOCO_INCLUDE}")
@@ -276,16 +285,18 @@ macro(mujoco_pybind11_module name)
   pybind11_add_module(${name} ${ARGN})
   target_compile_options(${name} PRIVATE ${AVX_COMPILE_OPTIONS})
   if(NOT MSVC)
-    target_compile_options(${name} PRIVATE -Wall -Werror)
+    target_compile_options(
-    if(CMAKE_C_COMPILER_ID STREQUAL GNU)
+      ${name}
-      target_compile_options(
+      PRIVATE -Werror
-        ${name}
+              -Wall
-        PRIVATE -Wno-int-in-bool-context
+              -Wimplicit-fallthrough
-                -Wno-maybe-uninitialized
+              -Wunused
-                -Wno-sign-compare
+              -Wno-int-in-bool-context
-                -Wno-stringop-overflow
+              -Wno-sign-compare
-                -Wno-stringop-truncation
+    )
-      )
+    if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
+      # Only allow fallthrough annotation via __attribute__.
+      target_compile_options(${name} PRIVATE -Wimplicit-fallthrough=5)
     endif()
   endif()
   set_target_properties(${name} PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})
@@ -380,7 +391,8 @@ target_link_libraries(
   PRIVATE mujoco
           mujoco::libsimulate
           raw
-          structs_header)
+          structs_header
+)
 
 set(LIBRARIES_FOR_WHEEL
     "$<TARGET_FILE:_callbacks>"

python/mujoco/errors.h

@@ -110,7 +110,8 @@ static thread_local std::jmp_buf mju_error_jmp_buf;
 static thread_local std::array<char, 1024> mju_error_msg{0};
 
 static inline void MjErrorHandler(const char* msg) {
-  std::strncpy(mju_error_msg.data(), msg, mju_error_msg.size());
+  std::strncpy(mju_error_msg.data(), msg, mju_error_msg.size() - 1);
+  mju_error_msg.data()[mju_error_msg.size() - 1] = '\0';
   std::longjmp(mju_error_jmp_buf, 1);
 }
 

python/mujoco/simulate.cc

@@ -25,6 +25,11 @@
 
 namespace mujoco::python {
 namespace {
+template <typename T, int N>
+constexpr inline std::size_t sizeof_arr(const T(&arr)[N]) {
+  return sizeof(arr);
+}
+
 PYBIND11_MODULE(_simulate, pymodule) {
   namespace py = ::pybind11;
   using SimulateMutex = decltype(mujoco::Simulate::mtx);
@@ -138,8 +143,9 @@ PYBIND11_MODULE(_simulate, pymodule) {
             return simulate.loadError;
           },
           [](mujoco::Simulate& simulate, const std::string& error) {
-            std::strncpy(simulate.loadError, error.c_str(),
+            const auto max_length = sizeof_arr(simulate.loadError);
-                         simulate.kMaxFilenameLength);
+            std::strncpy(simulate.loadError, error.c_str(), max_length - 1);
+            simulate.loadError[max_length - 1] = '\0';
           });
 
   pymodule.def("setglfwdlhandle", [](std::uintptr_t dlhandle) {

sample/array_safety.h

@@ -73,7 +73,16 @@ static inline int sprintf_arr(char (&dest)[N], const char* format, ...) {
 // dest is guaranteed to be null-terminated
 template <std::size_t N>
 static inline char* strcat_arr(char (&dest)[N], const char* src) {
-  return std::strncat(dest, src, sizeof_arr(dest) - strlen_arr(dest) - 1);
+  const std::size_t dest_len = strlen_arr(dest);
+  const std::size_t dest_size = sizeof_arr(dest);
+  for (std::size_t i = dest_len; i < dest_size; ++i) {
+    dest[i] = src[i - dest_len];
+    if (!dest[i]) {
+      break;
+    }
+  }
+  dest[dest_size - 1] = '\0';
+  return dest;
 }
 
 // like std::strcpy but won't write beyond the bound of dest

sample/cmake/SampleOptions.cmake

@@ -85,15 +85,19 @@ include(MujocoLinkOptions)
 get_mujoco_extra_link_options(EXTRA_LINK_OPTIONS)
 
 if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU" OR (CMAKE_CXX_COMPILER_ID MATCHES "Clang" AND NOT MSVC))
-  set(EXTRA_COMPILE_OPTIONS -Wall -Werror)
+  set(EXTRA_COMPILE_OPTIONS
+      -Werror
+      -Wall
+      -Wimplicit-fallthrough
+      -Wunused
+      -Wno-int-in-bool-context
+      -Wno-sign-compare
+      -Wno-unknown-pragmas
+  )
   if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
-    # TODO: This should add to EXTRA_COMPILE_OPTIONS rather than overwrite it.
+    # Set -Wimplicit-fallthrough=5 to only allow fallthrough annotation via __attribute__.
-    set(EXTRA_COMPILE_OPTIONS
+    set(EXTRA_COMPILE_OPTIONS ${EXTRA_COMPILE_OPTIONS} -Wimplicit-fallthrough=5
-        -Wno-int-in-bool-context
+                              -Wno-maybe-uninitialized
-        -Wno-maybe-uninitialized
-        -Wno-sign-compare
-        -Wno-stringop-overflow
-        -Wno-stringop-truncation
     )
   endif()
 endif()

simulate/CMakeLists.txt

@@ -104,19 +104,13 @@ endif()
 add_library(platform_ui_adapter OBJECT)
 target_sources(
   platform_ui_adapter
-  PUBLIC glfw_adapter.h
+  PUBLIC glfw_adapter.h glfw_dispatch.h platform_ui_adapter.h
-         glfw_dispatch.h
+  PRIVATE glfw_adapter.cc glfw_dispatch.cc platform_ui_adapter.cc
-         platform_ui_adapter.h
-  PRIVATE glfw_adapter.cc
-          glfw_dispatch.cc
-          platform_ui_adapter.cc
 )
 target_compile_options(platform_ui_adapter PUBLIC ${MUJOCO_SIMULATE_COMPILE_OPTIONS})
 target_include_directories(
-  platform_ui_adapter
+  platform_ui_adapter PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}
-  PUBLIC
+                             $<TARGET_PROPERTY:glfw,INTERFACE_INCLUDE_DIRECTORIES>
-  ${CMAKE_CURRENT_SOURCE_DIR}
-  $<TARGET_PROPERTY:glfw,INTERFACE_INCLUDE_DIRECTORIES>
 )
 target_link_libraries(platform_ui_adapter PUBLIC mujoco::mujoco)
 if(SIMULATE_GLFW_DYNAMIC_SYMBOLS)
@@ -131,8 +125,7 @@ add_library(mujoco::libsimulate ALIAS libsimulate)
 target_sources(
   libsimulate
   PUBLIC simulate.h
-  PRIVATE simulate.cc
+  PRIVATE simulate.cc array_safety.h
-          array_safety.h
 )
 target_include_directories(libsimulate PUBLIC ${CMAKE_CURRENT_SOURCE_DIR})
 target_compile_options(libsimulate PUBLIC ${MUJOCO_SIMULATE_COMPILE_OPTIONS})

simulate/array_safety.h

@@ -73,7 +73,16 @@ static inline int sprintf_arr(char (&dest)[N], const char* format, ...) {
 // dest is guaranteed to be null-terminated
 template <std::size_t N>
 static inline char* strcat_arr(char (&dest)[N], const char* src) {
-  return std::strncat(dest, src, sizeof_arr(dest) - strlen_arr(dest) - 1);
+  const std::size_t dest_len = strlen_arr(dest);
+  const std::size_t dest_size = sizeof_arr(dest);
+  for (std::size_t i = dest_len; i < dest_size; ++i) {
+    dest[i] = src[i - dest_len];
+    if (!dest[i]) {
+      break;
+    }
+  }
+  dest[dest_size - 1] = '\0';
+  return dest;
 }
 
 // like std::strcpy but won't write beyond the bound of dest

simulate/cmake/SimulateOptions.cmake

@@ -85,15 +85,19 @@ include(MujocoLinkOptions)
 get_mujoco_extra_link_options(EXTRA_LINK_OPTIONS)
 
 if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU" OR (CMAKE_CXX_COMPILER_ID MATCHES "Clang" AND NOT MSVC))
-  set(EXTRA_COMPILE_OPTIONS -Wall -Werror)
+  set(EXTRA_COMPILE_OPTIONS
+      -Werror
+      -Wall
+      -Wimplicit-fallthrough
+      -Wunused
+      -Wno-int-in-bool-context
+      -Wno-sign-compare
+      -Wno-unknown-pragmas
+  )
   if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
-    # TODO: This should add to EXTRA_COMPILE_OPTIONS rather than overwrite it.
+    # Set -Wimplicit-fallthrough=5 to only allow fallthrough annotation via __attribute__.
-    set(EXTRA_COMPILE_OPTIONS
+    set(EXTRA_COMPILE_OPTIONS ${EXTRA_COMPILE_OPTIONS} -Wimplicit-fallthrough=5
-        -Wno-int-in-bool-context
+                              -Wno-maybe-uninitialized
-        -Wno-maybe-uninitialized
-        -Wno-sign-compare
-        -Wno-stringop-overflow
-        -Wno-stringop-truncation
     )
   endif()
 endif()

src/cc/array_safety.h

@@ -73,7 +73,16 @@ static inline int sprintf_arr(char (&dest)[N], const char* format, ...) {
 // dest is guaranteed to be null-terminated
 template <std::size_t N>
 static inline char* strcat_arr(char (&dest)[N], const char* src) {
-  return std::strncat(dest, src, sizeof_arr(dest) - strlen_arr(dest) - 1);
+  const std::size_t dest_len = strlen_arr(dest);
+  const std::size_t dest_size = sizeof_arr(dest);
+  for (std::size_t i = dest_len; i < dest_size; ++i) {
+    dest[i] = src[i - dest_len];
+    if (!dest[i]) {
+      break;
+    }
+  }
+  dest[dest_size - 1] = '\0';
+  return dest;
 }
 
 // like std::strcpy but won't write beyond the bound of dest

src/engine/engine_collision_box.c

@@ -316,7 +316,7 @@ int mjc_CapsuleBox(const mjModel* m, const mjData* d, mjContact* con,
         };
         mjtNum c[2];
       } d2;
-      d2 p, s, d, c /*, tmp1*/;
+      d2 p, s, d /*, c, tmp1*/;
       mjtNum u, v, w, e1, best /* ,e2 */, l /* , e3, e4 */;
 
       bestdist = bestdistmax;
@@ -366,8 +366,8 @@ int mjc_CapsuleBox(const mjModel* m, const mjData* d, mjContact* con,
         }
       }
 
-      c.x = s.x * ((c1 / 2) ? -1 : 1);
+      // c.x = s.x * ((c1 / 2) ? -1 : 1);
-      c.y = s.y * ((c1 % 2) ? -1 : 1);
+      // c.y = s.y * ((c1 % 2) ? -1 : 1);
 
       e1 = fabs(w) / l;
       // e2 = best / l;

src/engine/engine_core_constraint.c

@@ -540,6 +540,7 @@ void mj_instantiateEquality(const mjModel* m, mjData* d) {
 
       case mjEQ_DISTANCE:
         mju_error("distance equality constraints are no longer supported");
+        break;
 
       default:                    // SHOULD NOT OCCUR
         mju_error_i("Invalid equality constraint type %d", m->eq_type[i]);

src/engine/engine_core_smooth.c

@@ -21,6 +21,7 @@
 #include <mujoco/mjmodel.h>
 #include "engine/engine_callback.h"
 #include "engine/engine_core_constraint.h"
+#include "engine/engine_crossplatform.h"
 #include "engine/engine_io.h"
 #include "engine/engine_macro.h"
 #include "engine/engine_plugin.h"
@@ -236,6 +237,7 @@ void mj_comPos(const mjModel* m, mjData* d) {
 
       // rotation components: same as ball
       skip = 18;
+      mjFALLTHROUGH;
 
     case mjJNT_BALL:
       for (int i=0; i<3; i++) {
@@ -1283,6 +1285,7 @@ void mj_comVel(const mjModel* m, mjData* d) {
 
         // continue with rotations
         j += 3;
+        mjFALLTHROUGH;
 
       case mjJNT_BALL:
         // compute all 3 cdofdots using parent velocity
@@ -1349,6 +1352,7 @@ void mj_passive(const mjModel* m, mjData* d) {
       // continue with rotations
       dadr += 3;
       padr += 3;
+      mjFALLTHROUGH;
 
     case mjJNT_BALL:
       // covert quatertion difference into angular "velocity"
@@ -2032,6 +2036,7 @@ void mj_rnePostConstraint(const mjModel* m, mjData* d) {
 
     case mjEQ_DISTANCE:
       mju_error("distance equality constraints are no longer supported");
+      break;
 
     default:
       mju_error_i("Unknown constraint type type %d", m->eq_type[id]);    // SHOULD NOT OCCUR

src/engine/engine_crossplatform.h

@@ -46,8 +46,10 @@
   #endif
 #endif
 
-#if defined(__GNUC__) && __GNUC__ >= 7
+#if defined(__cplusplus)
-  #define mjFALLTHROUGH __attribute__ ((fallthrough))
+  #define mjFALLTHROUGH [[fallthrough]]
+#elif defined(__clang__) || (defined(__GNUC__) && __GNUC__ >= 7)
+  #define mjFALLTHROUGH __attribute__((fallthrough))
 #else
   #define mjFALLTHROUGH ((void) 0)
 #endif

src/engine/engine_derivative.c

@@ -22,6 +22,7 @@
 #include "engine/engine_core_smooth.h"
 #include "engine/engine_forward.h"
 #include "engine/engine_core_constraint.h"
+#include "engine/engine_crossplatform.h"
 #include "engine/engine_io.h"
 #include "engine/engine_inverse.h"
 #include "engine/engine_macro.h"
@@ -402,6 +403,7 @@ static void mjd_comVel_vel(const mjModel* m, mjData* d, mjtNum* Dcvel, mjtNum* D
 
         // continue with rotations
         j += 3;
+        mjFALLTHROUGH;
 
       case mjJNT_BALL:
         // Dcdofdot = D crossMotion(cvel, cdof)

src/engine/engine_sensor.c

@@ -21,6 +21,7 @@
 #include <mujoco/mjplugin.h>
 #include "engine/engine_callback.h"
 #include "engine/engine_core_smooth.h"
+#include "engine/engine_crossplatform.h"
 #include "engine/engine_io.h"
 #include "engine/engine_macro.h"
 #include "engine/engine_plugin.h"
@@ -804,6 +805,7 @@ void mj_energyPos(const mjModel* m, mjData* d) {
 
         // continue with rotations
         padr += 3;
+        mjFALLTHROUGH;
 
       case mjJNT_BALL:
         // covert quatertion difference into angular "velocity"

src/engine/engine_support.c

@@ -1270,7 +1270,8 @@ void mj_differentiatePos(const mjModel* m, mjtNum* qvel, mjtNum dt,
       vadr += 3;
       padr += 3;
 
-    // continute with rotations
+      // continute with rotations
+      mjFALLTHROUGH;
 
     case mjJNT_BALL:
       mju_negQuat(neg, qpos1+padr);           // solve:  qpos1 * dif = qpos2
@@ -1304,7 +1305,8 @@ void mj_integratePos(const mjModel* m, mjtNum* qpos, const mjtNum* qvel, mjtNum
       padr += 3;
       vadr += 3;
 
-    // continue with rotation update
+      // continue with rotation update
+      mjFALLTHROUGH;
 
     case mjJNT_BALL:
       // quaternion update

src/engine/engine_vis_visualize.c

@@ -62,6 +62,7 @@ static void makeLabel(const mjModel* m, mjtObj type, int id, char* label) {
 
   // copy result into label
   strncpy(label, txt, 99);
+  label[99] = '\0';
 }
 
 

src/user/user_mesh.cc

@@ -1854,9 +1854,9 @@ void mjCSkin::LoadSKN(const mjVFS* vfs) {
 
     // read name
     char txt[40];
-    strncpy(txt, (char*)(pdata+cnt), 40);
+    strncpy(txt, (char*)(pdata+cnt), 39);
+    txt[39] = '\0';
     cnt += 10;
-    txt[39] = 0;
     bodyname.push_back(txt);
 
     // read bindpos

test/CMakeLists.txt

@@ -21,7 +21,13 @@ macro(mujoco_test name)
   set(options)
   set(oneValueArgs)
   set(multiValueArgs PROPERTIES)
-  cmake_parse_arguments(_ARGS "${options}" "${oneValueArgs}" "${multiValueArgs}" ${ARGN})
+  cmake_parse_arguments(
+    _ARGS
+    "${options}"
+    "${oneValueArgs}"
+    "${multiValueArgs}"
+    ${ARGN}
+  )
 
   add_executable(${name} ${name}.cc)
   target_link_libraries(${name} gtest_main mujoco)

test/user/CMakeLists.txt

@@ -19,7 +19,12 @@ mujoco_test(user_objects_test)
 target_link_libraries(user_objects_test fixture gmock)
 
 mujoco_test(user_mesh_test)
-target_link_libraries(user_mesh_test fixture gmock absl::str_format)
+target_link_libraries(
+  user_mesh_test
+  fixture
+  gmock
+  absl::str_format
+)
 
 mujoco_test(user_composite_test)
 target_link_libraries(user_composite_test fixture gmock)

test/xml/xml_native_writer_test.cc

@@ -917,8 +917,8 @@ TEST_F(XMLWriterLocaleTest, IgnoresLocale) {
   mj_deleteModel(model);
 
   // Test that MuJoCo doesn't override locales for subsequent calls.
-  char formatted[7];
+  char formatted[8];
-  std::snprintf(formatted, sizeof(formatted), "%f", 3.9375);
+  std::snprintf(formatted, sizeof(formatted), "%.4f", 3.9375);
   EXPECT_EQ(std::string(formatted), "3,9375");
 }