Enable several more clang-tidy checks. Fix a set of 'loop variable too small' errors. (#17690)

* enable more clang-tidy checks

* Enable one more check

* More enabled tests

* Enable more checks and make codechanges to make it pass

* Restyle

* Fix compilation

* One more compile failure

* Update more tidy errors in tests

* Mock attribute storage compile fixes

* Add message to static assert

* Undo emberAfClusterCount cast to uint8_t

* More changes for emberAfClusterCount returning uint8_t

* more updates for cluster count

* Updated code for iteration over array size

* one more review update

* one more review update

* Attempt to fix some non-null asserts by tidy

* Adding one more nullable marker

* Adding one more nullable marker

* Fix nullable marker syntax for darwin compile

* Update nullability markers again, this time darwin passes locally for all bridges

* Disable constructor nullability linter for test command constructors as they seem broken

* Move nolint for nullability into darwin-tool rather than main chip-tool

Author: andy31415

.clang-tidy

@@ -4,8 +4,6 @@ Checks: >
   modernize-use-nullptr,
   bugprone-*,
   -bugprone-not-null-terminated-result,
-  -bugprone-suspicious-memory-comparison,
-  -bugprone-argument-comment,
   -bugprone-unused-return-value,
   -bugprone-branch-clone,
   -bugprone-easily-swappable-parameters,
@@ -14,9 +12,7 @@ Checks: >
   -bugprone-forward-declaration-namespace,
   -bugprone-forwarding-reference-overload,
   -bugprone-undelegated-constructor,
-  -bugprone-sizeof-expression,
   -bugprone-implicit-widening-of-multiplication-result,
-  -bugprone-too-small-loop-variable,
   -bugprone-narrowing-conversions,
   -bugprone-misplaced-widening-cast,
   -bugprone-suspicious-include,
@@ -33,10 +29,8 @@ Checks: >
   -clang-analyzer-security.insecureAPI.strcpy,
   -clang-analyzer-nullability.NullablePassedToNonnull,
   -clang-analyzer-optin.performance.Padding,
-  -clang-analyzer-unix.cstring.NullArg,
   -clang-analyzer-security.insecureAPI.rand,
   -clang-analyzer-core.NonNullParamChecker,
-  -clang-analyzer-nullability.NullPassedToNonnull,
   -clang-analyzer-unix.Malloc,
   -clang-diagnostic-implicit-int-conversion
 WarningsAsErrors: '*'

examples/chip-tool-darwin/templates/tests/partials/test_cluster.zapt

@@ -2,6 +2,7 @@
 class {{filename}}: public TestCommandBridge
 {
   public:
+    // NOLINTBEGIN(clang-analyzer-nullability.NullPassedToNonnull): Test constructor nullability not enforced
     {{#if ../credsIssuerConfigArg}}
     {{filename}}(CredentialIssuerCommands * credsIssuerConfig): TestCommand("{{filename}}", credsIssuerConfig), mTestIndex(0)
     {{else}}
@@ -16,6 +17,7 @@ class {{filename}}: public TestCommandBridge
           {{/if}}
         {{/chip_tests_config}}
     }
+    // NOLINTEND(clang-analyzer-nullability.NullPassedToNonnull)
 
     ~{{filename}}()
     {

examples/common/pigweed/rpc_services/Descriptor.h

@@ -94,7 +94,7 @@ class Descriptor : public pw_rpc::nanopb::Descriptor::Service<Descriptor>
 private:
     void ClusterList(EndpointId endpoint, bool server, ServerWriter<::chip_rpc_Cluster> & writer)
     {
-        uint16_t cluster_count = emberAfClusterCount(endpoint, server);
+        uint8_t cluster_count = emberAfClusterCount(endpoint, server);
 
         for (uint8_t cluster_index = 0; cluster_index < cluster_count; cluster_index++)
         {

examples/shell/shell_common/cmd_server.cpp

@@ -172,8 +172,9 @@ static CHIP_ERROR CmdAppServerClusters(int argc, char ** argv)
 
     for (int i = 0; i < emberAfEndpointCount(); i++)
     {
-        EndpointId endpoint   = emberAfEndpointFromIndex(i);
-        uint16_t clusterCount = emberAfClusterCount(endpoint, server);
+        EndpointId endpoint = emberAfEndpointFromIndex(i);
+
+        uint8_t clusterCount = emberAfClusterCount(endpoint, server);
 
         streamer_printf(streamer_get(), "Endpoint %d:\r\n", endpoint);
 

src/app/AttributePathExpandIterator.cpp

@@ -127,7 +127,11 @@ void AttributePathExpandIterator::PrepareAttributeIndexRange(const AttributePath
             // and overflow to 0 for the max index) to us not going through
             // non-metadata global attributes for this attribute.
             mGlobalAttributeIndex = UINT8_MAX;
-            for (uint8_t idx = 0; idx < ArraySize(GlobalAttributesNotInMetadata); ++idx)
+
+            static_assert(ArraySize(GlobalAttributesNotInMetadata) <= UINT8_MAX, "Iterating over at most 256 array entries");
+
+            const uint8_t arraySize = static_cast<uint8_t>(ArraySize(GlobalAttributesNotInMetadata));
+            for (uint8_t idx = 0; idx < arraySize; ++idx)
             {
                 if (GlobalAttributesNotInMetadata[idx] == aAttributePath.mAttributeId)
                 {

src/app/clusters/descriptor/descriptor.cpp

@@ -124,7 +124,7 @@ CHIP_ERROR DescriptorAttrAccess::ReadDeviceAttribute(EndpointId endpoint, Attrib
 CHIP_ERROR DescriptorAttrAccess::ReadClientServerAttribute(EndpointId endpoint, AttributeValueEncoder & aEncoder, bool server)
 {
     CHIP_ERROR err = aEncoder.EncodeList([&endpoint, server](const auto & encoder) -> CHIP_ERROR {
-        uint16_t clusterCount = emberAfClusterCount(endpoint, server);
+        uint8_t clusterCount = emberAfClusterCount(endpoint, server);
 
         for (uint8_t clusterIndex = 0; clusterIndex < clusterCount; clusterIndex++)
         {

src/app/clusters/door-lock-server/door-lock-server.cpp

@@ -1423,7 +1423,7 @@ bool DoorLockServer::findUserIndexByCredential(chip::EndpointId endpointId, DlCr
             continue;
         }
 
-        for (uint16_t j = 0; j < user.credentials.size(); ++j)
+        for (size_t j = 0; j < user.credentials.size(); ++j)
         {
             if (user.credentials.data()[j].CredentialIndex == credentialIndex &&
                 user.credentials.data()[j].CredentialType == to_underlying(credentialType))

src/app/clusters/ias-zone-server/ias-zone-server.cpp

@@ -628,13 +628,12 @@ static void unenrollSecurityDevice(EndpointId endpoint)
 void emberAfPluginIasZoneServerStackStatusCallback(EmberStatus status)
 {
     EndpointId endpoint;
-    uint8_t i;
 
     // If the device has left the network, unenroll all endpoints on the device
     // that are servers of the IAS Zone Cluster
     if (status == EMBER_NETWORK_DOWN && emberAfNetworkState() == EMBER_NO_NETWORK)
     {
-        for (i = 0; i < emberAfEndpointCount(); i++)
+        for (uint16_t i = 0; i < emberAfEndpointCount(); i++)
         {
             endpoint = emberAfEndpointFromIndex(i);
             if (emberAfContainsServer(endpoint, ZCL_IAS_ZONE_CLUSTER_ID))

src/app/clusters/test-cluster-server/test-cluster-server.cpp

@@ -233,7 +233,7 @@ CHIP_ERROR TestAttrAccess::WriteNullableStruct(AttributeValueDecoder & aDecoder)
 CHIP_ERROR TestAttrAccess::ReadListInt8uAttribute(AttributeValueEncoder & aEncoder)
 {
     return aEncoder.EncodeList([](const auto & encoder) -> CHIP_ERROR {
-        for (uint8_t index = 0; index < gListUint8DataLen; index++)
+        for (size_t index = 0; index < gListUint8DataLen; index++)
         {
             ReturnErrorOnFailure(encoder.Encode(gListUint8Data[index]));
         }
@@ -281,7 +281,7 @@ CHIP_ERROR TestAttrAccess::WriteListInt8uAttribute(const ConcreteDataAttributePa
 CHIP_ERROR TestAttrAccess::ReadListOctetStringAttribute(AttributeValueEncoder & aEncoder)
 {
     return aEncoder.EncodeList([](const auto & encoder) -> CHIP_ERROR {
-        for (uint8_t index = 0; index < gListOctetStringDataLen; index++)
+        for (size_t index = 0; index < gListOctetStringDataLen; index++)
         {
             ReturnErrorOnFailure(encoder.Encode(gListOctetStringData[index].AsSpan()));
         }
@@ -335,7 +335,7 @@ CHIP_ERROR TestAttrAccess::ReadListLongOctetStringAttribute(AttributeValueEncode
     // The ListOctetStringAttribute takes 512 bytes, and the whole attribute will exceed the IPv6 MTU, so we can test list chunking
     // feature with this attribute.
     return aEncoder.EncodeList([](const auto & encoder) -> CHIP_ERROR {
-        for (uint8_t index = 0; index < gListLongOctetStringLen; index++)
+        for (size_t index = 0; index < gListLongOctetStringLen; index++)
         {
             ReturnErrorOnFailure(encoder.Encode(ByteSpan(chip::Uint8::from_const_char(sLongOctetStringBuf), 512)));
         }
@@ -381,7 +381,7 @@ CHIP_ERROR TestAttrAccess::WriteListLongOctetStringAttribute(const ConcreteDataA
 CHIP_ERROR TestAttrAccess::ReadListStructOctetStringAttribute(AttributeValueEncoder & aEncoder)
 {
     return aEncoder.EncodeList([](const auto & encoder) -> CHIP_ERROR {
-        for (uint8_t index = 0; index < gListOperationalCertLen; index++)
+        for (size_t index = 0; index < gListOperationalCertLen; index++)
         {
             Structs::TestListStructOctet::Type structOctet;
             structOctet.fabricIndex     = listStructOctetStringData[index].fabricIndex;

src/app/tests/TestPendingNotificationMap.cpp

@@ -78,7 +78,7 @@ void TestAddRemove(nlTestSuite * aSuite, void * aContext)
     pendingMap.RemoveAllEntriesForNode(0, 0);
     uint8_t expectedEntryIndecies[] = { 1, 2, 3, 4, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19 };
     iter                            = pendingMap.begin();
-    for (uint8_t i = 0; i < sizeof(expectedEntryIndecies); i++)
+    for (size_t i = 0; i < sizeof(expectedEntryIndecies); i++)
     {
         PendingNotificationEntry entry = *iter;
         NL_TEST_ASSERT(aSuite, entry.mBindingEntryId == expectedEntryIndecies[i]);

src/app/util/attribute-storage.cpp

@@ -404,7 +404,7 @@ static void initializeEndpoint(EmberAfDefinedEndpoint * definedEndpoint)
 // Calls the init functions.
 void emAfCallInits(void)
 {
-    uint8_t index;
+    uint16_t index;
     for (index = 0; index < emberAfEndpointCount(); index++)
     {
         if (emberAfEndpointIndexIsEnabled(index))
@@ -553,7 +553,7 @@ EmberAfStatus emAfReadOrWriteAttribute(EmberAfAttributeSearchRecord * attRecord,
 
     uint16_t attributeOffsetIndex = 0;
 
-    for (uint8_t ep = 0; ep < emberAfEndpointCount(); ep++)
+    for (uint16_t ep = 0; ep < emberAfEndpointCount(); ep++)
     {
         // Is this a dynamic endpoint?
         bool isDynamicEndpoint = (ep >= emberAfFixedEndpointCount());
@@ -702,7 +702,7 @@ const EmberAfCluster * emberAfFindClusterInType(const EmberAfEndpointType * endp
 
 uint8_t emberAfClusterIndex(EndpointId endpoint, ClusterId clusterId, EmberAfClusterMask mask)
 {
-    for (uint8_t ep = 0; ep < emberAfEndpointCount(); ep++)
+    for (uint16_t ep = 0; ep < emberAfEndpointCount(); ep++)
     {
         // Check the endpoint id first, because that way we avoid examining the
         // endpoint type for endpoints that are not actually defined.

src/app/util/mock/attribute-storage.cpp

@@ -103,19 +103,21 @@ uint16_t emberAfEndpointCount(void)
 
 uint16_t emberAfIndexFromEndpoint(chip::EndpointId endpoint)
 {
-    for (uint16_t i = 0; i < ArraySize(endpoints); i++)
+    static_assert(ArraySize(endpoints) < UINT16_MAX, "Need to be able to return endpoint index as a 16-bit value.");
+
+    for (size_t i = 0; i < ArraySize(endpoints); i++)
     {
         if (endpoints[i] == endpoint)
         {
-            return i;
+            return static_cast<uint16_t>(i);
         }
     }
     return UINT16_MAX;
 }
 
 uint8_t emberAfClusterCount(chip::EndpointId endpoint, bool server)
 {
-    for (uint16_t i = 0; i < ArraySize(endpoints); i++)
+    for (size_t i = 0; i < ArraySize(endpoints); i++)
     {
         if (endpoints[i] == endpoint)
         {
@@ -127,9 +129,9 @@ uint8_t emberAfClusterCount(chip::EndpointId endpoint, bool server)
 
 uint16_t emberAfGetServerAttributeCount(chip::EndpointId endpoint, chip::ClusterId cluster)
 {
-    uint16_t endpointIndex          = emberAfIndexFromEndpoint(endpoint);
-    uint16_t clusterCountOnEndpoint = emberAfClusterCount(endpoint, true);
-    for (uint16_t i = 0; i < clusterCountOnEndpoint; i++)
+    uint16_t endpointIndex         = emberAfIndexFromEndpoint(endpoint);
+    uint8_t clusterCountOnEndpoint = emberAfClusterCount(endpoint, true);
+    for (uint8_t i = 0; i < clusterCountOnEndpoint; i++)
     {
         if (clusters[i + clusterIndex[endpointIndex]] == cluster)
         {
@@ -142,9 +144,9 @@ uint16_t emberAfGetServerAttributeCount(chip::EndpointId endpoint, chip::Cluster
 uint16_t emberAfGetServerAttributeIndexByAttributeId(chip::EndpointId endpoint, chip::ClusterId cluster,
                                                      chip::AttributeId attributeId)
 {
-    uint16_t endpointIndex          = emberAfIndexFromEndpoint(endpoint);
-    uint16_t clusterCountOnEndpoint = emberAfClusterCount(endpoint, true);
-    for (uint16_t i = 0; i < clusterCountOnEndpoint; i++)
+    uint16_t endpointIndex         = emberAfIndexFromEndpoint(endpoint);
+    uint8_t clusterCountOnEndpoint = emberAfClusterCount(endpoint, true);
+    for (uint8_t i = 0; i < clusterCountOnEndpoint; i++)
     {
         if (clusters[i + clusterIndex[endpointIndex]] == cluster)
         {
@@ -180,9 +182,9 @@ chip::Optional<chip::ClusterId> emberAfGetNthClusterId(chip::EndpointId endpoint
 chip::Optional<chip::AttributeId> emberAfGetServerAttributeIdByIndex(chip::EndpointId endpoint, chip::ClusterId cluster,
                                                                      uint16_t index)
 {
-    uint16_t endpointIndex          = emberAfIndexFromEndpoint(endpoint);
-    uint16_t clusterCountOnEndpoint = emberAfClusterCount(endpoint, true);
-    for (uint16_t i = 0; i < clusterCountOnEndpoint; i++)
+    uint16_t endpointIndex         = emberAfIndexFromEndpoint(endpoint);
+    uint8_t clusterCountOnEndpoint = emberAfClusterCount(endpoint, true);
+    for (uint8_t i = 0; i < clusterCountOnEndpoint; i++)
     {
         if (clusters[i + clusterIndex[endpointIndex]] == cluster)
         {
@@ -199,8 +201,8 @@ chip::Optional<chip::AttributeId> emberAfGetServerAttributeIdByIndex(chip::Endpo
 
 uint8_t emberAfClusterIndex(chip::EndpointId endpoint, chip::ClusterId cluster, EmberAfClusterMask mask)
 {
-    uint16_t endpointIndex          = emberAfIndexFromEndpoint(endpoint);
-    uint16_t clusterCountOnEndpoint = emberAfClusterCount(endpoint, true);
+    uint16_t endpointIndex         = emberAfIndexFromEndpoint(endpoint);
+    uint8_t clusterCountOnEndpoint = emberAfClusterCount(endpoint, true);
     for (uint8_t i = 0; i < clusterCountOnEndpoint; i++)
     {
         if (clusters[i + clusterIndex[endpointIndex]] == cluster)

src/app/util/util.cpp

@@ -520,7 +520,7 @@ int8_t emberAfCompareValues(const uint8_t * val1, const uint8_t * val2, uint16_t
         // no length means nothing to compare.  Shouldn't even happen, since len is sizeof(some-integer-type).
         return 0;
     }
-    uint8_t i, j, k;
+
     if (signedNumber)
     { // signed number comparison
         if (len <= 4)
@@ -529,12 +529,12 @@ int8_t emberAfCompareValues(const uint8_t * val1, const uint8_t * val2, uint16_t
             int32_t accum2 = 0x0;
             int32_t all1s  = -1;
 
-            for (i = 0; i < len; i++)
+            for (uint16_t i = 0; i < len; i++)
             {
-                j = (val1 == nullptr ? 0 : (EM_BIG_ENDIAN ? val1[i] : val1[(len - 1) - i]));
+                uint8_t j = (val1 == nullptr ? 0 : (EM_BIG_ENDIAN ? val1[i] : val1[(len - 1) - i]));
                 accum1 |= j << (8 * (len - 1 - i));
 
-                k = (EM_BIG_ENDIAN ? val2[i] : val2[(len - 1) - i]);
+                uint8_t k = (EM_BIG_ENDIAN ? val2[i] : val2[(len - 1) - i]);
                 accum2 |= k << (8 * (len - 1 - i));
             }
 
@@ -568,10 +568,10 @@ int8_t emberAfCompareValues(const uint8_t * val1, const uint8_t * val2, uint16_t
     }
 
     // regular unsigned number comparison
-    for (i = 0; i < len; i++)
+    for (uint16_t i = 0; i < len; i++)
     {
-        j = (val1 == nullptr ? 0 : (EM_BIG_ENDIAN ? val1[i] : val1[(len - 1) - i]));
-        k = (EM_BIG_ENDIAN ? val2[i] : val2[(len - 1) - i]);
+        uint8_t j = (val1 == nullptr ? 0 : (EM_BIG_ENDIAN ? val1[i] : val1[(len - 1) - i]));
+        uint8_t k = (EM_BIG_ENDIAN ? val2[i] : val2[(len - 1) - i]);
 
         if (j > k)
         {

src/credentials/CHIPCert.cpp

@@ -1233,7 +1233,7 @@ CHIP_ERROR ExtractCATsFromOpCert(const ChipCertificateData & opcert, CATValues &
             cats.values[catCount++] = static_cast<CASEAuthTag>(rdn.mChipVal);
         }
     }
-    for (uint8_t i = catCount; i < cats.size(); ++i)
+    for (size_t i = catCount; i < cats.size(); ++i)
     {
         cats.values[i] = kUndefinedCAT;
     }

src/darwin/Framework/CHIP/CHIPAttestationTrustStoreBridge.h

@@ -30,7 +30,7 @@ class CHIPAttestationTrustStoreBridge : public chip::Credentials::AttestationTru
         const chip::ByteSpan & skid, chip::MutableByteSpan & outPaaDerBuffer) const override;
 
 private:
-    NSArray<NSData *> * mPaaCerts;
+    NSArray<NSData *> * _Nullable mPaaCerts;
 };
 
 NS_ASSUME_NONNULL_END

src/darwin/Framework/CHIP/CHIPDevicePairingDelegateBridge.h

@@ -40,8 +40,8 @@ class CHIPDevicePairingDelegateBridge : public chip::Controller::DevicePairingDe
     void OnCommissioningComplete(chip::NodeId deviceId, CHIP_ERROR error) override;
 
 private:
-    id<CHIPDevicePairingDelegate> mDelegate;
-    dispatch_queue_t mQueue;
+    _Nullable id<CHIPDevicePairingDelegate> mDelegate;
+    _Nullable dispatch_queue_t mQueue;
 
     CHIPPairingStatus MapStatus(chip::Controller::DevicePairingDelegate::Status status);
 };

src/darwin/Framework/CHIP/CHIPP256KeypairBridge.h

@@ -49,7 +49,7 @@ class CHIPP256KeypairBridge : public chip::Crypto::P256KeypairBase
     const chip::Crypto::P256PublicKey & Pubkey() const override { return mPubkey; };
 
 private:
-    id<CHIPKeypair> mKeypair;
+    id<CHIPKeypair> _Nullable mKeypair;
     chip::Crypto::P256PublicKey mPubkey;
 
     CHIP_ERROR setPubkey();

src/darwin/Framework/CHIP/CHIPPersistentStorageDelegateBridge.h

@@ -35,9 +35,8 @@ class CHIPPersistentStorageDelegateBridge : public chip::PersistentStorageDelega
     CHIP_ERROR SyncDeleteKeyValue(const char * key) override;
 
 private:
-    id<CHIPPersistentStorageDelegate> mDelegate;
-
-    dispatch_queue_t mWorkQueue;
+    _Nullable id<CHIPPersistentStorageDelegate> mDelegate;
+    _Nullable dispatch_queue_t mWorkQueue;
 };
 
 NS_ASSUME_NONNULL_END

src/lib/dnssd/tests/TestTxtFields.cpp

@@ -232,7 +232,7 @@ void TestGetRotatingDeviceId(nlTestSuite * inSuite, void * inContext)
     strcpy(ri, "000102030405060708090A0B0C0D0E0F101112131415161718191A1B1C1D1E1F202122232425262728292A2B2C2D2E2F3031");
     GetRotatingDeviceId(GetSpan(ri), id, &len);
     NL_TEST_ASSERT(inSuite, len == sizeof(id));
-    for (uint8_t i = 0; i < sizeof(id); ++i)
+    for (size_t i = 0; i < sizeof(id); ++i)
     {
         NL_TEST_ASSERT(inSuite, id[i] == i);
     }

src/lib/shell/streamer.cpp

@@ -57,6 +57,7 @@ ssize_t ENFORCE_FORMAT(2, 0) streamer_vprintf(streamer_t * self, const char * fm
 
     // vsnprintf doesn't return negative numbers as long as the length it's
     // passed fits in INT_MAX.
+    // NOLINTNEXTLINE(bugprone-sizeof-expression)
     static_assert(sizeof(buf) <= INT_MAX, "Return value cast not valid");
     len = static_cast<unsigned int>(vsnprintf(buf, sizeof(buf), fmt, ap));
     if (len >= sizeof(buf))

src/lib/support/tests/TestFixedBufferAllocator.cpp

@@ -36,13 +36,17 @@ void TestClone(nlTestSuite * inSuite, void * inContext)
 
     NL_TEST_ASSERT(inSuite, allocatedString != nullptr);
     NL_TEST_ASSERT(inSuite, allocatedString != kTestString);
+
+    // NOLINTNEXTLINE(clang-analyzer-unix.cstring.NullArg): null check for allocated string already done
     NL_TEST_ASSERT(inSuite, strcmp(allocatedString, kTestString) == 0);
 
     const uint8_t kTestData[]     = { 0xDE, 0xAD, 0xBE, 0xEF };
     const uint8_t * allocatedData = alloc.Clone(kTestData, sizeof(kTestData));
 
     NL_TEST_ASSERT(inSuite, allocatedData != nullptr);
     NL_TEST_ASSERT(inSuite, allocatedData != kTestData);
+
+    // NOLINTNEXTLINE(clang-analyzer-unix.cstring.NullArg): null check for allocated data already done
     NL_TEST_ASSERT(inSuite, memcmp(allocatedData, kTestData, sizeof(kTestData)) == 0);
 }
 

src/setup_payload/Base38Encode.cpp

@@ -55,7 +55,7 @@ CHIP_ERROR base38Encode(ByteSpan in_buf, MutableCharSpan & out_buf)
 
         size_t bytesInChunk = (in_buf_len >= kMaxBytesSingleChunkLen) ? kMaxBytesSingleChunkLen : in_buf_len;
 
-        for (uint8_t byte_idx = 0; byte_idx < bytesInChunk; byte_idx++)
+        for (size_t byte_idx = 0; byte_idx < bytesInChunk; byte_idx++)
         {
             value += static_cast<uint32_t>(in_buf_ptr[byte_idx] << (8 * byte_idx));
         }