util::Result has confusing interface for std::*_ptr T #26004

1. 
   luke-jr commented at 9:31 pm on September 4, 2022: member

   util::Result<std::unique_ptr<...>> treats a nullptr as a true value. This has caused at least one bug so far.

   Can we easily forbid passing nullptr in this way? Is there a better/safer solution?

2. 
   fanquake commented at 9:36 pm on September 4, 2022: member

   cc @ryanofsky
3. 
   luke-jr commented at 9:39 pm on September 4, 2022: member

   (For the mentioned bug, see https://github.com/bitcoin-core/gui/issues/661 and fix in #26005)
4. 
   ryanofsky commented at 11:45 am on September 5, 2022: member

   Good catch, and I think it would be good to do something to prevent this. Few ideas:

   1. Result class could drop operator bool and force you to write if (result.has_value()) instead of if (result). I don’t thnk this would really solve problem because Result<unique_ptr> could still be assigned nullptr, and result.has_value() would still return true in that case.

   2. Return type could be changed from Result<unique_ptr> to Result<not_null<unique_ptr>> or Result<nn_unique_ptr> using a not-null helper class like one from https://github.com/bitwizeshift/not_null#references. I think this would be a potentially good solution because it would make it a compile time error to assign a nullptr or plain unique_ptr to the result. Only a non-nullable pointer could be assigned to avoid the compiler error.

   3. Could introduce util::NullableResult to complement util::Result. Nullable result class would have the same behavior as normal result class except operator bool would be implemented as return has_value() && value() instead of return has_value(). The name NullableResult would clearly communicate that a null result is possible and not an error, and bool operator would make it easier to check for.

   4. Could do nothing. Result<unique_ptr>{nullptr} being true is not any different from optional<unique_ptr>{nullptr} being true or optional<int>{0} being true or const char* s{""} being true. It’s kind of just how indirection works in C/C++ with multiple layers of indirection.

   I tend to think (2) is nicest solution, but I’m not sure if there are other use-cases for non-nullable pointers that would justify the complexity since they don’t seem completely trivial to implement. I don’t like solution (1) because it creates an unnecessary difference between std::optional and util::Result and IMO would makes code using the result class look ugly. Solution (3) I think could be implemented in a few lines and I think would be better than the status quo, because it communicates that the value can be null and makes it easy to check for, even if it doesn’t prevent null from being assigned.

5. 
   MarcoFalke commented at 12:12 pm on September 5, 2022: member

   See also #24423
6. 
   ryanofsky commented at 2:07 pm on September 5, 2022: member

   See also #24423

   Forgot about that discussion. This does seem like a genuine use-case for not_null.

   Another idea for the list:

   5. Could replace Result<unique_ptr<Wallet>> with ResultPtr<Wallet> where a new ResultPtr class basically acts like unique_ptr except it has a util::Error constructor so it can hold error information, and is always set to an error or non-null pointer, never a plain null pointer. ResultPtr<Wallet> would be semantically equivalent to Result<not_null<unique_ptr<Wallet>>> except it would have nicer syntax and not require double dereferencing to call wallet methods.
7. 
   luke-jr commented at 7:59 pm on September 5, 2022: member

   The issue with not_null seems to be that in some cases, it might not detect it until runtime, and then throw an exception - which would have been a bug in this case (https://github.com/bitcoin-core/gui/issues/661) too.

   Can we maybe throw an exception in debug builds, but in production builds simply treat nullptr assignments as clearing?

8. 
   luke-jr commented at 8:29 pm on September 5, 2022: member

   5. Could replace Result<unique_ptr> with ResultPtr where a new ResultPtr class basically acts like unique_ptr except it has a util::Error constructor so it can hold error information, and is always set to an error or non-null pointer, never a plain null pointer. ResultPtr would be semantically equivalent to Result<not_null<unique_ptr» except it would have nicer syntax and not require double dereferencing to call wallet methods.

   Would this impede having a constant Wallet object per wallet?

9. ryanofsky referenced this in commit a8973ae714 on Sep 6, 2022
10. 
    ryanofsky commented at 4:34 pm on September 6, 2022: member

    The issue with not_null seems to be that in some cases, it might not detect it until runtime, and then throw an exception - which would have been a bug in this case (bitcoin-core/gui#661) too.

    Yes, we would want to use a strict not_null implementation that makes it a compile error to try to assign a nullable pointer to a non-nullable one. Strict non_null pointers can only be initialized with special constructors, and if you call those constructors correctly, the compiler can guarantee the pointers are safe to dereference after that. The benefit of this is that it is easier to verify not_null pointers are constructed properly than it is to verify nullable pointers are safe to dereference every place they are dereferenced.

    Can we maybe throw an exception in debug builds, but in production builds simply treat nullptr assignments as clearing?

    It would be an implementation detail of the not_null class, but it could provide a constructor that only throws in debug builds.

    Would this impede having a constant Wallet object per wallet?

    I’m not even sure we have this currently. interfaces::Wallet objects are disposable wrappers around a more permanent wallet::CWallet object, but I think as long as pointers can be moved from, then object lifetimes / constness should be unaffected by things proposed here.

11. 
    luke-jr commented at 7:02 pm on September 6, 2022: member

    I’m not even sure we have this currently. interfaces::Wallet objects are disposable wrappers around a more permanent wallet::CWallet object, but I think as long as pointers can be moved from, then object lifetimes / constness should be unaffected by things proposed here.

    We don’t, but I think we should. It would be nice to put the abstraction interface between all accesses - trying to make CWallet itself abstract has proven impractical (trying to add support for a Lightning wallet).

12. ryanofsky referenced this in commit 8ba9cb26a1 on Sep 20, 2022
13. ryanofsky referenced this in commit 8ebf0cd171 on Sep 20, 2022

Contributors
luke-jr fanquake ryanofsky MarcoFalke