Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More clippy #261

Merged
merged 7 commits into from
Dec 22, 2020
Merged

More clippy #261

merged 7 commits into from
Dec 22, 2020

Conversation

tcharding
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

tcharding commented Dec 22, 2020

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
Copy link
Member

apoelstra commented Dec 22, 2020

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
Copy link
Member

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
Copy link
Member Author

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.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack a584643

@apoelstra apoelstra merged commit 2917de5 into rust-bitcoin:master Dec 22, 2020
@tcharding tcharding deleted the more-clippy branch January 11, 2022 04:46
@apoelstra
Copy link
Member

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
Copy link
Member Author

tcharding commented Aug 16, 2023

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
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants