Add an IsEmpty derive macro and use it in pos-accounting - #2096
Add an IsEmpty derive macro and use it in pos-accounting#2096oliv3rdrt wants to merge 2 commits into
Conversation
607fbe5 to
0c550cc
Compare
|
Nice cleanup. I reproduced the derive in an isolated workspace and checked the generated One thing I'd like fixed before merge: The generated impl uses an unqualified trait path (
Emitting Two smaller ones:
Also, could you rebase onto current master? This branch is based on |
PoSAccountingData had a hand-written is_empty that ANDs all of its fields, with a TODO asking for it to be generated. It is easy to add a field and forget to update such a method. Add an is-empty crate with an IsEmpty trait and a matching derive, mirroring the existing typename/typename-derive pair. The derive implements is_empty as the conjunction of each field's own is_empty, so a value is empty when all of its fields are empty. Apply it to PoSAccountingData and drop the manual method; pos-accounting re-exports the trait so existing callers keep working.
The generated impl named the trait unqualified, so it resolved in the caller's scope. Deriving through a path, as in #[derive(is_empty::IsEmpty)], failed with E0405 unless the trait was also imported, and worse, if another trait called IsEmpty happened to be in scope the derive implemented that one instead and the type silently did not satisfy is_empty::IsEmpty. Emit ::is_empty::IsEmpty instead. The per field call stays unqualified on purpose, since the standard collections provide is_empty inherently. That path does not resolve inside the is-empty crate itself, so alias the crate to its own name for the tests, and cover both cases with a test that derives without the trait imported while a decoy IsEmpty is in scope. Also drop the IsEmpty re-export from pos-accounting, which existed only to give deriving users the trait in scope and is no longer needed. The one crate that calls the trait method now depends on is-empty directly. The doc comment claimed every field type must have an is_empty, which reads like a compile time guarantee. It is ordinary method lookup, so say what that means, including the fixed size array case that would silently make a struct permanently non-empty.
0c550cc to
5de506f
Compare
|
@erubboli All three done! Worth flagging: emitting ::is_empty::IsEmpty breaks the is-empty crate's own tests, since that path doesn't resolve inside the crate itself. Added extern crate self as is_empty; for that. There's now a test that derives through a qualified path with the trait not imported and a decoy IsEmpty in scope, which fails without the fix. Dropped the re-export and pointed chainstate-test-suite at is-empty directly. Doc comment now says it's plain method lookup and calls out the fixed size array case. On the rebase, the branch was already on master with #2102 in it, but it had drifted a couple of commits so it's up to date now. |
2 participants
Problem
PoSAccountingData::is_emptyis written by hand as the conjunction of its five fields'is_emptycalls, and carries a// TODO: avoid manual implementation (mintlayer/mintlayer-core#669). As the issue notes, adding a field and forgetting to update such a method is an easy mistake.Change
Add an
IsEmptytrait and a derive macro for it, as a newis-empty/is-empty-derivecrate pair that mirrors the existingtypename/typename-derivesetup. The derive generatesis_emptyas the AND of each field's ownis_empty, so a struct is empty exactly when all of its fields are empty (a field-less struct is always empty). Every field type therefore needs anis_emptymethod: the standard collections,String, or anotherIsEmptytype.Apply the derive to
PoSAccountingData, removing the manual implementation and the TODO.pos-accountingre-exports the trait so the existing callers only need it in scope.Tests
The
is-emptycrate has unit tests covering named-field, tuple and unit structs, and the empty and non-empty cases. The existingpos-accountingandchainstatetests still pass.Closes #669