treewide: fix a bunch of lints

Fixes:
- Identifiers starting with _ are prohibited
- Some driveby header dependency cleaning which wound up with doing some
  extra fixups.
- Fucking C style casts, man. C++ made these 1000% worse by letting you
  also do memory corruption with them with references.
  - Remove casts to Expr * where ExprBlackHole is an incomplete type by
    introducing an explicitly-cast eBlackHoleAddr as Expr *.
  - An incredibly illegal cast of the text bytes of the StorePath hash
    into a size_t directly. You can't DO THAT.

    Replaced with actually parsing the hash so we get 100% of the bits
    being entropy, then memcpying the start of the hash. If this shows
    up in a profile we should just make the hash parser faster with a
    lookup table or something sensible like that.
  - This horrendous bit of UB which I thankfully slapped a deprecation
    warning on, built, and it didn't trigger anywhere so it was dead
    code and I just deleted it. But holy crap you *cannot* do that.

    inline void mkString(const Symbol & s)
    {
        mkString(((const std::string &) s).c_str());
    }
- Some wrong lints. Lots of wrong macro lints, one wrong
  suspicious-sizeof lint triggered by the template being instantiated
  with only pointers, but the calculation being correct for both
  pointers and not-pointers.
- Exceptions in destructors strike again. I tried to catch the
  exceptions that might actually happen rather than all the exceptions
  imaginable. We can let the runtime hard-kill it on other exceptions
  imo.

Change-Id: I71761620846cba64d66ee7ca231b20c061e69710

src/libcmd/built-path.hh

@@ -20,13 +20,15 @@ struct SingleBuiltPathBuilt {
     DECLARE_CMP(SingleBuiltPathBuilt);
 };
 
-using _SingleBuiltPathRaw = std::variant<
+namespace built_path::detail {
+using SingleBuiltPathRaw = std::variant<
     DerivedPathOpaque,
     SingleBuiltPathBuilt
 >;
+}
 
-struct SingleBuiltPath : _SingleBuiltPathRaw {
+struct SingleBuiltPath : built_path::detail::SingleBuiltPathRaw {
-    using Raw = _SingleBuiltPathRaw;
+    using Raw = built_path::detail::SingleBuiltPathRaw;
     using Raw::Raw;
 
     using Opaque = DerivedPathOpaque;
@@ -65,17 +67,19 @@ struct BuiltPathBuilt {
     DECLARE_CMP(BuiltPathBuilt);
 };
 
-using _BuiltPathRaw = std::variant<
+namespace built_path::detail {
+using BuiltPathRaw = std::variant<
     DerivedPath::Opaque,
     BuiltPathBuilt
 >;
+}
 
 /**
  * A built path. Similar to a DerivedPath, but enriched with the corresponding
  * output path(s).
  */
-struct BuiltPath : _BuiltPathRaw {
+struct BuiltPath : built_path::detail::BuiltPathRaw {
-    using Raw = _BuiltPathRaw;
+    using Raw = built_path::detail::BuiltPathRaw;
     using Raw::Raw;
 
     using Opaque = DerivedPathOpaque;

src/libexpr/eval-error.hh

@@ -1,9 +1,8 @@
 #pragma once
 ///@file
 
-#include <algorithm>
-
 #include "error.hh"
+#include "types.hh"
 #include "pos-idx.hh"
 
 namespace nix {

src/libexpr/eval-inline.hh

@@ -31,7 +31,7 @@ Value * EvalState::allocValue()
 #endif
 
     nrValues++;
-    return (Value *) p;
+    return static_cast<Value *>(p);
 }
 
 
@@ -54,10 +54,10 @@ Env & EvalState::allocEnv(size_t size)
         void * p = *env1AllocCache;
         *env1AllocCache = GC_NEXT(p);
         GC_NEXT(p) = nullptr;
-        env = (Env *) p;
+        env = static_cast<Env *>(p);
     } else
 #endif
-        env = (Env *) gcAllocBytes(sizeof(Env) + size * sizeof(Value *));
+        env = static_cast<Env *>(gcAllocBytes(sizeof(Env) + size * sizeof(Value *)));
 
     /* We assume that env->values has been cleared by the allocator; maybeThunk() and lookupVar fromWith expect this. */
 

src/libexpr/gc-alloc.hh

@@ -120,6 +120,7 @@ inline T * gcAllocType(size_t howMany = 1)
     // However, people can and do request zero sized allocations, so we need
     // to check that neither of our multiplicands were zero before complaining
     // about it.
+    // NOLINTNEXTLINE(bugprone-sizeof-expression): yeah we only seem to alloc pointers with this. the calculation *is* correct though!
     auto checkedSz = checked::Checked<size_t>(howMany) * sizeof(T);
     size_t sz = checkedSz.valueWrapping();
     if (checkedSz.overflowed()) {

src/libexpr/nixexpr.cc

@@ -11,6 +11,7 @@
 namespace nix {
 
 ExprBlackHole eBlackHole;
+Expr *eBlackHoleAddr = &eBlackHole;
 
 // FIXME: remove, because *symbols* are abstract and do not have a single
 //        textual representation; see printIdentifier()

src/libexpr/value.hh

@@ -136,7 +136,9 @@ class ExternalValueBase
 
 std::ostream & operator << (std::ostream & str, const ExternalValueBase & v);
 
-extern ExprBlackHole eBlackHole;
+/** This is just the address of eBlackHole. It exists because eBlackHole has an
+ * incomplete type at usage sites so is not possible to cast. */
+extern Expr *eBlackHoleAddr;
 
 struct NewValueAs
 {
@@ -196,6 +198,7 @@ private:
 public:
 
     // Discount `using NewValueAs::*;`
+// NOLINTNEXTLINE(bugprone-macro-parentheses)
 #define USING_VALUETYPE(name) using name = NewValueAs::name
     USING_VALUETYPE(integer_t);
     USING_VALUETYPE(floating_t);
@@ -473,7 +476,7 @@ public:
     /// Constructs an evil thunk, whose evaluation represents infinite recursion.
     explicit Value(blackhole_t)
         : internalType(tThunk)
-        , thunk({ .env = nullptr, .expr = reinterpret_cast<Expr *>(&eBlackHole) })
+        , thunk({ .env = nullptr, .expr = eBlackHoleAddr })
     { }
 
     Value(Value const & rhs) = default;
@@ -513,7 +516,10 @@ public:
     // type() == nThunk
     inline bool isThunk() const { return internalType == tThunk; };
     inline bool isApp() const { return internalType == tApp; };
-    inline bool isBlackhole() const;
+    inline bool isBlackhole() const
+    {
+        return internalType == tThunk && thunk.expr == eBlackHoleAddr;
+    }
 
     // type() == nFunction
     inline bool isLambda() const { return internalType == tLambda; };
@@ -669,11 +675,6 @@ public:
 
     void mkStringMove(const char * s, const NixStringContext & context);
 
-    inline void mkString(const Symbol & s)
-    {
-        mkString(((const std::string &) s).c_str());
-    }
-
     void mkPath(const SourcePath & path);
 
     inline void mkPath(const char * path)
@@ -732,7 +733,11 @@ public:
         lambda.fun = f;
     }
 
-    inline void mkBlackhole();
+    inline void mkBlackhole()
+    {
+        internalType = tThunk;
+        thunk.expr = eBlackHoleAddr;
+    }
 
     void mkPrimOp(PrimOp * p);
 
@@ -832,18 +837,6 @@ public:
     }
 };
 
-
-bool Value::isBlackhole() const
-{
-    return internalType == tThunk && thunk.expr == (Expr*) &eBlackHole;
-}
-
-void Value::mkBlackhole()
-{
-    internalType = tThunk;
-    thunk.expr = (Expr*) &eBlackHole;
-}
-
 using ValueVector = GcVector<Value *>;
 using ValueMap = GcMap<Symbol, Value *>;
 using ValueVectorMap = std::map<Symbol, ValueVector>;

src/libstore/common-protocol-impl.hh

@@ -20,6 +20,7 @@ namespace nix {
     { \
         return LengthPrefixedProtoHelper<CommonProto, T >::read(store, conn); \
     } \
+    /* NOLINTNEXTLINE(bugprone-macro-parentheses) */ \
     TEMPLATE [[nodiscard]] WireFormatGenerator CommonProto::Serialise< T >::write(const Store & store, CommonProto::WriteConn conn, const T & t) \
     { \
         return LengthPrefixedProtoHelper<CommonProto, T >::write(store, conn, t); \

src/libstore/derived-path.hh

@@ -78,10 +78,12 @@ struct SingleDerivedPathBuilt {
     DECLARE_CMP(SingleDerivedPathBuilt);
 };
 
-using _SingleDerivedPathRaw = std::variant<
+namespace derived_path::detail {
+using SingleDerivedPathRaw = std::variant<
     DerivedPathOpaque,
     SingleDerivedPathBuilt
 >;
+}
 
 /**
  * A "derived path" is a very simple sort of expression (not a Nix
@@ -94,8 +96,8 @@ using _SingleDerivedPathRaw = std::variant<
  * - built, in which case it is a pair of a derivation path and an
  *   output name.
  */
-struct SingleDerivedPath : _SingleDerivedPathRaw {
+struct SingleDerivedPath : derived_path::detail::SingleDerivedPathRaw {
-    using Raw = _SingleDerivedPathRaw;
+    using Raw = derived_path::detail::SingleDerivedPathRaw;
     using Raw::Raw;
 
     using Opaque = DerivedPathOpaque;
@@ -201,10 +203,12 @@ struct DerivedPathBuilt {
     DECLARE_CMP(DerivedPathBuilt);
 };
 
-using _DerivedPathRaw = std::variant<
+namespace derived_path::detail {
+using DerivedPathRaw = std::variant<
     DerivedPathOpaque,
     DerivedPathBuilt
 >;
+}
 
 /**
  * A "derived path" is a very simple sort of expression that evaluates
@@ -216,8 +220,8 @@ using _DerivedPathRaw = std::variant<
  * - built, in which case it is a pair of a derivation path and some
  *   output names.
  */
-struct DerivedPath : _DerivedPathRaw {
+struct DerivedPath : derived_path::detail::DerivedPathRaw {
-    using Raw = _DerivedPathRaw;
+    using Raw = derived_path::detail::DerivedPathRaw;
     using Raw::Raw;
 
     using Opaque = DerivedPathOpaque;

src/libstore/length-prefixed-protocol-helper.hh

@@ -61,9 +61,9 @@ template<class Inner, typename... Ts>
 LENGTH_PREFIXED_PROTO_HELPER(Inner, std::tuple<Ts...>);
 
 template<class Inner, typename K, typename V>
-#define _X std::map<K, V>
+#define DONT_SUBSTITUTE_KV_TYPE std::map<K, V>
-LENGTH_PREFIXED_PROTO_HELPER(Inner, _X);
+LENGTH_PREFIXED_PROTO_HELPER(Inner, DONT_SUBSTITUTE_KV_TYPE);
-#undef _X
+#undef DONT_SUBSTITUTE_KV_TYPE
 
 template<class Inner, typename T>
 std::vector<T>

src/libstore/path.cc

@@ -112,3 +112,13 @@ PathSet Store::printStorePathSet(const StorePathSet & paths) const
 }
 
 }
+
+std::size_t std::hash<nix::StorePath>::operator()(const nix::StorePath & path) const noexcept
+{
+    // It's already a cryptographic hash of 160 bits (assuming that nobody gives us bogus ones...), so just parse it.
+    auto h = nix::Hash::parseNonSRIUnprefixed(path.hashPart(), nix::HashType::SHA1);
+    // This need not be stable across machines, so bit casting the start of it is fine.
+    size_t r;
+    memcpy(&r, h.hash, sizeof(r));
+    return r;
+}

src/libstore/path.hh

@@ -2,8 +2,9 @@
 ///@file
 
 #include <string_view>
+#include <string>
 
-#include "types.hh"
+#include "types.hh" // IWYU pragma: keep
 
 namespace nix {
 
@@ -89,10 +90,7 @@ const std::string drvExtension = ".drv";
 namespace std {
 
 template<> struct hash<nix::StorePath> {
-    std::size_t operator()(const nix::StorePath & path) const noexcept
+    std::size_t operator()(const nix::StorePath & path) const noexcept;
-    {
-        return * (std::size_t *) path.to_string().data();
-    }
 };
 
 }

src/libstore/pathlocks.hh

@@ -49,8 +49,12 @@ struct FdLock
 
     ~FdLock()
     {
-        if (acquired)
+        try {
-            lockFile(fd, ltNone, false);
+            if (acquired)
+                lockFile(fd, ltNone, false);
+        } catch (SysError &) {
+            ignoreException();
+        }
     }
 };
 

src/libstore/serve-protocol-impl.hh

@@ -20,6 +20,7 @@ namespace nix {
     { \
         return LengthPrefixedProtoHelper<ServeProto, T >::read(store, conn); \
     } \
+    /* NOLINTNEXTLINE(bugprone-macro-parentheses) */ \
     TEMPLATE [[nodiscard]] WireFormatGenerator ServeProto::Serialise< T >::write(const Store & store, ServeProto::WriteConn conn, const T & t) \
     { \
         return LengthPrefixedProtoHelper<ServeProto, T >::write(store, conn, t); \

src/libstore/sqlite.hh

@@ -1,9 +1,9 @@
 #pragma once
 ///@file
 
-#include <functional>
 #include <string>
 
+#include "types.hh"
 #include "error.hh"
 
 struct sqlite3;

src/libstore/worker-protocol-impl.hh

@@ -20,6 +20,7 @@ namespace nix {
     { \
         return LengthPrefixedProtoHelper<WorkerProto, T >::read(store, conn); \
     } \
+    /* NOLINTNEXTLINE(bugprone-macro-parentheses) */ \
     TEMPLATE [[nodiscard]] WireFormatGenerator WorkerProto::Serialise< T >::write(const Store & store, WorkerProto::WriteConn conn, const T & t) \
     { \
         return LengthPrefixedProtoHelper<WorkerProto, T >::write(store, conn, t); \

src/libutil/error.hh

@@ -16,16 +16,12 @@
  */
 
 #include "suggestions.hh"
-#include "ref.hh"
-#include "types.hh"
 #include "fmt.hh"
 
 #include <cstring>
 #include <list>
 #include <memory>
-#include <map>
 #include <optional>
-#include <compare>
 
 #include <sys/types.h>
 #include <sys/stat.h>
@@ -173,6 +169,7 @@ public:
 };
 
 #define MakeError(newClass, superClass) \
+    /* NOLINTNEXTLINE(bugprone-macro-parentheses) */    \
     class newClass : public superClass                  \
     {                                                   \
     public:                                             \

src/libutil/finally.hh

@@ -22,6 +22,7 @@ public:
     Finally(Finally &&other) : fun(std::move(other.fun)) {
         other.movedFrom = true;
     }
+    // NOLINTNEXTLINE(bugprone-exception-escape): the noexcept is declared properly here, the analysis seems broken
     ~Finally() noexcept(noexcept(fun()))
     {
         try {

src/libutil/hash.cc

@@ -7,6 +7,7 @@
 #include "args.hh"
 #include "hash.hh"
 #include "archive.hh"
+#include "charptr-cast.hh"
 #include "logging.hh"
 #include "split.hh"
 
@@ -129,7 +130,7 @@ std::string Hash::to_string(Base base, bool includeType) const
         break;
     case Base::Base64:
     case Base::SRI:
-        s += base64Encode(std::string_view(reinterpret_cast<const char *>(hash), hashSize));
+        s += base64Encode(std::string_view(charptr_cast<const char *>(hash), hashSize));
         break;
     }
     return s;

src/libutil/serialise.cc

@@ -1,4 +1,5 @@
 #include "serialise.hh"
+#include "charptr-cast.hh"
 #include "signals.hh"
 
 #include <cstring>

src/libutil/serialise.hh

@@ -4,6 +4,7 @@
 #include <concepts>
 #include <memory>
 
+#include "charptr-cast.hh"
 #include "generator.hh"
 #include "strings.hh"
 #include "types.hh"
@@ -385,7 +386,7 @@ struct SerializingTransform
         buf[5] = (n >> 40) & 0xff;
         buf[6] = (n >> 48) & 0xff;
         buf[7] = (unsigned char) (n >> 56) & 0xff;
-        return {reinterpret_cast<const char *>(buf.begin()), 8};
+        return {charptr_cast<const char *>(buf.begin()), 8};
     }
 
     static Bytes padding(size_t unpadded)
@@ -417,6 +418,9 @@ struct SerializingTransform
 
 void writePadding(size_t len, Sink & sink);
 
+// NOLINTBEGIN(cppcoreguidelines-avoid-capturing-lambda-coroutines):
+// These coroutines do their entire job before the semicolon and are not
+// retained, so they live long enough.
 inline Sink & operator<<(Sink & sink, uint64_t u)
 {
     return sink << [&]() -> WireFormatGenerator { co_yield u; }();
@@ -441,6 +445,7 @@ inline Sink & operator<<(Sink & sink, const Error & ex)
 {
     return sink << [&]() -> WireFormatGenerator { co_yield ex; }();
 }
+// NOLINTEND(cppcoreguidelines-avoid-capturing-lambda-coroutines)
 
 MakeError(SerialisationError, Error);
 
@@ -448,7 +453,7 @@ template<typename T>
 T readNum(Source & source)
 {
     unsigned char buf[8];
-    source((char *) buf, sizeof(buf));
+    source(charptr_cast<char *>(buf), sizeof(buf));
 
     auto n = readLittleEndian<uint64_t>(buf);
 
@@ -540,13 +545,17 @@ struct FramedSource : Source
 
     ~FramedSource()
     {
-        if (!eof) {
+        try {
-            while (true) {
+            if (!eof) {
-                auto n = readInt(from);
+                while (true) {
-                if (!n) break;
+                    auto n = readInt(from);
-                std::vector<char> data(n);
+                    if (!n) break;
-                from(data.data(), n);
+                    std::vector<char> data(n);
+                    from(data.data(), n);
+                }
             }
+        } catch (...) {
+            ignoreException();
         }
     }
 

src/libutil/signals.cc

@@ -3,6 +3,7 @@
 #include "sync.hh"
 #include "terminal.hh"
 
+#include <map>
 #include <thread>
 
 namespace nix {

src/libutil/thread-pool.hh

@@ -4,6 +4,7 @@
 #include "error.hh"
 #include "sync.hh"
 
+#include <map>
 #include <queue>
 #include <functional>
 #include <thread>

src/libutil/url.hh

@@ -2,6 +2,7 @@
 ///@file
 
 #include "error.hh"
+#include <map>
 
 namespace nix {
 

src/libutil/variant-wrapper.hh

@@ -8,6 +8,7 @@
  * Force the default versions of all constructors (copy, move, copy
  * assignment).
  */
+// NOLINTBEGIN(bugprone-macro-parentheses)
 #define FORCE_DEFAULT_CONSTRUCTORS(CLASS_NAME) \
     CLASS_NAME(const CLASS_NAME &) = default; \
     CLASS_NAME(CLASS_NAME &) = default; \
@@ -15,6 +16,7 @@
     \
     CLASS_NAME & operator =(const CLASS_NAME &) = default; \
     CLASS_NAME & operator =(CLASS_NAME &) = default;
+// NOLINTEND(bugprone-macro-parentheses)
 
 /**
  * Make a wrapper constructor. All args are forwarded to the

tests/unit/libutil/url.cc

@@ -1,4 +1,5 @@
 #include "url.hh"
+#include "types.hh"
 #include <gtest/gtest.h>
 
 namespace nix {