New clippy lint in rustc nightly "lifetime flowing from input to output
with different syntax can be confusing".
Apply the suggested fix and use the anonymous lifetime for paths.
There is a new lint error on nightly-2025-04-25 "variables can be used
directly in the `format!` string".
Exclude the lint to allow the existing syntax in `format!` strings.
280eb2239a io: Remove the impl_write macro (Tobin C. Harding)
Pull request description:
We have found calling macros from other crates to be error prone, especially when they involve feature gates. Furthermore the `impl_write` macro is trivial to write, users can just write it in the crate that needs it.
Lets just remove the macro. While we are at it point users to the examples in `bitcoin/examples/io.rs` for usage of the `io` crate.
Close: #3872
ACKs for top commit:
Kixunil:
ACK 280eb2239a
apoelstra:
ACK 280eb2239a2fcaa83a35ae6fb1ce62fe35a1e222; successfully ran local tests
Tree-SHA512: 62766ff6e4b5f3fae2d6214e87bd90c15b3103248effd4399a070ac831473b0288e599eed2377c08d2b7eca263220f172c8399a607756793477f82d3dc8f1b67
We would like to remove the `GeneralHash` trait but we want to keep the
`hash_reader` functionality.
Add a stand alone function (when `hashes` is enabled) `hash_reader`.
Remove the extension trait.
Currently in order to release `hashes v1.0` we need to 1.0 `io` as well.
For multiple reasons, many out of our control, the `io` crate may not
stabalise any time soon.
Instead we can invert the dependency between the two crates.
This is an ingenious idea, props to Kixunil for coming up with it.
Notes
- `io` does not currently re-export the `hashes` crate.
- This work highlights that we cannot call `hash_reader` on a siphash.
- The `Hmac::hash_reader` uses the default key which may not be obvious.
Signed-off-by: Tobin C. Harding <me@tobin.cc>
We have found calling macros from other crates to be error prone,
especially when they involve feature gates. Furthermore the `impl_write`
macro is trivial to write, users can just write it in the crate that
needs it.
Remove the macro. While we are at it point users to the examples in
`bitcoin/examples/io.rs` for usage of the `io` crate.
71fab82a1b io: Improve docs on macro (Tobin C. Harding)
9c81d5e747 io: Move macro_export below docs (Tobin C. Harding)
b109177e11 io: Improve rustdocs (Tobin C. Harding)
07d8703a00 io: Add SPDX identifier (Tobin C. Harding)
b844637935 io: Remove rustdoc for private module (Tobin C. Harding)
Pull request description:
As part of #3643 improve the public documentation on the `io` crate.
While doing this I also checked:
- C-FAILURE
- C-LINK
- C-METADATA
- C-RELNOTES
- C-HIDDEN
ACKs for top commit:
apoelstra:
ACK 71fab82a1b32e6077adb896be9d8ac12d5513e0a; successfully ran local tests
Tree-SHA512: 91b8807102277fb7f6602837b7d0e64e4276c9c5bf748ab875ea0e4f1f7f91bc989413acd25e4412d72d6f3744a22b746ed63cbac20d9b42217cd3164c7e6847
Add two unit tests:
- Check we can read into an empty buffer as validation of args as
part of C-VALIDATE
- Do basic read using `Take::read_to_end` since it is currently
untested.
In preparation for releasing `io v1.0` make a sweep of the rustdocs and
improve them for all modules except `macros`.
With this applied I believe we can tick off:
- C-FAILURE
- C-LINK
- C-HIDDEN
The `io` crate is licensed in the manifest using the CC0-1.0 license
same as the rest of the codebase but none of the individual files have a
license blurb.
Add a CC0-1.0 license blurb by way of an SPDX-License-Identifier tag.
426f585a47 api: Run just check-api (Tobin C. Harding)
6cf90132bc io: Add traits (Tobin C. Harding)
Pull request description:
So that our `io` crate is not surprising it seems we should generally, unless there is a good reason not to, follow `std::io`.
Copy the derived trait implementations from `std::io` for `Cursor`, and `Sink`. `Take` is correct already, just `Debug`.
Done while investigating C-COMMON-TRAITS
ACKs for top commit:
apoelstra:
ACK 426f585a479ca20b7c3390c589b836f8726b9b03; successfully ran local tests
Tree-SHA512: 0fdacfa0295c36e9ee2bdffd5e649b923f48eeff7baa3afc99fda0836ae30b794610a176c0e76341a268c712baab51139355976c23913a0744692cd1e38a4d11
e315aaf1c6 io: Allow setting position bigger than inner buffer (Tobin C. Harding)
Pull request description:
Currently if one calls `set_position` on a cursor setting the position greater than the total length of the inner buffer future calls to `Read` will panic. Bad `Cursor` - no biscuit.
Mirror the stdlib `Cursor` and allow setting the position to any value. Make reads past the end of the buffer return `Ok(0)`.
ACKs for top commit:
apoelstra:
ACK e315aaf1c65225b89435d0a60907a3233caa5db1; successfully ran local tests
Tree-SHA512: 71b151218e96343a86c8b2c21c6e8212e2d1c2aea6517b4662da3ad01b3b598cec497028b863a8e36a6b1fae1d4f7f975c8610c3b6f39f1c256adc5751d06ea0
So that our `io` crate is not surprising it seems we should generally,
unless there is a good reason not to, follow `std::io`.
Copy the derived trait implementations from `std::io` for `Cursor`,
`Sink`, and `Take`.
Done while investigating C-COMMON-TRAITS
Currently if one calls `set_position` on a cursor setting the position
greater than the total length of the inner buffer future calls to `Read`
will panic. Bad `Cursor` - no biscuit.
Mirror the stdlib `Cursor` and allow setting the position to any value.
Make reads past the end of the buffer return `Ok(0)`.
Our `Cursor` is a replacement of sorts for the `std::io::Cursor`. Users
of the `Cursor` are likely to expect the same API so to get a reference
and/or mutable reference users will likely reach for `get_ref` and
`get_mut`. We should provide these.
Deprecate `inner` since it is the same as `get_ref` but less
idiomatically named (should have been `as_inner`).
Add `get_ref` and `get_mut` methods to the `Cursor` type.
There is a range of different wordings used in the docs of constructor
type functions.
Change all to start with `Constructs a new` or `Constructs an empty`.
In functions that act like constructors there is a mixture of the usage
of `creates` and `constructs`.
Replace all occurrences of `creates` with `constructs` in the first line
of docs of constructor like functions.
This has been fixed and we use nightly to lint so we have access to the
merged fix.
Removing the attribute uncovers a bunch of real lint warnings, fix
them while we are at it.
Examples in documentation are not linted in the same way as other code,
but should still contain correctly written code.
Throughout all of the crates except internals (another commit) unused
variables have been prefixed with `_`, unused imports have been removed,
and a warn attribute added to all of the `lib.rs` files.
4ead0adcb5 Add blanket impl of io traits for `&mut T` (Martin Habovstiak)
Pull request description:
The impl wasn't previously available because we thought we can do a blanket impl for `std` traits. We've removed the `std` blanket impl when we realized it's broken but forgot to add the `&mut T` impls. This adds them.
Note: this is in part an experiment to see if this is API breaking. I suspect it might be.
ACKs for top commit:
apoelstra:
ACK 4ead0adcb5 successfully ran local tests
tcharding:
ACK 4ead0adcb5
Tree-SHA512: 1e4411fdf019f3793e6366eda7e8b46246e3263bd7e41802877c20cc5e8ea451a79d89b4c59204ea9c1eb590054975de52a791a9d17a1d180a5cfac316efa959
ad34a98c61 Refactor Rust version checking (Martin Habovstiak)
7d5ce89dad Fix type ambiguity in IO tests (Martin Habovstiak)
Pull request description:
Conditional compilation depending on Rust version using `cfg` had the disadvantage that we had to write the same code multiple times, compile it multiple times, execute it multiple times, update it multiple times... Apart from obvious maintenance issues the build script wasn't generating the list of allowed `cfg`s so those had to be maintained manually in `Cargo.toml`. This was fixable by printing an appropriate line but it's best to do it together with the other changes.
Because we cannot export `cfg` flags from a crate to different crates we take a completely different approach: we define a macro called `rust_version` that takes a very naturally looking condition such as `if >= 1.70 {}`. This macro is auto-generated so that it produces different results based on the compiler version - it either expands to first block or the second block (after `else`).
This way, the other crates can simply call the macro when needed.
Unfortunately some minimal maintenance is still needed: to update the max version number when a newer version is used. (Note that code will still work with higher versions, it only limits which conditions can be used in downstream code.) This can be automated with the pin update script or we could just put the pin file into the `internals` directory and read the value from there. Not automating isn't terrible either since anyone adding a cfg with higher version will see a nice error about unknown version of Rust and can update it manually.
Because this changes syntax to a more naturally looking version number, as a side effect the `cond_const` macro could be also updated to use the new macro under the hood, providing much nicer experience - it is no longer needed to provide human-readable version of the version string to put in the note about `const`ness requiring a newer version. As such the note is now always there using a single source of truth.
It's also a great moment to introduce this change right now since there's currently no conditional compilation used in `bitcoin` crate making the changes minimal. However it is not yet added to `bitcoin-io` since `bitcoin-io` is not depending on `internals`. It might be a reason to start depending on it but that's for later discussion.
ACKs for top commit:
apoelstra:
ACK ad34a98c61 successfully ran local tests
tcharding:
ACK ad34a98c61
Tree-SHA512: 7a82017fc51ba1b473ca638c7bdbf5c893da0a78c70ea8f1a0241049e7874592fad328abd60b3e09a00439b771e7cfc5ebad5e874314d15a48271ec6cb2d7bb7
The impl wasn't previously available because we thought we can do a
blanket impl for `std` traits. We've removed the `std` blanket impl when
we realized it's broken but forgot to add the `&mut T` impls. This adds
them.
Without the ability to seek, the `Cursor` type is only useful for
wrapping owned buffers. In `std` the equivalent type also allows
seeking. We currently do not have the `Seek` trait so this simply adds a
method to set the position. Further, this adds a method to access inner
type so seeking from the end can be implemented (to compute the position
relative to the end).
Closes#3174
This adds a reverse of the previous commit however it does not add
convenience methods since user should generally prefer the macro
instead. I had a clever idea to add a convenience conversion trait but
that is blocked by Rust stabilizing overlapping *marker* trait impls.
When we've removed the blanket impl we forgot to replace it with a
bridge that would make the usage with `std` types easy. This change
implements the missing bridge forwarding all the methods and traits.
051c358bcb Remove deprecated legacy numeric methods (Divyansh Gupta)
Pull request description:
As `rustc 1.79.0-nightly (9d79cd5f7 2024-04-05)` is released which solves the issue mentioned , but the release has deperacted legacy numeric methods.
Thus replaced `u16::max_value()` etc with `u32::MAX` & `core::u16` to directly `u16`.
fix#2639
ACKs for top commit:
tcharding:
ACK 051c358bcb
apoelstra:
ACK 051c358bcb thanks! I will remove an equivalent commit from my #2669
Tree-SHA512: c08c856f7f3b281417c29283351eac5e0f75cc1c8d23d9aae58d969219a327b2337fe57932053e53773ebb9dbec04254f90149266b6639a66c5c09f2ad1675ef
As `rustc 1.79.0-nightly (9d79cd5f7 2024-04-05)` is released which solves the issue mentioned , but the release has deperacted legacy numeric methods.
Thus replace `u16::max_value()` etc with `u32::MAX` & `core::u16` to directly `u16`.
fix#2639
This lint triggers when parsing a reference to a large struct as a
generic argument, which is wrong.
Allow it crate wide because [subjectively] this lint never warns for
anything useful.
Remove the blanket impls of `Read`, `BufRead`, and `Write`. Replace them
with a set of sane impls.
Note, we add code to the `impl_write` macro to implement both
`crate::Write` and `std::io::Write` when "std" feature is
enabled.
Fix: #2432
Currently `Take::read_to_end` is private forcing users to use our
"custom" `read_to_limit`, for seasoned Rust hackers
`foo.take(16).read_to_end(buf)` make be more unsurprising.
Make `read_to_end` public.
Make the trait level attributes uniform across all released crates in
the repo. Excludes things that are obviously not needed, eg, bench stuff
if there is not bench code.
- Remove `uninhabited_references` - this is allow by default now.
- Remove `unconditional_recursion` and mark the single false positive we
have with an `allow`.
Note, this does not add `missing_docs` to the `io` crate. There is an
open PR at the moment to add that along with the required docs.
The `std::io::Read` trait includes `read_to_end` but that method
provides a denial of service attack vector since an unbounded reader
will exhaust all system memory.
Add a method to our `Read` trait called `read_to_limit` that does the
same as `std::io::Read::read_to_end` but with memory exhaustion
protection.
Add a `read_to_end` method on our `Take` trait and call through to it
from the new method on our `Read` trait called `read_to_limit`.
Jamil Lambert, PhD
merge-script
Tobin C. Harding
Fmt Bot
Martin Habovstiak
Divyansh Gupta