From fa9aacf614f6066ff3b5c02f0daaaeb0ebb93f33 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Fri, 6 Dec 2024 09:55:54 +0100 Subject: [PATCH] lint: Move assertion linter into lint runner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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" --- test/lint/lint-assertions.py | 54 ----------------------- test/lint/test_runner/src/main.rs | 71 ++++++++++++++++++++++++++++++- 2 files changed, 69 insertions(+), 56 deletions(-) delete mode 100755 test/lint/lint-assertions.py diff --git a/test/lint/lint-assertions.py b/test/lint/lint-assertions.py deleted file mode 100755 index 5d01b13fd4..0000000000 --- a/test/lint/lint-assertions.py +++ /dev/null @@ -1,54 +0,0 @@ -#!/usr/bin/env python3 -# -# Copyright (c) 2018-2022 The Bitcoin Core developers -# Distributed under the MIT software license, see the accompanying -# file COPYING or http://www.opensource.org/licenses/mit-license.php. -# -# Check for assertions with obvious side effects. - -import sys -import subprocess - - -def git_grep(params: [], error_msg: ""): - try: - output = subprocess.check_output(["git", "grep", *params], text=True, encoding="utf8") - print(error_msg) - print(output) - return 1 - except subprocess.CalledProcessError as ex1: - if ex1.returncode > 1: - raise ex1 - return 0 - - -def main(): - # Aborting the whole process is undesirable for RPC code. So nonfatal - # checks should be used over assert. See: src/util/check.h - # src/rpc/server.cpp is excluded from this check since it's mostly meta-code. - exit_code = git_grep([ - "--line-number", - "--extended-regexp", - r"\<(A|a)ss(ume|ert)\(", - "--", - "src/rpc/", - "src/wallet/rpc*", - ":(exclude)src/rpc/server.cpp", - ], "CHECK_NONFATAL(condition) or NONFATAL_UNREACHABLE should be used instead of assert for RPC code.") - - # The `BOOST_ASSERT` macro requires to `#include boost/assert.hpp`, - # which is an unnecessary Boost dependency. - exit_code |= git_grep([ - "--line-number", - "--extended-regexp", - r"BOOST_ASSERT\(", - "--", - "*.cpp", - "*.h", - ], "BOOST_ASSERT must be replaced with Assert, BOOST_REQUIRE, or BOOST_CHECK.") - - sys.exit(exit_code) - - -if __name__ == "__main__": - main() diff --git a/test/lint/test_runner/src/main.rs b/test/lint/test_runner/src/main.rs index f4fb1c36e5..ebfc3d051e 100644 --- a/test/lint/test_runner/src/main.rs +++ b/test/lint/test_runner/src/main.rs @@ -48,6 +48,16 @@ fn get_linter_list() -> Vec<&'static Linter> { name: "std_filesystem", lint_fn: lint_std_filesystem }, + &Linter { + description: "Check that fatal assertions are not used in RPC code", + name: "rpc_assert", + lint_fn: lint_rpc_assert + }, + &Linter { + description: "Check that boost assertions are not used", + name: "boost_assert", + lint_fn: lint_boost_assert + }, &Linter { description: "Check that release note snippets are in the right folder", name: "doc_release_note_snippets", @@ -237,7 +247,7 @@ fn lint_py_lint() -> LintResult { "F822", // undefined name name in __all__ "F823", // local variable name … referenced before assignment "F841", // local variable 'foo' is assigned to but never used - "PLE", // Pylint errors + "PLE", // Pylint errors "W191", // indentation contains tabs "W291", // trailing whitespace "W292", // no newline at end of file @@ -273,6 +283,7 @@ fn lint_std_filesystem() -> LintResult { let found = git() .args([ "grep", + "--line-number", "std::filesystem", "--", "./src/", @@ -293,6 +304,62 @@ fs:: namespace, which has unsafe filesystem functions marked as deleted. } } +fn lint_rpc_assert() -> LintResult { + let found = git() + .args([ + "grep", + "--line-number", + "--extended-regexp", + r"\<(A|a)ss(ume|ert)\(", + "--", + "src/rpc/", + "src/wallet/rpc*", + ":(exclude)src/rpc/server.cpp", + // src/rpc/server.cpp is excluded from this check since it's mostly meta-code. + ]) + .status() + .expect("command error") + .success(); + if found { + Err(r#" +^^^ +CHECK_NONFATAL(condition) or NONFATAL_UNREACHABLE should be used instead of assert for RPC code. + +Aborting the whole process is undesirable for RPC code. So nonfatal +checks should be used over assert. See: src/util/check.h + "# + .to_string()) + } else { + Ok(()) + } +} + +fn lint_boost_assert() -> LintResult { + let found = git() + .args([ + "grep", + "--line-number", + "--extended-regexp", + r"BOOST_ASSERT\(", + "--", + "*.cpp", + "*.h", + ]) + .status() + .expect("command error") + .success(); + if found { + Err(r#" +^^^ +BOOST_ASSERT must be replaced with Assert, BOOST_REQUIRE, or BOOST_CHECK to avoid an unnecessary +include of the boost/assert.hpp dependency. + "# + .to_string()) + } else { + Ok(()) + } +} + fn lint_doc_release_note_snippets() -> LintResult { let non_release_notes = check_output(git().args([ "ls-files", @@ -593,7 +660,7 @@ fn main() -> ExitCode { "{err}\n^---- ⚠️ Failure generated from lint check '{}'!", linter.name ); - println!("{}", linter.description); + println!("{}\n\n", linter.description); test_failed = true; } }