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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

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

Conflicts

No conflicts as of last run.

Will it be the first Rust code in our codebase?

Why to introduce Rust as additional dependency just for the linter code?

Why to introduce Rust as additional dependency just for the linter code?

Because I don't think it is a good use of developer and reviewer time to write not type-safe code, only to later (after review or merge) discover that it crashes with a TypeError. (Just one example: #27921 (comment), obviously the problem is a general one and it is left as an exercise to the reader to create a full list of past issues, including the ones that were more severe.)
Yes, I know that mypy exists, but no one uses it, it is incomplete, and it comes with many other issues.

Moreover, the linter code already has a bunch of random dependencies, so adding one more shouldn't be an issue? Finally, it is entirely optional, and already run by CI, so no one is forced or even encouraged to run it. (Personally I can't recall the last time I've run any linter outside of testing one after writing the code for it.)

Because I don't think it is a good use of developer and reviewer time to write not type-safe code, only to later (after review or merge) discover that it crashes with a TypeError.

Why not just use C++ then? (I'm not a Rust expert, maybe there are good reasons)

I can't recall the last time I've run any linter outside of testing one after writing the code for it.

I try always to remember running linters locally before submitting a new PR (sometimes I fail and forget, need to admit).

Why not just use C++ then? (I'm not a Rust expert, maybe there are good reasons)

I tried that, but vanilla C++ doesn't have a nice interface for processes. There's only libc, or third party libs. So it seemed easier to just use rust with the added bonus that the written code closely resembles the look and feel of python. If you write something elegant in C++, I am happy to push it here, tough.

I'd love for Rust to be "backdoored" into the project, but I am not sure if adding yet another linting infrastructure is the right way to do this.

What does this check do that a check with a clang-tidy plugin could not? I have pushed a dirty prototype (likely incomplete) for such a plugin check here: TheCharlatan/bitcoin-tidy-experiments@5c1f1e1. It checks for declarations of std::filesystem::path, and calls to its string method. It also showcases some of the pre-processor checks that it can do. In this case, it checks if <filesystem> is included, and whether std::filesystem can be found anywhere in the source file text.

The tradeoff is obviously the readability of the code and that there might be some declaration edge cases that are not caught by them. A simple git-grep will always be a decent catch-all, but might miss certain declarations, or be unable to distinguish between comment and code. What do you think?

What do you think?

I'd say we should pick what reviewers like the best. However, as you say the clang-tidy plugin has many downsides:

• Likely incomplete (May be missing a Decl matcher, ignores comments, ...)
• Harder to review, and easier to break
• More code
• Requires a full build system
• Slower

I think it should be decided on a case-by-case basis whether a simple git grep-based linter is preferred, or a fully engineered clang-tidy plugin. Here, I think a git grep seems a better fit.

The git grep-based linters obviously assume some kind of formatting, but I think this is fine. Otherwise we'd have to rewrite the locale linter to clang-tidy, because someone could write std :: to_string to trick the linter? (Same for the other linters, such as the assertions or includes linter). Maybe for critical stuff we could consider adding both, one using git grep and another using clang-tidy, but I don't think we have any "critical" linters yet.

unbootstrappable

Simply claiming this doesn't seem helpful. What is the compile error you encountered? (Ideally with steps to reproduce)

rebased

ACK fa8d99d (first commit)

I have no issues with Rust, or using it in the CI; we already do in other CIs in the Bitcoin Core project. Saying that, I'm not sure if we want to introduce a 4th way to do linting in this repo (yet). One potential alternative, if we go further down the clang-tidy route, is the <filesystem> linter here: https://github.com/fanquake/bitcoin/commits/integrate_thecharlatan_filesystem, put together by @TheCharlatan.

4th way

Currently the following is used:

• Bash as a driver for ./ci/lint/06_script.sh, and in some lint script that weren't converted to python
• Python, for most lint scripts.
• Haskell (only if you compile shellcheck from source)
• pip dependencies (which may already use rust if they are wheels)

Personally I don't see a problem of adding a new dependency. Once and if this was merged, my plans were to at least remove the Bash driver, and fix the outstanding issues, e.g. to collect all failures before aborting (#28103 (comment)). So personally, at least we'd end up with one consistent driver eventually, instead of several that behave differently. But if reviewers are happy with the current lint driver, I am happy to close this pull.

clang-tidy

I am a fan of clang-tidy, but I don't think it is the better solution in this case, see #28076 (comment). In real-world scenarios it only has downsides when solving this problem at hand. Or did I miss something? It may be good to explain with an example why clang-tidy is better.

Currently the following is used:

Did you exclude clang-tidy here, or is it part of the list in the quote later in your comment. We already use it for linting, and my plan is to migrate more checks too it. For example, we can replace our python based assert linter (and more) with the equivalent clang-tidy check (LLVM 17 though).

I think the CI clang-tidy task is completely separate from the CI lint task. Just to repeat some points: It requires a full build system, a compile_commands.json, and must do a full build (slow). For the specifics, it may use more complicated and more verbose code, and it apparently ignores commented code?

For example, we can replace our python based assert linter (and more) with the equivalent clang-tidy check

Yes, this is possible, but comes with the above downsides. Or did I miss something? It may be good to explain with an example why clang-tidy is better in a specific case.

Simply claiming this doesn't seem helpful. What is the compile error you encountered? (Ideally with steps to reproduce)

The several issues bootstrapping Rust are well-known and documented elsewhere. Fixing it is not within the scope of this project. It simply shouldn't be relied on until then.

Guix is shipping rust, according to https://packages.guix.gnu.org/packages/rust-cargo, so it can be bootstrapped on all systems that can be used to compile release binaries. I am not aware of any requirement nor efforts that the linters must be as bootstrappable as the release binaries, but at least the ones in this pull are, so that alone seems sufficient. On top of that, NixOS ships rust: https://github.com/NixOS/nixpkgs/tree/nixos-23.05/pkgs/development/compilers/rust, there is also mrustc for amd64 and gcc-rust (experimental), and maybe others that I've missed.

In any case, if there is a requirement that the linters are bootstrappable on some architectures, at a minimum it should be documented. Currently, they are literally hard-coding the requirement to linux.x86_64, see

Finally, the wrapper/driver code is simply calling git grep, meaning anyone can just directly run the git-grep command directly on any system that ships with git, with no need for bootstrapping rustc, or even compiling any lint code.

I don't think we are going to get the Rust linter over the line here. Obviously the filesystem commit is correct, and should be merged. Do you want to split that out, and continue the lint discussion, or drop the linter from this PR for now?

Do you want to split that out, and continue the lint discussion

Seems better to keep the discussion in one place for now.

Seems better to keep the discussion in one place for now.

Ok, but the discussion of introducing Rust into the codebase (in whatever form) is clearly contentious, and shouldn't be a blocker for merging obvious bug fixes.

I don't think anything here is a bug fix (currently it should be a refactor). Using fs:: is safer, as explained in the pull request description.

There shouldn't be any rush to merge this pull request before someone reviewed the code, and I am happy to rebase it every now and then (when no review happened yet) to see if there are any real (unlikely) bugs introduced.

I would just like to remove the

confusing and potentially dangerous,

code. I don't see how that is predicated on, or needs to wait for a Rust based linter. Happy to ACK the first commit.

rebased

Concept ACK

This remains NACK'd and should not have been merged. Please revert it ASAP.

Here is what happened when I tried to use this according to the added documentation. Any suggestion?

jon|master =:~/bitcoin/bitcoin$ cd ./test/lint/test_runner/ && cargo fmt && cargo clippy && cargo run 
zsh: command not found: cargo

jon|master =:~/bitcoin/bitcoin/test/lint/test_runner$ brew install cargo
Running `brew update --auto-update`...
==> Auto-updated Homebrew!
==> Updated Homebrew from 75e8376816 to 8df40f4a41.
No changes to formulae or casks.

Warning: No available formula with the name "cargo". Did you mean cargo-c, argo, cairo, carton, cdargs or charge?
cargo is part of the rust formula:
  brew install rust

jon|master =:~/bitcoin/bitcoin/test/lint/test_runner$ brew install rust 
==> Downloading https://ghcr.io/v2/homebrew/core/rust/manifests/1.72.1
################################################################################################################################################################################## 100.0%
==> Fetching dependencies for rust: libssh2 and libgit2@1.6
==> Downloading https://ghcr.io/v2/homebrew/core/libssh2/manifests/1.11.0_1
################################################################################################################################################################################## 100.0%
==> Fetching libssh2
==> Downloading https://ghcr.io/v2/homebrew/core/libssh2/blobs/sha256:41e860bcf96b8e86bb5f2c321fb1ca14b620adce510cec881eeac2f432e00e5e
################################################################################################################################################################################## 100.0%
==> Downloading https://ghcr.io/v2/homebrew/core/libgit2/1.6/manifests/1.6.4
################################################################################################################################################################################## 100.0%
==> Fetching libgit2@1.6
==> Downloading https://ghcr.io/v2/homebrew/core/libgit2/1.6/blobs/sha256:d2b76777e0b8bf572537ff560539d6ad082c737851a148d71635ab899dbe6ead
################################################################################################################################################################################## 100.0%
==> Fetching rust
==> Downloading https://ghcr.io/v2/homebrew/core/rust/blobs/sha256:be9922a4b56016f18d209067f5a4d148d2aad4db3061092f848744aff41e337d
################################################################################################################################################################################## 100.0%
==> Installing dependencies for rust: libssh2 and libgit2@1.6
==> Installing rust dependency: libssh2
==> Downloading https://ghcr.io/v2/homebrew/core/libssh2/manifests/1.11.0_1
Already downloaded: /Users/jon/Library/Caches/Homebrew/downloads/48ca0c7785b21630a4817c59b72205609ccb0575e7abc64d64af2e61a60b5b0a--libssh2-1.11.0_1.bottle_manifest.json
==> Pouring libssh2--1.11.0_1.arm64_ventura.bottle.tar.gz
🍺  /opt/homebrew/Cellar/libssh2/1.11.0_1: 197 files, 1.2MB
==> Installing rust dependency: libgit2@1.6
==> Downloading https://ghcr.io/v2/homebrew/core/libgit2/1.6/manifests/1.6.4
Already downloaded: /Users/jon/Library/Caches/Homebrew/downloads/e26ed1c3bc32b8b069e105866bbd146413a04ff18eee86eadc48fa356651736b--libgit2@1.6-1.6.4.bottle_manifest.json
==> Pouring libgit2@1.6--1.6.4.arm64_ventura.bottle.tar.gz
🍺  /opt/homebrew/Cellar/libgit2@1.6/1.6.4: 104 files, 4.5MB
==> Installing rust
==> Pouring rust--1.72.1.arm64_ventura.bottle.tar.gz
==> Caveats
zsh completions have been installed to:
  /opt/homebrew/share/zsh/site-functions
==> Summary
🍺  /opt/homebrew/Cellar/rust/1.72.1: 38,925 files, 842.8MB
==> Running `brew cleanup rust`...
Disable this behaviour by setting HOMEBREW_NO_INSTALL_CLEANUP.
Hide these hints with HOMEBREW_NO_ENV_HINTS (see `man brew`).
==> Caveats
==> rust
zsh completions have been installed to:
  /opt/homebrew/share/zsh/site-functions

jon|master =:~/bitcoin/bitcoin/test/lint/test_runner$ cargo fmt && cargo clippy && cargo run                          
error: no such command: `fmt`

	Did you mean `fix`?

	View all installed commands with `cargo --list`

jon|master =:~/bitcoin/bitcoin/test/lint/test_runner$ cargo --list                                                          
Installed Commands:
    add                  Add dependencies to a Cargo.toml manifest file
    b                    alias: build
    bench                Execute all benchmarks of a local package
    build                Compile a local package and all of its dependencies
    c                    alias: check
    check                Check a local package and all of its dependencies for errors
    clean                Remove artifacts that cargo has generated in the past
    clippy               Checks a package to catch common mistakes and improve your Rust code.
    config               Inspect configuration values
    d                    alias: doc
    doc                  Build a package's documentation
    fetch                Fetch dependencies of a package from the network
    fix                  Automatically fix lint warnings reported by rustc
    generate-lockfile    Generate the lockfile for a package
    git-checkout         This command has been removed
    help                 Displays help for a cargo subcommand
    init                 Create a new cargo package in an existing directory
    install              Install a Rust binary. Default location is $HOME/.cargo/bin
    locate-project       Print a JSON representation of a Cargo.toml file's location
    login                Save an api token from the registry locally. If token is not specified, it will be read from stdin.
    logout               Remove an API token from the registry locally
    metadata             Output the resolved dependencies of a package, the concrete used versions including overrides, in machine-readable format
    new                  Create a new cargo package at <path>
    owner                Manage the owners of a crate on the registry
    package              Assemble the local package into a distributable tarball
    pkgid                Print a fully qualified package specification
    publish              Upload a package to the registry
    r                    alias: run
    read-manifest        Print a JSON representation of a Cargo.toml manifest.
    remove               Remove dependencies from a Cargo.toml manifest file
    report               Generate and display various kinds of reports
    rm                   alias: remove
    run                  Run a binary or example of the local package
    rustc                Compile a package, and pass extra options to the compiler
    rustdoc              Build a package's documentation, using specified custom flags.
    search               Search packages in crates.io
    t                    alias: test
    test                 Execute all unit and integration tests and build examples of a local package
    tree                 Display a tree visualization of a dependency graph
    uninstall            Remove a Rust binary
    update               Update dependencies as recorded in the local lock file
    vendor               Vendor all dependencies for a project locally
    verify-project       Check correctness of crate manifest
    version              Show version information
    yank                 Remove a pushed crate from the index

jon|master =:~/bitcoin/bitcoin/test/lint/test_runner$ cargo clippy && cargo run       
dyld[51046]: Symbol not found: __ZN4llvm12writeArchiveENS_9StringRefENS_8ArrayRefINS_16NewArchiveMemberEEEbNS_6object7Archive4KindEbbNSt3__110unique_ptrINS_12MemoryBufferENS7_14default_deleteIS9_EEEE
  Referenced from: <21A05006-4BB0-3CF9-B95B-E327A38E2C39> /opt/homebrew/Cellar/rust/1.72.1/lib/librustc_driver-b4744cc222a0204b.dylib
  Expected in:     <E1E1E0C9-E06E-39DC-87F4-F3E473AE4DED> /opt/homebrew/Cellar/llvm/17.0.4/lib/libLLVM.dylib
zsh: abort      cargo clippy

jon|master =:~/bitcoin/bitcoin/test/lint/test_runner$ cargo run              
error: process didn't exit successfully: `rustc -vV` (signal: 6, SIGABRT: process abort signal)
--- stderr
dyld[51064]: Symbol not found: __ZN4llvm12writeArchiveENS_9StringRefENS_8ArrayRefINS_16NewArchiveMemberEEEbNS_6object7Archive4KindEbbNSt3__110unique_ptrINS_12MemoryBufferENS7_14default_deleteIS9_EEEE
  Referenced from: <21A05006-4BB0-3CF9-B95B-E327A38E2C39> /opt/homebrew/Cellar/rust/1.72.1/lib/librustc_driver-b4744cc222a0204b.dylib
  Expected in:     <E1E1E0C9-E06E-39DC-87F4-F3E473AE4DED> /opt/homebrew/Cellar/llvm/17.0.4/lib/libLLVM.dylib

I run (most of) the linters locally with a bash alias before pushing a change.

I'd suggest fixing your brew / cargo / rust installation. It looks broken, i.e:

error: process didn't exit successfully: `rustc -vV`

Just running the compiler doesn't work.

The cargo fmt and clippy part is just for writing new code and not strictly needed for just running code. You should be good to skip over it for now. However, if you want to write code, it may be better to look into getting it to work.

Thanks. Resolved (on ARM64 macOS 13.6 Ventura) as follows:

$ brew remove rust
$ curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh
$ rustc --version
rustc 1.73.0 (cc66ad468 2023-10-03)
$ cargo --list

(running cargo --list now includes the fmt command)

$ cd ./test/lint/test_runner/ && cargo fmt && cargo clippy && cargo run 
    Checking test_runner v0.1.0 (/Users/jon/bitcoin/bitcoin/test/lint/test_runner)
    Finished dev [unoptimized + debuginfo] target(s) in 0.16s
   Compiling test_runner v0.1.0 (/Users/jon/bitcoin/bitcoin/test/lint/test_runner)
    Finished dev [unoptimized + debuginfo] target(s) in 1.37s
     Running `target/debug/test_runner`

Concept NACK

The integration of Rust into Core, further increases the technical debt. Each person's ability to run tests locally is essential, and relying on CIs to manage Rust code is not a viable solution. In addition, introducing Rust can encourage the addition of other languages, risking fragmenting and further complicating the code base.

relying on CIs

This pull request does not change how much CI is relied upon. Previously, CI was running on all patches, and it is still after this pull request. Previously, it was possible to run any CI task locally, and it is still possible, in the same way. I am running the CI locally regularly and it obviously helps if others do the same. If you have issues running anything locally, it would be good to open an issue, so that it can be fixed. When filing an issue, adding context which documentation you followed and adding exact steps to reproduce is helpful.