ci: add clang-tidy and clang-tidy-fix targets (#242)

* Add clang-tidy and clang-tidy-fix targets

* Make linter target depend on starling-raw.wasm

This is needed to build compile_command.json file that clang-tidy uses.

* fix: clang-analyzer errors

These are various fixes reported by clang-analyzer regarding memory
leaks, unsafe functions and inconsistent use use of new and free.

* fix: clang-analyzer errors in script loader

This changes script loader paths to use owned string instead of raw
pointers to fix potential memory leaks reported by clang. The tradeof is
that now we call c_str in few places, but the impact on performance
should be negligible since we call this only once.

* fix: enable cppcoreguidelines checks and apply auto fixes

This patch enables cppcoreguidelines but disables all the lints that
don't offer auto fixes.

* fix: enable modernize lints and apply auto fixes

This patch enables modernize lints and disable all lints from this group
that are not offering auto fix.

* fix: use js_new for RefCounted types

This fixes linter error, where clang complains about using new/free
instead of new/delete.

* fix: fix performance lint about unnecesairly large enum base

* fix: enable performace checks and appy autofixes

* fix: apply linter readability fixes

* fix: do not use const members

* fix: remove unused declarations

* ci: add CI step for linter

* fix: apply fixes after merging main

* fix: enable no-malloc checks and apply fixes

* fix: allow implicit pointer to bool conversions

* Apply suggestions from code review

Co-authored-by: Till Schneidereit <till@tillschneidereit.net>

* fix: remove clang pragmas

* fix: bounds constant array index

* fix: enable special member functions lint

* fix: enable type const cast check

* fix: enable bugprone checks

* fix: correct indentation

* Apply suggestions from code review

Co-authored-by: Till Schneidereit <till@tillschneidereit.net>

* fix: align declaration arguments with impl

---------

Co-authored-by: Till Schneidereit <till@tillschneidereit.net>

Author: andreiltd

.clang-tidy

@@ -0,0 +1,58 @@
+Checks: >
+  clang-analyzer-*,
+  clang-diagnostic-*,
+  cppcoreguidelines-*,
+  modernize-*,
+  performance-*,
+  readability-*,
+  bugprone-*,
+  misc-redundant-expression,
+  misc-uniqueptr-reset-release,
+  misc-unused-alias-decls,
+  misc-unused-using-decls,
+  -clang-analyzer-optin.core.EnumCastOutOfRange,
+  -cppcoreguidelines-avoid-do-while,
+  -cppcoreguidelines-avoid-c-arrays,
+  -cppcoreguidelines-pro-type-reinterpret-cast,
+  -cppcoreguidelines-avoid-magic-numbers,
+  -cppcoreguidelines-pro-bounds-array-to-pointer-decay,
+  -cppcoreguidelines-owning-memory,
+  -cppcoreguidelines-avoid-non-const-global-variables,
+  -cppcoreguidelines-pro-type-vararg,
+  -cppcoreguidelines-narrowing-conversions,
+  -cppcoreguidelines-pro-bounds-pointer-arithmetic,
+  -cppcoreguidelines-pro-type-cstyle-cast,
+  -cppcoreguidelines-init-variables,
+  -cppcoreguidelines-macro-usage,
+  -cppcoreguidelines-interfaces-global-init,
+  -cppcoreguidelines-use-default-member-init,
+  -cppcoreguidelines-pro-type-static-cast-downcast,
+  -modernize-use-trailing-return-type,
+  -modernize-avoid-c-arrays,
+  -modernize-use-ranges,
+  -readability-identifier-length,
+  -readability-static-accessed-through-instance,
+  -readability-function-cognitive-complexity,
+  -readability-magic-numbers,
+  -readability-simplify-boolean-expr,
+  -bugprone-easily-swappable-parameters,
+  -bugprone-dynamic-static-initializers,
+  -bugprone-assignment-in-if-condition,
+  -bugprone-narrowing-conversions,
+
+# Check options configuration
+CheckOptions:
+  - key: readability-implicit-bool-conversion.AllowPointerConditions
+    value: 'true'
+
+# Treat all warnings as errors
+WarningsAsErrors: '*'
+
+# Only warn on headers in the code tree
+HeaderFilterRegex: 'builtins/|runtime/'
+
+# Use the same style as clang-format (look for .clang-format)
+FormatStyle: file
+
+# Explicitly exclude system/dependency headers
+SystemHeaders: false

.github/workflows/main.yml

@@ -113,6 +113,13 @@ jobs:
         name: spidermonkey-${{ matrix.build }}
         path: spidermonkey-dist-${{ matrix.build }}/*
 
+    - name: Install Just
+      uses: taiki-e/install-action@just
+
+    - name: Run clang linter
+      if: matrix.build == 'debug'
+      run: just lint
+
   release-spidermonkey:
     needs: test
     if: needs.test.outputs.SM_TAG_EXISTS == 'false' && (github.event_name == 'push' && 

CMakeLists.txt

@@ -42,6 +42,7 @@ include("spidermonkey")
 include("openssl")
 include("${HOST_API}/host_api.cmake")
 include("build-crates")
+include("lint")
 
 option(ENABLE_JS_DEBUGGER "Enable support for debugging content via a socket connection" ON)
 

builtins/web/abort/abort-controller.cpp

@@ -1,9 +1,9 @@
 #include "abort-controller.h"
 #include "abort-signal.h"
 
-namespace builtins {
-namespace web {
-namespace abort {
+
+
+namespace builtins::web::abort {
 
 const JSFunctionSpec AbortController::static_methods[] = {
     JS_FS_END,
@@ -65,6 +65,6 @@ bool AbortController::init_class(JSContext *cx, JS::HandleObject global) {
   return init_class_impl(cx, global);
 }
 
-}  // namespace abort
-}  // namespace web
-}  // namespace builtins
+} // namespace builtins::web::abort
+
+

builtins/web/abort/abort-controller.h

@@ -3,9 +3,9 @@
 
 #include "builtin.h"
 
-namespace builtins {
-namespace web {
-namespace abort {
+
+
+namespace builtins::web::abort {
 
 class AbortController : public BuiltinImpl<AbortController> {
   static bool signal_get(JSContext *cx, unsigned argc, JS::Value *vp);
@@ -17,7 +17,7 @@ class AbortController : public BuiltinImpl<AbortController> {
   static constexpr const char *class_name = "AbortController";
   static constexpr unsigned ctor_length = 0;
 
-  enum Slots { Signal = 0, Count };
+  enum Slots : uint8_t { Signal = 0, Count };
 
   static const JSFunctionSpec static_methods[];
   static const JSPropertySpec static_properties[];
@@ -28,9 +28,9 @@ class AbortController : public BuiltinImpl<AbortController> {
   static bool constructor(JSContext *cx, unsigned argc, Value *vp);
 };
 
-} // namespace abort
-} // namespace web
-} // namespace builtins
+} // namespace builtins::web::abort
+
+
 
 
 

builtins/web/abort/abort-signal.cpp

@@ -5,9 +5,9 @@
 #include "../event/event.h"
 #include "../timers.h"
 
-namespace builtins {
-namespace web {
-namespace abort {
+
+
+namespace builtins::web::abort {
 
 using event::Event;
 using event::EventTarget;
@@ -195,7 +195,7 @@ bool AbortSignal::add_algorithm(JSObject *self, js::UniquePtr<AbortAlgorithm> al
   }
 
   // 2. Append algorithm to signal's abort algorithms.
-  auto algorithms = AbortSignal::algorithms(self);
+  auto *algorithms = AbortSignal::algorithms(self);
   return algorithms->append(std::move(algorithm));
 }
 
@@ -228,7 +228,7 @@ bool AbortSignal::abort(JSContext *cx, HandleObject self, HandleValue reason) {
   JS::RootedObjectVector dep_signals_to_abort(cx);
 
   // 4. For each dependentSignal of signal's dependent signals:
-  auto dep_signals = dependent_signals(self);
+  auto *dep_signals = dependent_signals(self);
   for (auto const &sig : dep_signals->items()) {
     RootedObject signal(cx, sig);
     // 1. If dependentSignal is not aborted:
@@ -259,9 +259,11 @@ bool AbortSignal::abort(JSContext *cx, HandleObject self, HandleValue reason) {
 bool AbortSignal::run_abort_steps(JSContext *cx, HandleObject self) {
   // To run the abort steps for an AbortSignal signal:
   // 1. For each algorithm of signal's abort algorithms: run algorithm.
-  auto algorithms = AbortSignal::algorithms(self);
+  auto *algorithms = AbortSignal::algorithms(self);
   for (auto &algorithm : *algorithms) {
-    if (!algorithm->run(cx)) return false;
+    if (!algorithm->run(cx)) {
+      return false;
+    }
   }
 
   // 2. Empty signals's abort algorithms.
@@ -274,11 +276,7 @@ bool AbortSignal::run_abort_steps(JSContext *cx, HandleObject self) {
   RootedObject event(cx, Event::create(cx, type_val, JS::NullHandleValue));
   RootedValue event_val(cx, JS::ObjectValue(*event));
 
-  if (!EventTarget::dispatch_event(cx, self, event_val, &res_val)) {
-    return false;
-  }
-
-  return true;
+  return EventTarget::dispatch_event(cx, self, event_val, &res_val);
 }
 
 // Set signal's abort reason to reason if it is given; otherwise to a new "AbortError" DOMException.
@@ -420,7 +418,7 @@ JSObject *AbortSignal::create_with_signals(JSContext *cx, HandleValueArray signa
 
   // 3. Set resultSignal's dependent to true.
   SetReservedSlot(self, Slots::Dependent, JS::TrueValue());
-  auto our_signals = source_signals(self);
+  auto *our_signals = source_signals(self);
 
   // 4. For each signal of signals:
   for (size_t i = 0; i < signals.length(); ++i) {
@@ -431,20 +429,20 @@ JSObject *AbortSignal::create_with_signals(JSContext *cx, HandleValueArray signa
       // 1. Append signal to resultSignal's source signals.
       our_signals->insert(signal);
       // 2. Append resultSignal to signal's dependent signals.
-      auto their_signals = dependent_signals(signal);
+      auto *their_signals = dependent_signals(signal);
       their_signals->insert(self);
     }
     // 2. Otherwise...
     else {
-      auto src_signals = source_signals(signal);
+      auto *src_signals = source_signals(signal);
       // for each sourceSignal of signal's source signals:
       for (auto const &source : src_signals->items()) {
         // 1. Assert: sourceSignal is not aborted and not dependent.
         MOZ_ASSERT(!is_aborted(source) && !is_dependent(source));
         // 2. Append sourceSignal to resultSignal's source signals.
         our_signals->insert(source);
         // 3. Append resultSignal to sourceSignal's dependent signals.
-        auto their_signals = dependent_signals(source);
+        auto *their_signals = dependent_signals(source);
         their_signals->insert(self);
       };
     }
@@ -469,21 +467,21 @@ void AbortSignal::trace(JSTracer *trc, JSObject *self) {
 
   auto has_sources = !JS::GetReservedSlot(self, Slots::SourceSignals).isNullOrUndefined();
   if (has_sources) {
-    auto srcsig = source_signals(self);
+    auto *srcsig = source_signals(self);
     srcsig->trace(trc);
     srcsig->traceWeak(trc);
   }
 
   auto has_deps = !JS::GetReservedSlot(self, Slots::DependentSignals).isNullOrUndefined();
   if (has_deps) {
-    auto depsig = dependent_signals(self);
+    auto *depsig = dependent_signals(self);
     depsig->trace(trc);
     depsig->traceWeak(trc);
   }
 
   auto has_algorithms = !JS::GetReservedSlot(self, Slots::Algorithms).isNullOrUndefined();
   if (has_algorithms) {
-    auto algorithms = AbortSignal::algorithms(self);
+    auto *algorithms = AbortSignal::algorithms(self);
     algorithms->trace(trc);
   }
 }
@@ -515,6 +513,6 @@ bool install(api::Engine *engine) {
 
 JSString *AbortSignal::abort_type_atom = nullptr;
 
-} // namespace abort
-} // namespace web
-} // namespace builtins
+} // namespace builtins::web::abort
+
+

builtins/web/abort/abort-signal.h

@@ -6,15 +6,22 @@
 
 #include "../event/event-target.h"
 
-namespace builtins {
-namespace web {
-namespace abort {
+
+
+namespace builtins::web::abort {
 
 struct AbortAlgorithm {
   bool virtual run(JSContext *cx) = 0;
   void virtual trace(JSTracer *trc) { };
 
+  AbortAlgorithm() = default;
   virtual ~AbortAlgorithm() = default;
+
+  AbortAlgorithm(const AbortAlgorithm &) = default;
+  AbortAlgorithm(AbortAlgorithm &&) = delete;
+
+  AbortAlgorithm &operator=(const AbortAlgorithm &) = default;
+  AbortAlgorithm &operator=(AbortAlgorithm &&) = delete;
 };
 
 class AbortSignal : public BuiltinImpl<AbortSignal, TraceableClassPolicy> {
@@ -45,7 +52,7 @@ class AbortSignal : public BuiltinImpl<AbortSignal, TraceableClassPolicy> {
 
 public:
   static constexpr int ParentSlots = event::EventTarget::Slots::Count;
-  enum Slots {
+  enum Slots : uint8_t {
     Reason = ParentSlots,
     Algorithms,
     Dependent,
@@ -81,8 +88,8 @@ class AbortSignal : public BuiltinImpl<AbortSignal, TraceableClassPolicy> {
   static void trace(JSTracer *trc, JSObject *self);
 };
 
-} // namespace abort
-} // namespace web
-} // namespace builtins
+} // namespace builtins::web::abort
+
+
 
 #endif // BUILTINS_WEB_ABORT_SIGNAL_H_

builtins/web/abort/weak-index-set.h

@@ -13,16 +13,15 @@ class WeakIndexSet {
   WeakVec items_;
 
 public:
-  WeakIndexSet() : items_() {}
+  WeakIndexSet()  = default;
 
   bool insert(JSObject* obj) {
-    auto it = std::find_if(items_.begin(), items_.end(), [&obj](const auto &item) { return item == obj; });
+    auto *it = std::find_if(items_.begin(), items_.end(), [&obj](const auto &item) { return item == obj; });
 
     if (it == items_.end()) {
       return items_.append(obj);
-    } else {
-      return true;
     }
+    return true;
   }
 
   bool remove(JSObject* obj) {
@@ -31,7 +30,7 @@ class WeakIndexSet {
   }
 
   WeakVec &items() { return items_;}
-  const WeakVec &items() const { return items_; }
+  [[nodiscard]] const WeakVec &items() const { return items_; }
 
   void trace(JSTracer* trc) { items_.trace(trc); }