From fa9aacf614f6066ff3b5c02f0daaaeb0ebb93f33 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Fri, 6 Dec 2024 09:55:54 +0100 Subject: [PATCH 1/2] 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; } } From e8f0e6efaf555a7d17bdb118464bd572bd5f3933 Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Thu, 12 Dec 2024 19:47:19 +0100 Subject: [PATCH 2/2] 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. --- test/lint/test_runner/src/main.rs | 37 +++++++++++++++---------------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/test/lint/test_runner/src/main.rs b/test/lint/test_runner/src/main.rs index ebfc3d051e..8479fd2d64 100644 --- a/test/lint/test_runner/src/main.rs +++ b/test/lint/test_runner/src/main.rs @@ -294,10 +294,10 @@ fn lint_std_filesystem() -> LintResult { .success(); if found { Err(r#" -^^^ Direct use of std::filesystem may be dangerous and buggy. Please include and use the fs:: namespace, which has unsafe filesystem functions marked as deleted. "# + .trim() .to_string()) } else { Ok(()) @@ -322,12 +322,12 @@ fn lint_rpc_assert() -> LintResult { .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 "# + .trim() .to_string()) } else { Ok(()) @@ -350,10 +350,10 @@ fn lint_boost_assert() -> LintResult { .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. "# + .trim() .to_string()) } else { Ok(()) @@ -370,17 +370,15 @@ fn lint_doc_release_note_snippets() -> LintResult { if non_release_notes.is_empty() { Ok(()) } else { - Err(format!( - r#" -{} -^^^ + println!("{non_release_notes}"); + Err(r#" Release note snippets and other docs must be put into the doc/ folder directly. The doc/release-notes/ folder is for archived release notes of previous releases only. Snippets are expected to follow the naming "/doc/release-notes-.md". - "#, - non_release_notes - )) + "# + .trim() + .to_string()) } } @@ -423,7 +421,6 @@ fn lint_trailing_whitespace() -> LintResult { .success(); if trailing_space { Err(r#" -^^^ Trailing whitespace (including Windows line endings [CR LF]) is problematic, because git may warn about it, or editors may remove it by default, forcing developers in the future to either undo the changes manually or spend time on review. @@ -433,6 +430,7 @@ Thus, it is best to remove the trailing space now. Please add any false positives, such as subtrees, Windows-related files, patch files, or externally sourced files to the exclude list. "# + .trim() .to_string()) } else { Ok(()) @@ -449,7 +447,6 @@ fn lint_tabs_whitespace() -> LintResult { .success(); if tabs { Err(r#" -^^^ Use of tabs in this codebase is problematic, because existing code uses spaces and tabs will cause display issues and conflict with editor settings. @@ -457,6 +454,7 @@ Please remove the tabs. Please add any false positives, such as subtrees, or externally sourced files to the exclude list. "# + .trim() .to_string()) } else { Ok(()) @@ -531,7 +529,6 @@ fn lint_includes_build_config() -> LintResult { if missing { return Err(format!( r#" -^^^ One or more files use a symbol declared in the bitcoin-build-config.h header. However, they are not including the header. This is problematic, because the header may or may not be indirectly included. If the indirect include were to be intentionally or accidentally removed, the build could @@ -547,12 +544,13 @@ include again. #include // IWYU pragma: keep "#, defines_regex - )); + ) + .trim() + .to_string()); } let redundant = print_affected_files(false); if redundant { return Err(r#" -^^^ None of the files use a symbol declared in the bitcoin-build-config.h header. However, they are including the header. Consider removing the unused include. "# @@ -605,7 +603,9 @@ Markdown link errors found: {} "#, stderr - )) + ) + .trim() + .to_string()) } Err(e) if e.kind() == ErrorKind::NotFound => { println!("`mlc` was not found in $PATH, skipping markdown lint check."); @@ -657,10 +657,9 @@ fn main() -> ExitCode { env::set_current_dir(&git_root).unwrap(); if let Err(err) = (linter.lint_fn)() { println!( - "{err}\n^---- ⚠️ Failure generated from lint check '{}'!", - linter.name + "^^^\n{err}\n^---- ⚠️ Failure generated from lint check '{}' ({})!\n\n", + linter.name, linter.description, ); - println!("{}\n\n", linter.description); test_failed = true; } }