lint: Move assertion linter into lint runner #31435

pull maflcko wants to merge 2 commits into bitcoin:master from maflcko:2412-lint-assert changing 2 files +84 −72
  1. maflcko commented at 11:26 am on December 6, 2024: member

    On failure, this makes the output more consistent with the other linters. Each failure will be marked with an ‘⚠️ ’ emoji and explanation, making it easier to spot.

    Also, add –line-number to the filesystem linter.

    Also, add newlines after each failing check, to visually separate different failures from each other.

    Can be reviewed with: --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

  2. DrahtBot commented at 11:26 am on December 6, 2024: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31435.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK hodlinator, davidgumberg, TheCharlatan

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  3. DrahtBot added the label Tests on Dec 6, 2024
  4. in test/lint/test_runner/src/main.rs:354 in faa0b09a4b outdated
    349+        .expect("command error")
    350+        .success();
    351+    if found {
    352+        Err(r#"
    353+^^^
    354+BOOST_ASSERT must be replaced with Assert, BOOST_REQUIRE, or BOOST_CHECK to avoid an unessecary
    


    hodlinator commented at 4:52 pm on December 12, 2024:
    0BOOST_ASSERT must be replaced with Assert, BOOST_REQUIRE, or BOOST_CHECK to avoid an unnecessary
    

    maflcko commented at 8:52 am on December 13, 2024:
    thx, pushed your branch!

    hodlinator commented at 4:34 pm on December 13, 2024:
    Glad you liked my commit! Unfortunately I’m currently operating on an unfamiliar keyboard, slipping my own “ĺint”-typo into the commit message. :upside_down_face: Feel free to replace with e8f0e6efaf555a7d17bdb118464bd572bd5f3933, or not.

    maflcko commented at 4:41 pm on December 13, 2024:
    sure, done
  5. hodlinator changes_requested
  6. hodlinator commented at 6:59 pm on December 12, 2024: contributor

    Concept ACK faa0b09a4b6267399a214d7f5beaf4dfe9f3976c

    Good call on integrating more of the comments from the old linter into the error messages.

    Tested adding assert()/CHECK_NONFATAL()/BOOST_ASSERT() with expected results in src/rpc/mining.cpp.

    Might as well avoid introducing the typo (see inline comment). That’s the only light blocker for me.


    nit: The whitespace changes are in the right direction, but I think they are still unclear with the current change.

    faa0b09a4b6267399a214d7f5beaf4dfe9f3976c

     0...
     1All checks passed!
     2src/rpc/mining.cpp:127:    BOOST_ASSERT(true);
     3src/rpc/mining.cpp:137:    BOOST_ASSERT(true);
     4
     5^^^
     6BOOST_ASSERT must be replaced with Assert, BOOST_REQUIRE, or BOOST_CHECK to avoid an unessecary
     7include of the boost/assert.hpp dependency.
     8            
     9^---- ⚠️ Failure generated from lint check 'boost_assert'!
    10Check that boost assertions are not used
    11
    12
    13src/crc32c in HEAD currently refers to tree 454691a9b89ee8b9e1f71a48a7398edba49c3805
    14...
    

    With additional changes (7973da4cb9b7a6eaae7dfc1d3d681d9b75c5a226)

    • No empty line separating errors and arrows ("^^^"). Keeping them together signals they are related.
    • No empty line separating error message and linter failure line (not completely empty, it contains several spaces left over from Rust multi-line literal).
    • Keep the linter description on the same line as the failure line, otherwise it looks like it’s a description for the following step.
     0...
     1All checks passed!
     2src/rpc/mining.cpp:127:    BOOST_ASSERT(true);
     3src/rpc/mining.cpp:137:    BOOST_ASSERT(true);
     4^^^
     5BOOST_ASSERT must be replaced with Assert, BOOST_REQUIRE, or BOOST_CHECK to avoid an unessecary
     6include of the boost/assert.hpp dependency.
     7^---- ⚠️ Failure generated from lint check 'boost_assert' (Check that boost assertions are not used)!
     8
     9
    10src/crc32c in HEAD currently refers to tree 454691a9b89ee8b9e1f71a48a7398edba49c3805
    11...
    
  7. lint: Move assertion linter into lint runner
    On failure, this makes the output more consistent with the other linter.
    Each failure will be marked with an '⚠️ ' emoji and explanation, making
    it easier to spot.
    
    Also, add --line-number to the filesystem linter.
    
    Also, add newlines after each failing check, to visually separate
    different failures from each other.
    
    Can be reviewed with:
    "--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space"
    fa9aacf614
  8. maflcko force-pushed on Dec 13, 2024
  9. lint: output-only - Avoid repeated arrows, trim
    - No empty line separating errors and arrows ("^^^"). Keeping them together signals they are related.
    - No empty line separating error message and linter failure line (not completely empty, it contains several spaces left over from Rust multi-line literal).
    - Keep the linter description on the same line as the failure line, otherwise it looks like it's a description for the following step.
    e8f0e6efaf
  10. hodlinator approved
  11. hodlinator commented at 4:35 pm on December 13, 2024: contributor
    ACK fcf0e873e3194158d718db107386f94f8e8e4a16
  12. maflcko force-pushed on Dec 13, 2024
  13. hodlinator approved
  14. hodlinator commented at 4:49 pm on December 13, 2024: contributor
    re-ACK e8f0e6efaf555a7d17bdb118464bd572bd5f3933
  15. davidgumberg commented at 2:00 am on December 24, 2024: contributor

    crACK https://github.com/bitcoin/bitcoin/commit/e8f0e6efaf555a7d17bdb118464bd572bd5f3933

    Tested manually with the following diff:

     0diff --git a/src/coins.cpp b/src/coins.cpp
     1index 24a102b0bc..000068c7e2 100644
     2--- a/src/coins.cpp
     3+++ b/src/coins.cpp
     4@@ -21,6 +21,7 @@ std::unique_ptr<CCoinsViewCursor> CCoinsView::Cursor() const { return nullptr; }
     5
     6 bool CCoinsView::HaveCoin(const COutPoint &outpoint) const
     7 {
     8+    BOOST_ASSERT(1);
     9     return GetCoin(outpoint).has_value();
    10 }
    11
    12diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
    13index 60828c1711..0199be420b 100644
    14--- a/src/rpc/mining.cpp
    15+++ b/src/rpc/mining.cpp
    16@@ -133,6 +133,7 @@ static RPCHelpMan getnetworkhashps()
    17
    18 static bool GenerateBlock(ChainstateManager& chainman, Mining& miner, CBlock&& block, uint64_t& max_tries, std::shared_ptr<const CBlock>& block_out, bool process_new_block)
    19 {
    20+    assert(1);
    21     block_out.reset();
    22     block.hashMerkleRoot = BlockMerkleRoot(block);
    23
    24diff --git a/src/rpc/server_util.cpp b/src/rpc/server_util.cpp
    25index 0387cbb8e2..16b9827e13 100644
    26--- a/src/rpc/server_util.cpp
    27+++ b/src/rpc/server_util.cpp
    28@@ -20,6 +20,7 @@ using node::NodeContext;
    29
    30 NodeContext& EnsureAnyNodeContext(const std::any& context)
    31 {
    32+    Assert(true);
    33     auto node_context = util::AnyPtr<NodeContext>(context);
    34     if (!node_context) {
    35         throw JSONRPCError(RPC_INTERNAL_ERROR, "Node context not found");
    36diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp
    37index f1430a3c60..079c9e0644 100644
    38--- a/src/wallet/rpc/coins.cpp
    39+++ b/src/wallet/rpc/coins.cpp
    40@@ -467,6 +467,7 @@ RPCHelpMan getbalances()
    41     const auto bal = GetBalance(wallet);
    42     UniValue balances{UniValue::VOBJ};
    43     {
    44+        Assume(0);
    45         UniValue balances_mine{UniValue::VOBJ};
    46         balances_mine.pushKV("trusted", ValueFromAmount(bal.m_mine_trusted));
    47         balances_mine.pushKV("untrusted_pending", ValueFromAmount(bal.m_mine_untrusted_pending));
    

    and got the expected output:

     0$ cargo run -- --lint=rpc_assert --lint=boost_assert
     1src/rpc/mining.cpp:136:    assert(1);
     2src/rpc/server_util.cpp:23:    Assert(true);
     3src/wallet/rpc/coins.cpp:470:        Assume(0);
     4^^^
     5CHECK_NONFATAL(condition) or NONFATAL_UNREACHABLE should be used instead of assert for RPC code.
     6
     7Aborting the whole process is undesirable for RPC code. So nonfatal
     8checks should be used over assert. See: src/util/check.h
     9^---- ⚠️  Failure generated from lint check 'rpc_assert' (Check that fatal assertions are not used in RPC code)!
    10
    11
    12src/coins.cpp:24:    BOOST_ASSERT(1);
    13^^^
    14BOOST_ASSERT must be replaced with Assert, BOOST_REQUIRE, or BOOST_CHECK to avoid an unnecessary
    15include of the boost/assert.hpp dependency.
    16^---- ⚠️  Failure generated from lint check 'boost_assert' (Check that boost assertions are not used)!
    
  16. TheCharlatan approved
  17. TheCharlatan commented at 8:19 pm on January 5, 2025: contributor
    ACK e8f0e6efaf555a7d17bdb118464bd572bd5f3933
  18. maflcko assigned fanquake on Jan 6, 2025
  19. fanquake merged this on Jan 6, 2025
  20. fanquake closed this on Jan 6, 2025


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-01-07 00:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me