Replace failable_function with an Expected-carrying result handler

.drone.jsonnet

@@ -305,8 +305,8 @@ local clang(version) = debian_build(
               ' -DCMAKE_CXX_COMPILER=clang++-' + version
 );
 
-local full_llvm(version) = debian_build(
-  'Debian sid/llvm-' + version,
+local full_llvm(version, cxx=null) = debian_build(
+  'Debian sid/llvm-' + version + (if cxx == null then '' else '/C++' + cxx),
   docker_base + 'debian-sid-clang',
   deps=['clang-' + version, ' lld-' + version, ' libc++-' + version + '-dev', 'libc++abi-' + version + '-dev']
        + default_deps_nocxx,
@@ -315,6 +315,7 @@ local full_llvm(version) = debian_build(
   cmake_extra='-DCMAKE_C_COMPILER=clang-' + version +
               ' -DCMAKE_CXX_COMPILER=clang++-' + version +
               ' -DCMAKE_CXX_FLAGS="-stdlib=libc++ -fcolor-diagnostics" ' +
+              (if cxx == null then '' else '-DCMAKE_CXX_STANDARD=' + cxx + ' ') +
               ' -DOXEN_LOGGING_FORCE_SUBMODULES=ON ' +
               std.join(' ', [
                 '-DDEPS_FORCE_' + m + '_SUBMODULE=ON'
@@ -477,6 +478,17 @@ local static_build(name,
 
   debian_build('Debian sid/Debug', docker_base + 'debian-sid', build_type='Debug'),
   debian_build('Debian testing', docker_base + 'debian-testing'),
+
+  // C++23, which is what `session::Expected` is written against: under it `expected.hpp` resolves
+  // to `std::expected` rather than the local stand-in, so these compile every use in the project
+  // against the real thing.  That is what keeps the stand-in a strict subset -- a use that has
+  // drifted outside it fails here rather than waiting for whoever eventually raises the standard.
+  //
+  // Both libstdc++ and libc++, because the two disagree about plenty and a subset that only holds
+  // against one of them is not a subset.
+  debian_build('Debian sid/C++23', docker_base + 'debian-sid', cmake_extra='-DCMAKE_CXX_STANDARD=23'),
+  full_llvm(23, cxx=23),
+
   clang(19),
   full_llvm(19),
   debian_build('Debian stable (i386)', docker_base + 'debian-stable/i386'),

CMakeLists.txt

@@ -41,7 +41,14 @@ else()
 endif()
 
 
-set(CMAKE_CXX_STANDARD 20)
+# 20 is what this is written against, but it has to go in the cache rather than be a plain `set`:
+# a normal variable shadows the cache, so `-DCMAKE_CXX_STANDARD=23` would be accepted and then
+# silently ignored.  Building at 23 is how `session::Expected` is checked against the real
+# `std::expected` (see include/session/expected.hpp), so that has to actually take effect.
+#
+# Claiming the entry here also settles it before `external/date` does: it sets the same cache
+# variable to 17, and whichever of the two runs first wins, since neither uses FORCE.
+set(CMAKE_CXX_STANDARD 20 CACHE STRING "C++ standard to build with")
 set(CMAKE_CXX_STANDARD_REQUIRED ON)
 set(CMAKE_CXX_EXTENSIONS OFF)
 set(CMAKE_POLICY_VERSION_MINIMUM 3.5)

include/session/client/conversation.hpp

@@ -202,17 +202,17 @@ class Conversation {
     /// meaning "all of it" -- SQLite reads `LIMIT -1` that way, this does not.  History grows
     /// without bound, so reading all of it means paging with `before` until a page comes back
     /// short.
-    void messages(failable_function<void(std::vector<Message>)> cb) const;
-    void messages(int limit, failable_function<void(std::vector<Message>)> cb) const;
+    void messages(result_function<std::vector<Message>> cb) const;
+    void messages(int limit, result_function<std::vector<Message>> cb) const;
     void messages(
             int limit,
             std::optional<MessageCursor> before,
-            failable_function<void(std::vector<Message>)> cb) const;
+            result_function<std::vector<Message>> cb) const;
     void messages(
             int limit,
             std::optional<MessageCursor> before,
             bool include_deleted,
-            failable_function<void(std::vector<Message>)> cb) const;
+            result_function<std::vector<Message>> cb) const;
     std::vector<Message> messages(await_t) const;
     std::vector<Message> messages(int limit, await_t) const;
     std::vector<Message> messages(int limit, std::optional<MessageCursor> before, await_t) const;
@@ -237,7 +237,7 @@ class Conversation {
     ///
     /// Messages we sent and deleted everywhere are not exposed to this: their swarm copy is already
     /// gone, so there is nothing left to return.
-    void purge_deleted(failable_function<void(size_t removed)> cb);
+    void purge_deleted(result_function<size_t> cb);
     size_t purge_deleted(await_t);
 
     // -- Read state ---------------------------------------------------------------------------
@@ -249,14 +249,14 @@ class Conversation {
     ///
     /// Never moves the watermark backwards.  Clears `marked_unread`, since reading the conversation
     /// is the thing that was being asked for.
-    void mark_read(failable_function<void()> cb);
-    void mark_read(std::optional<sys_ms> up_to, failable_function<void()> cb);
+    void mark_read(result_function<> cb);
+    void mark_read(std::optional<sys_ms> up_to, result_function<> cb);
     void mark_read(await_t);
     void mark_read(std::optional<sys_ms> up_to, await_t);
 
     /// Marks the conversation unread, or clears that — the deliberate "I want to come back to this"
     /// rather than a count of messages.  Synced, so it follows you between devices.
-    void set_marked_unread(bool unread, failable_function<void()> cb);
+    void set_marked_unread(bool unread, result_function<> cb);
     void set_marked_unread(bool unread, await_t);
 
     // -- Settings -----------------------------------------------------------------------------
@@ -266,24 +266,23 @@ class Conversation {
     /// Reported to subscribers as `conversation_list_replaced`, not as an update to the one
     /// conversation, because hiding removes a conversation from the list and unhiding returns it —
     /// so what changed is the list, not a row in it.
-    void set_priority(int priority, failable_function<void()> cb);
+    void set_priority(int priority, result_function<> cb);
     void set_priority(int priority, await_t);
 
     /// Sets when to notify, and until when to stay quiet.  Separate calls because they are separate
     /// decisions: muting until Monday does not change what you want notified after Monday, and a UI
     /// that offered them together would have to invent an answer for the other one.
     ///
     /// A `mute_until` in the past, or the epoch, is not muted.
-    void set_notifications(config::notify_mode mode, failable_function<void()> cb);
+    void set_notifications(config::notify_mode mode, result_function<> cb);
     void set_notifications(config::notify_mode mode, await_t);
-    void set_mute_until(std::chrono::sys_seconds until, failable_function<void()> cb);
+    void set_mute_until(std::chrono::sys_seconds until, result_function<> cb);
     void set_mute_until(std::chrono::sys_seconds until, await_t);
 
     /// Sets the disappearing-message mode and timer together, because neither means anything
     /// alone: a timer with no mode does not expire, and a mode with no timer has nothing to count.
     /// `expiration_mode::none` clears the timer whatever is passed with it.
-    void set_expiry(
-            config::expiration_mode mode, std::chrono::seconds timer, failable_function<void()> cb);
+    void set_expiry(config::expiration_mode mode, std::chrono::seconds timer, result_function<> cb);
     void set_expiry(config::expiration_mode mode, std::chrono::seconds timer, await_t);
 
     /// Sets what this conversation fetches without being asked.
@@ -295,7 +294,7 @@ class Conversation {
     /// Calling this is also what records that the question has been asked at all, so a client that
     /// prompts on first use should call it with whatever answer it was given — `none` included,
     /// since that is not the same as never having asked, and only the latter should prompt again.
-    void set_auto_download(AutoDownload mode, failable_function<void()> cb);
+    void set_auto_download(AutoDownload mode, result_function<> cb);
     void set_auto_download(AutoDownload mode, await_t);
 
     // -- Sending ------------------------------------------------------------------------------
@@ -355,11 +354,8 @@ class Conversation {
     /// conversation; thrown on the calling thread, before anything is stored or dispatched.
     using upload_progress = std::function<void(
             size_t index, int64_t sent, int64_t total, std::optional<int> result)>;
-    void send_message(
-            OutgoingMessage msg,
-            upload_progress on_upload,
-            failable_function<void(int64_t message_id)> cb);
-    void send_message(OutgoingMessage msg, failable_function<void(int64_t message_id)> cb);
+    void send_message(OutgoingMessage msg, upload_progress on_upload, result_function<int64_t> cb);
+    void send_message(OutgoingMessage msg, result_function<int64_t> cb);
     int64_t send_message(OutgoingMessage msg, upload_progress on_upload, await_t);
     int64_t send_message(OutgoingMessage msg, await_t);
 
@@ -380,7 +376,7 @@ class Conversation {
 
     /// Deletes the messages, keeping the conversation itself and everything that describes it — the
     /// contact, the nickname, the pin.
-    void clear_messages(failable_function<void()> cb);
+    void clear_messages(result_function<> cb);
     void clear_messages(await_t);
 
     /// Removes the conversation from the list without forgetting who it is with: the contact entry
@@ -390,8 +386,8 @@ class Conversation {
     /// `keep_messages` distinguishes the two things a UI calls this for.  Deleting a conversation
     /// destroys its history; hiding one — which is what "Hide" on Note to Self does — leaves the
     /// history to come back with it.
-    void delete_conversation(failable_function<void()> cb);
-    void delete_conversation(bool keep_messages, failable_function<void()> cb);
+    void delete_conversation(result_function<> cb);
+    void delete_conversation(bool keep_messages, result_function<> cb);
     void delete_conversation(await_t);
     void delete_conversation(bool keep_messages, await_t);
 
@@ -460,7 +456,7 @@ class DM : public Conversation {
 
     /// Sets or clears the name we have given them, which is ours rather than theirs and follows us
     /// between devices.  An empty nickname removes it, so `display_name` falls back to `name`.
-    void set_nickname(std::string_view nickname, failable_function<void()> cb);
+    void set_nickname(std::string_view nickname, result_function<> cb);
     void set_nickname(std::string_view nickname, await_t);
 
     /// Blocks or unblocks the account.  Blocking is synced, so it takes effect on every device, and
@@ -469,7 +465,7 @@ class DM : public Conversation {
     /// Blocking someone we hold no contact entry for creates one, since being blocked is a fact
     /// about a relationship and there is nowhere else to record it.  It does not approve them, and
     /// unblocking does not undo anything else the block implied.
-    void set_blocked(bool blocked, failable_function<void()> cb);
+    void set_blocked(bool blocked, result_function<> cb);
     void set_blocked(bool blocked, await_t);
 
     /// Deletes the contact along with the conversation and its history, everywhere.
@@ -486,7 +482,7 @@ class DM : public Conversation {
     /// Takes no `keep_messages`: there would be no conversation left to keep them in.
     ///
     /// @throws std::invalid_argument for our own account, which is not a contact of ours.
-    void delete_contact(failable_function<void()> cb);
+    void delete_contact(result_function<> cb);
     void delete_contact(await_t);
 };
 

include/session/client/error_codes.hpp

@@ -0,0 +1,39 @@
+#pragma once
+
+#include <string_view>
+
+/// The `Error::code` values Client reports, named so that a caller comparing against one cannot be
+/// defeated by a typo at either end, and so that adding one is a visible change rather than a new
+/// string literal somewhere in a function body.
+///
+/// Dotted and grouped by what failed.  An open set: a caller that meets a code it does not know
+/// falls back to the message, so this list growing is not a breaking change.
+namespace session::client::err {
+
+/// A name or nickname longer than the config will hold.  See `config::validate_contact_name`.
+inline constexpr std::string_view name_too_long = "contacts.name_too_long";
+
+/// The operation names a conversation, message or attachment that is not there.
+inline constexpr std::string_view not_found = "client.not_found";
+
+/// The arguments are wrong in a way the caller could have checked -- a path that is a directory, a
+/// conversation that is not a DM.  Reported rather than thrown only where the check cannot happen
+/// until the work is already on the loop.
+inline constexpr std::string_view invalid_argument = "client.invalid_argument";
+
+/// A file could not be fetched from the file server.  The message carries the status.
+inline constexpr std::string_view download_failed = "attachment.download_failed";
+
+/// A file arrived but could not be decrypted or did not match what was promised.
+inline constexpr std::string_view download_corrupt = "attachment.download_corrupt";
+
+/// A file could not be written where it was asked to go.
+inline constexpr std::string_view save_failed = "attachment.save_failed";
+
+/// An upload to the file server did not complete.  The message carries the status.
+inline constexpr std::string_view upload_failed = "attachment.upload_failed";
+
+/// There is no network attached, so nothing that needs one can be done.
+inline constexpr std::string_view no_network = "client.no_network";
+
+}  // namespace session::client::err

include/session/client/handler.hpp

@@ -10,7 +10,10 @@ namespace session::client {
 /// cannot depend on Client.  See <session/handler.hpp> for what each one means.
 using session::await;
 using session::await_t;
-using session::failable_function;
+using session::Error;
+using session::Expected;
+using session::result_function;
+using session::unexpected;
 
 /// Runs a job on the application's own thread.
 ///

include/session/config/contacts.hpp

@@ -132,20 +132,68 @@ struct contact_info {
 
     /// API: contacts/contact_info::set_name
     ///
-    /// Sets a name or nickname; this is exactly the same as assigning to .name/.nickname directly,
-    /// except that we throw an exception if the given name is longer than MAX_NAME_LENGTH.
+    /// Sets the contact's own name, as assigning to .name does, except that an over-long one is
+    /// put through `fixup_contact_name` rather than rejected: this is their name, arriving in their
+    /// profile, and refusing it would leave us unable to hold the contact at all.
     ///
     /// Inputs:
     /// - `name` -- Name to assign to the contact
     void set_name(std::string name);
+
+    /// API: contacts/contact_info::set_nickname
+    ///
+    /// Sets our own name for the contact, as assigning to .nickname does, except that it throws if
+    /// the nickname is longer than MAX_NAME_LENGTH.  It throws rather than truncating because a
+    /// nickname is something a person here typed, and storing a prefix of what they wrote changes
+    /// what they said.  Check it with `validate_contact_name` before calling this -- or as it is
+    /// typed -- and put the refusal in front of them.
+    ///
+    /// Inputs:
+    /// - `nickname` -- Nickname to assign to the contact
     void set_nickname(std::string nickname);
+
+    /// API: contacts/contact_info::set_nickname_truncated
+    ///
+    /// As `set_nickname`, but truncating rather than throwing.  Only for a caller that has already
+    /// decided truncation is acceptable for what it holds.
+    ///
+    /// Inputs:
+    /// - `nickname` -- Nickname to assign to the contact
     void set_nickname_truncated(std::string nickname);
 
   private:
     friend class Contacts;
     void load(const dict& info_dict);
 };
 
+/// API: contacts/validate_contact_name
+///
+/// Reports what is wrong with `name` as a contact name, nickname or profile name, or nullopt when
+/// nothing is.  The same limits apply to all three, so one check serves them.
+///
+/// A free function needing no account or config, so an application can call it on each keystroke
+/// while a name is being typed, rather than finding out when it tries to store one.  That is the
+/// intended use: anything a person typed should be refused here and corrected by them, because the
+/// alternative -- keeping a prefix of what they wrote -- changes what they said.
+///
+/// Inputs:
+/// - `name` -- the candidate name
+///
+/// Outputs:
+/// - `std::optional<std::string>` -- what is wrong with it, or nullopt if nothing is
+std::optional<std::string> validate_contact_name(std::string_view name);
+
+/// API: contacts/fixup_contact_name
+///
+/// Makes `name` storable as a contact name, changing as little as it can -- today that is
+/// truncating it to MAX_NAME_LENGTH on a utf8 boundary, but it is the place for whatever else
+/// storing a name comes to require.
+///
+/// For names we are *given* rather than told: a peer's profile name has to be stored whatever they
+/// set it to, and refusing it would leave us unable to hold their contact at all.  Never use it on
+/// something a person typed here -- see `validate_contact_name`.
+void fixup_contact_name(std::string& name);
+
 struct blinded_contact_info {
     const std::string session_id() const;  // in hex
     std::string name;