More clippy #261

tcharding · 2020-12-22T03:16:21Z

We recently merged some work that introduced a bunch of clippy warnings. Lets keep clippy quiet and clean them up. Is there a reason we don't have CI catching these?

Patches are all trivial except the last two, particularly I just blindly followed clippy for the last one, I've not encountered ManuallyDrop before.

Clippy warns of missing `is_empty`, trivially implement it by calling through to `self.data.is_empty()`.

Found by clippy. We don't need a `return` for the final statement.

apoelstra · 2020-12-22T03:28:55Z

The reason clippy isn't in CI is that it's too noisy and the attributes needed to shut it up result in errors/warnings about "unknown attributes" from rustc 1.29.

I haven't revisited this decision in a few years - maybe things are better now. Certainly this PR found some really bizarre stuff, such as the (0..COUNT).map loop dating back to early 2016. Maybe we can just clean up all the lints and then keep it that way.

I also need to read up on ManuallyDrop -- I'm not sure what was wrong with the original code.

My local CI scripts show the first failure on e42f3f8

cargo +stable  test   '--features=bitcoin_hashes global-context lowmemory rand rand-std recovery serde' # 2020-12-22T03:29:10 / cargo 1.48.0 (65cbdd2dc 2020-10-14)
Command failed: cargo +stable test --features=bitcoin_hashes global-context lowmemory rand rand-std recovery serde
   Compiling secp256k1 v0.19.0 (/tmp/tmp2dyb6y88)
error[E0599]: no method named `unwrap` found for struct `schnorrsig::Signature` in the current scope
   --> src/schnorrsig.rs:731:14
    |
18  | pub struct Signature([u8; constants::SCHNORRSIG_SIGNATURE_SIZE]);
    | ----------------------------------------------------------------- method `unwrap` not found for this
...
731 |             .unwrap();
    |              ^^^^^^ method not found in `schnorrsig::Signature`

warning: unused import: `Configure`
   --> src/schnorrsig.rs:722:41
    |
722 |         use serde_test::{assert_tokens, Configure, Token};
    |                                         ^^^^^^^^^
    |
    = note: `#[warn(unused_imports)]` on by default

error: aborting due to previous error; 1 warning emitted

tcharding · 2020-12-22T03:46:13Z

2 from 2 CI denials today :) What scripts are you running locally please so I can do the same and not waste your time too much. Oh poo, now I'm not building with --all-features I'm missing a bunch of feature guarded stuff :)

What's your workflow to get around this please (only when you get time to answer, no rush) i.e., build with all features except fuzztarget?

Do you just use --features serde --features ... listing all features? Something like:

cargo check --features serde --features unstable --features default --features std  --features rand-std  --features recovery  --features endomorphism --features lowmemory  --features  global-context --all-targets

This helper never returns an error, remove the `Result` return type. Found by clippy.

Clippy emits warning: warning: passing a unit value to a function Just return `Ok(())` after calling `fill_bytes`.

Type is `Copy`, no need for clone.

Currently we are misusing `map` on an iterator to loop `n` times, additionally the assertion is pointless. Use a for loop and assert against the length of the set.

Suggested by clippy, we need to use ManuallyDrop for these types in order to correctly free up the memory.

apoelstra · 2020-12-22T04:16:50Z

Yep - all features except fuzz-target and external-symbols. The script I use is very much a work in progress ... its current state is at https://github.com/apoelstra/git-scripts/blob/4f02118f435e2cdc5fc17cefb2e8f9feb8219776/check.py ... I fetch all the PRs from github then run that with pr/261/head as the first arg and

'[
    {
        "type": "rust",
        "version": [ "stable", "nightly" ],
        "features": [ "bitcoin_hashes",  "global-context", "lowmemory", "rand", "rand-std", "recovery", "serde" ]
    },
    {
        "working-dir": "secp256k1-sys",
        "type": "rust",
        "version": [ "stable", "nightly" ],
        "features": [ "lowmemory", "recovery" ]
    },
    {
        "only-tip": true,
        "type": "rust",
        "version": "1.29.0",
        "features": [ "bitcoin_hashes", "global-context", "lowmemory", "rand", "rand-std", "recovery", "serde" ]
    },
    {
        "only-tip": true,
        "working-dir": "secp256k1-sys",
        "type": "rust",
        "version": "1.29.0",
        "features": [ "lowmemory", "recovery" ]
    }
]

as the second.

But this is a pretty new thing and I'm iterating rapidly (I've changed the structure of the input 4 or 5 times this week) so I wouldn't rely on this. I have related scripts which (attempt to) match commits to pull requests and which attach local review notes to things.

Don't worry about wasting my time :) it takes about a minute for me to run through a PR checking that it doesn't do weird unsafe things or access the filesystem, then I just start running that script and read Reddit until my CPU fan stops whirring.

apoelstra · 2020-12-22T04:29:04Z

Ah! So, the reason for the ManuallyDrop lint is that these constructors return a ManuallyDrop type ... and in fact, giving this to drop is a no-op. You have to use the ManuallyDrop::drop method.

In fact, the drop impl on these context objects is a no-op, so there is no problem here ... except that the point of this unit test was to make sure it really was a no-op! So the test was broken and clippy saved us.

tcharding · 2020-12-22T04:32:55Z

Ah! So, the reason for the ManuallyDrop lint is that these constructors return a ManuallyDrop type ... and in fact, giving this to drop is a no-op. You have to use the ManuallyDrop::drop method.

In fact, the drop impl on these context objects is a no-op, so there is no problem here ... except that the point of this unit test was to make sure it really was a no-op! So the test was broken and clippy saved us.

Sweet, long live clippy.

apoelstra

ack a584643

apoelstra · 2023-08-16T19:15:50Z

I have no idea what I was thinking with this comment. The Drop impl on Secp256k1 was not, and never was, a "no-op". At the time I made this comment, it called ffi::secp256k1_context_preallocated_destroy; so after this PR, that function was called twice. As it happened, for a long time that function was safe to call twice, although after the first time, it would leave the context object in an invalid state.

The commit message here which suggests that the use of ManuallyDrop will "clean up the memory" is simply wrong because calling Drop on these particular context objects will not free memory.

The original test was correct, though a little weird. clippy was wrong and the new code is wrong.

BTW the clippy lint was moved from Clippy into Rustc in rust-lang/rust#111530 as deny_by_default, which would have broken our code 🙄 but whatever.

tcharding · 2023-08-16T23:55:17Z

~~Deleted comment while I think more.~~ Ok I was right, I can revert the commit but I don't understand what the test is testing, what is the point of calling drop explicitly on a ManuallyDrop?

tcharding · 2023-08-17T00:05:37Z

Oh not to worry, I see you fixed it in #645

tcharding added 2 commits December 22, 2020 13:37

Implement is_empty method

4e87e6f

Clippy warns of missing `is_empty`, trivially implement it by calling through to `self.data.is_empty()`.

Remove unnecessary return statements

ed29f12

Found by clippy. We don't need a `return` for the final statement.

tcharding added 5 commits December 22, 2020 14:56

Remove unused error return value

34ad411

This helper never returns an error, remove the `Result` return type. Found by clippy.

Return Ok directly

ef23cb8

Clippy emits warning: warning: passing a unit value to a function Just return `Ok(())` after calling `fill_bytes`.

Remove unnecessary clone

c92b946

Type is `Copy`, no need for clone.

Use for loop instead of map

c38136b

Currently we are misusing `map` on an iterator to loop `n` times, additionally the assertion is pointless. Use a for loop and assert against the length of the set.

Use ManuallyDrop

a584643

Suggested by clippy, we need to use ManuallyDrop for these types in order to correctly free up the memory.

tcharding force-pushed the more-clippy branch from fb7e1f3 to a584643 Compare December 22, 2020 03:56

tcharding mentioned this pull request Dec 22, 2020

Add feature zeroize #262

Closed

apoelstra approved these changes Dec 22, 2020

View reviewed changes

apoelstra merged commit 2917de5 into rust-bitcoin:master Dec 22, 2020

tcharding deleted the more-clippy branch January 11, 2022 04:46

apoelstra mentioned this pull request Aug 16, 2023

update vendored libsecp to 0.3.2 #645

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More clippy #261

More clippy #261

tcharding commented Dec 22, 2020

apoelstra commented Dec 22, 2020

tcharding commented Dec 22, 2020 •

edited

Loading

apoelstra commented Dec 22, 2020 •

edited

Loading

apoelstra commented Dec 22, 2020

tcharding commented Dec 22, 2020

apoelstra left a comment

apoelstra commented Aug 16, 2023

tcharding commented Aug 16, 2023 •

edited

Loading

tcharding commented Aug 17, 2023

More clippy #261

More clippy #261

Conversation

tcharding commented Dec 22, 2020

apoelstra commented Dec 22, 2020

tcharding commented Dec 22, 2020 • edited Loading

apoelstra commented Dec 22, 2020 • edited Loading

apoelstra commented Dec 22, 2020

tcharding commented Dec 22, 2020

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Aug 16, 2023

tcharding commented Aug 16, 2023 • edited Loading

tcharding commented Aug 17, 2023

tcharding commented Dec 22, 2020 •

edited

Loading

apoelstra commented Dec 22, 2020 •

edited

Loading

tcharding commented Aug 16, 2023 •

edited

Loading