fable: Use [[nodiscard]] where relevant

Implementing this also highlighted some errors and inconsistencies
in the interfaces, which have been resolved.

Author: cassava

fable/include/fable/conf.hpp

@@ -82,17 +82,17 @@ class Conf {
    *   c) network
    * - This method should not throw.
    */
-  bool is_from_file() const { return !file_.empty(); }
+  [[nodiscard]] bool is_from_file() const { return !file_.empty(); }
 
   /**
    * Return the file associated with this configuration.
    */
-  const std::string& file() const { return file_; }
+  [[nodiscard]] const std::string& file() const { return file_; }
 
   /**
    * Return whether this configuration is empty.
    */
-  bool is_empty() const { return data_.is_null(); }
+  [[nodiscard]] bool is_empty() const { return data_.is_null(); }
 
   /**
    * Return a reference to the JSON.
@@ -113,25 +113,25 @@ class Conf {
   /**
    * Return the root of the current JSON as a JSON pointer.
    */
-  std::string root() const { return (root_.empty() ? "/" : root_); }
+  [[nodiscard]] std::string root() const { return (root_.empty() ? "/" : root_); }
 
   /**
    * Return whether the key is present in the configuration.
    *
    * - This method should not throw.
    */
-  bool has(const std::string& key) const { return data_.count(key) != 0; }
-  bool has(const JsonPointer& key) const;
-  bool has_pointer(const std::string& key) const { return has(JsonPointer(key)); }
+  [[nodiscard]] bool has(const std::string& key) const { return data_.count(key) != 0; }
+  [[nodiscard]] bool has(const JsonPointer& key) const;
+  [[nodiscard]] bool has_pointer(const std::string& key) const { return has(JsonPointer(key)); }
 
   /**
    * Return a new Conf basing off the JSON pointer.
    *
    * - Throws a ConfError if the key cannot be found.
    */
-  Conf at(const std::string& key) const;
-  Conf at(const JsonPointer& key) const;
-  Conf at_pointer(const std::string& key) const { return at(JsonPointer(key)); }
+  [[nodiscard]] Conf at(const std::string& key) const;
+  [[nodiscard]] Conf at(const JsonPointer& key) const;
+  [[nodiscard]] Conf at_pointer(const std::string& key) const { return at(JsonPointer(key)); }
 
   /**
    * Erase a key from the Conf if it exists and return the number of elements
@@ -146,15 +146,15 @@ class Conf {
    *
    * - Throws a ConfError if the object is not an array.
    */
-  std::vector<Conf> to_array() const;
+  [[nodiscard]] std::vector<Conf> to_array() const;
 
   /**
    * Return a value of type T.
    *
    * - Throws a ConfError if the key is of the wrong type.
    */
   template <typename T>
-  T get() const {
+  [[nodiscard]] T get() const {
     try {
       return data_.get<T>();
     } catch (nlohmann::detail::type_error&) {
@@ -168,7 +168,7 @@ class Conf {
    * - Throws a ConfError if the key cannot be found or is wrong type.
    */
   template <typename T>
-  T get(const std::string& key) const {
+  [[nodiscard]] T get(const std::string& key) const {
     try {
       return data_.at(key).get<T>();
     } catch (nlohmann::detail::out_of_range&) {
@@ -179,7 +179,7 @@ class Conf {
   }
 
   template <typename T>
-  T get(const JsonPointer& key) const {
+  [[nodiscard]] T get(const JsonPointer& key) const {
     try {
       return data_.at(key).get<T>();
     } catch (nlohmann::detail::out_of_range&) {
@@ -190,7 +190,7 @@ class Conf {
   }
 
   template <typename T>
-  T get_pointer(const std::string& key) const {
+  [[nodiscard]] T get_pointer(const std::string& key) const {
     return get<T>(JsonPointer(key));
   }
 
@@ -199,7 +199,7 @@ class Conf {
    * key cannot be found.
    */
   template <typename T>
-  T get_or(const std::string& key, T def) const {
+  [[nodiscard]] T get_or(const std::string& key, T def) const {
     if (!data_.count(key)) {
       return def;
     }
@@ -211,7 +211,7 @@ class Conf {
   }
 
   template <typename T>
-  T get_or(const JsonPointer& key, T def) const {
+  [[nodiscard]] T get_or(const JsonPointer& key, T def) const {
     try {
       return data_.at(key).get<T>();
     } catch (nlohmann::detail::out_of_range&) {
@@ -222,7 +222,7 @@ class Conf {
   }
 
   template <typename T>
-  T get_pointer_or(const std::string& key, T def) const {
+  [[nodiscard]] T get_pointer_or(const std::string& key, T def) const {
     return get_or(key, def);
   }
 
@@ -318,8 +318,8 @@ class Conf {
    * - If the path is relative and file is not stdin, return relative to
    *   the file.
    */
-  std::filesystem::path resolve_file(const std::filesystem::path& filename) const;
-  std::string resolve_file(const std::string& filename) const;
+  [[nodiscard]] std::filesystem::path resolve_file(const std::filesystem::path& filename) const;
+  [[nodiscard]] std::string resolve_file(const std::string& filename) const;
 
   template <typename... Args>
   [[noreturn]] void throw_error(std::string_view format, Args&&... args) const {

fable/include/fable/confable.hpp

@@ -66,15 +66,15 @@ class Confable {
    * This method uses `schema_impl` under the hood, which the object should
    * implement.
    */
-  Schema& schema();
+  [[nodiscard]] Schema& schema();
 
   /**
    * Return the object schema for description and validation.
    *
    * This method uses `schema_impl` under the hood, which the object should
    * implement.
    */
-  const Schema& schema() const;
+  [[nodiscard]] const Schema& schema() const;
 
   /**
    * Validate a Conf without applying it.
@@ -128,7 +128,7 @@ class Confable {
   /**
    * Serialize a Confable to Json.
    */
-  Json to_json() const;
+  [[nodiscard]] Json to_json() const;
 
  protected:
   /**
@@ -138,7 +138,7 @@ class Confable {
    * object is created or moved, since Schema contains references to fields
    * that (should be) in the object.
    */
-  virtual Schema schema_impl();
+  [[nodiscard]] virtual Schema schema_impl();
 
  private:
   mutable std::unique_ptr<Schema> schema_;

fable/include/fable/environment.hpp

@@ -42,10 +42,10 @@ class Environment {
   Environment(std::map<std::string, std::string>&& defines) : defines_(std::move(defines)) {}
   ~Environment() = default;
 
-  bool prefer_external() const { return prefer_external_; }
+  [[nodiscard]] bool prefer_external() const { return prefer_external_; }
   void prefer_external(bool value) { prefer_external_ = value; }
 
-  bool allow_undefined() const { return allow_undefined_; }
+  [[nodiscard]] bool allow_undefined() const { return allow_undefined_; }
   void allow_undefined(bool value) { allow_undefined_ = value; }
 
   void insert(const std::string& key, const std::string& value) {
@@ -64,10 +64,10 @@ class Environment {
    *
    * This is roughly equivalent to ${KEY}.
    */
-  std::optional<std::string> get(const std::string& key) const {
+  [[nodiscard]] std::optional<std::string> get(const std::string& key) const {
     return get(key, prefer_external_);
   }
-  std::optional<std::string> get(const std::string& key, bool prefer_external) const;
+  [[nodiscard]] std::optional<std::string> get(const std::string& key, bool prefer_external) const;
 
   /**
    * Return the value of a literal key, trying both environment and internal
@@ -77,11 +77,11 @@ class Environment {
    *
    * This is equivalent to ${KEY-ALTERNATIVE}, and cannot fail.
    */
-  std::string get_or(const std::string& key, const std::string& alternative) const {
+  [[nodiscard]] std::string get_or(const std::string& key, const std::string& alternative) const {
     return get(key).value_or(alternative);
   }
-  std::string get_or(const std::string& key, const std::string& alternative,
-                     bool prefer_external) const {
+  [[nodiscard]] std::string get_or(const std::string& key, const std::string& alternative,
+                                   bool prefer_external) const {
     return get(key, prefer_external).value_or(alternative);
   }
 
@@ -93,30 +93,34 @@ class Environment {
    *
    * This is roughly equivalent to ${KEY?out_of_range}.
    */
-  std::string require(const std::string& key) const { return require(key, prefer_external_); }
-  std::string require(const std::string& key, bool prefer_external) const;
+  [[nodiscard]] std::string require(const std::string& key) const {
+    return require(key, prefer_external_);
+  }
+  [[nodiscard]] std::string require(const std::string& key, bool prefer_external) const;
 
   /**
    * Evaluate a single variable, such as "KEY" or "KEY-ALTERNATIVE".
    *
    * Throws std::out_of_range if allow_undefined() is false and
    * std::invalid_argument if a malformed string is supplied.
    */
-  std::string evaluate(const std::string& s) const {
+  [[nodiscard]] std::string evaluate(const std::string& s) const {
     return evaluate(s, prefer_external_, allow_undefined_);
   }
-  std::string evaluate(const std::string& s, bool prefer_external, bool allow_undefined) const;
+  [[nodiscard]] std::string evaluate(const std::string& s, bool prefer_external,
+                                     bool allow_undefined) const;
 
   /**
    * Interpolate a string will evaluate all variable instances in a string.
    *
    * Throws std::out_of_range if allow_undefined() is false and
    * std::invalid_argument if a malformed string is supplied.
    */
-  std::string interpolate(const std::string& s) const {
+  [[nodiscard]] std::string interpolate(const std::string& s) const {
     return interpolate(s, prefer_external_, allow_undefined_);
   }
-  std::string interpolate(const std::string& s, bool prefer_external, bool allow_undefined) const;
+  [[nodiscard]] std::string interpolate(const std::string& s, bool prefer_external,
+                                        bool allow_undefined) const;
 
  private:
   bool prefer_external_{true};

fable/include/fable/error.hpp

@@ -66,12 +66,12 @@ class ConfError : public Error {
   ConfError(const Conf& c, std::string_view format, Args&&... args)
       : Error(format, std::forward<Args>(args)...), data_(c) {}
 
-  std::string file() const { return data_.file(); }
-  std::string root() const { return data_.root(); }
-  const Conf& conf() const { return data_; }
-  const Json& data() const { return *data_; }
+  [[nodiscard]] std::string file() const noexcept { return data_.file(); }
+  [[nodiscard]] std::string root() const noexcept { return data_.root(); }
+  [[nodiscard]] const Conf& conf() const noexcept { return data_; }
+  [[nodiscard]] const Json& data() const noexcept { return *data_; }
 
-  virtual std::string message() const {
+  [[nodiscard]] virtual std::string message() const {
     return fmt::format("{}:{}: {}", file(), root(), this->what());
   }
 
@@ -170,8 +170,8 @@ class SchemaError : public ConfError {
       : ConfError(c, format, std::forward<Args>(args)...), schema_(s), context_(ctx) {}
 
  public:  // Special
-  const Json& schema() const { return schema_; }
-  const Json& context() const { return context_; }
+  [[nodiscard]] const Json& schema() const { return schema_; }
+  [[nodiscard]] const Json& context() const { return context_; }
 
   friend void to_json(Json& j, const SchemaError& e) {
     j = Json{

fable/include/fable/schema.hpp

@@ -229,13 +229,13 @@ class Schema : public schema::Interface {
     return std::move(*this);
   }
 
-  Json json_schema_qualified() const {
+  [[nodiscard]] Json json_schema_qualified() const {
     Json j = impl_->json_schema();
     j["$schema"] = "http://json-schema.org/draft-07/schema#";
     return j;
   }
 
-  Json json_schema_qualified(const std::string& id) const {
+  [[nodiscard]] Json json_schema_qualified(const std::string& id) const {
     Json j = json_schema_qualified();
     j["$id"] = id;
     return j;
@@ -245,16 +245,18 @@ class Schema : public schema::Interface {
 
  public:  // Overrides
   using Interface::to_json;
-  operator schema::Box() const { return schema::Box{impl_}; }
-  Interface* clone() const override { return impl_->clone(); }
-  JsonType type() const override { return impl_->type(); }
-  std::string type_string() const override { return impl_->type_string(); }
-  bool is_required() const override { return impl_->is_required(); }
-  const std::string& description() const override { return impl_->description(); }
+  [[nodiscard]] operator schema::Box() const { return schema::Box{impl_}; }
+  [[nodiscard]] Interface* clone() const override { return impl_->clone(); }
+  [[nodiscard]] JsonType type() const override { return impl_->type(); }
+  [[nodiscard]] std::string type_string() const override { return impl_->type_string(); }
+  [[nodiscard]] bool is_required() const override { return impl_->is_required(); }
+  [[nodiscard]] const std::string& description() const override { return impl_->description(); }
   void set_description(std::string s) override { return impl_->set_description(std::move(s)); }
-  Json usage() const override { return impl_->usage(); }
-  Json json_schema() const override { return impl_->json_schema(); }
-  bool validate(const Conf& c, std::optional<SchemaError>& err) const override { return impl_->validate(c, err); }
+  [[nodiscard]] Json usage() const override { return impl_->usage(); }
+  [[nodiscard]] Json json_schema() const override { return impl_->json_schema(); }
+  bool validate(const Conf& c, std::optional<SchemaError>& err) const override {
+    return impl_->validate(c, err);
+  }
   void to_json(Json& j) const override { impl_->to_json(j); }
   void from_conf(const Conf& c) override { impl_->from_conf(c); }
   void reset_ptr() override { impl_->reset_ptr(); }

fable/include/fable/schema/array.hpp

@@ -115,7 +115,7 @@ class Array : public Base<Array<T, N, P>> {
    *
    * By default, this is false.
    */
-  bool require_all() const { return option_require_all_; }
+  [[nodiscard]] bool require_all() const { return option_require_all_; }
 
   /**
    * Set whether deserialization requires setting the entire array.
@@ -144,15 +144,15 @@ class Array : public Base<Array<T, N, P>> {
    *
    * \see set_require_all(bool)
    */
-  Array<T, N, P> require_all(bool value) && {
+  [[nodiscard]] Array<T, N, P> require_all(bool value) && {
     this->set_require_all(value);
     return std::move(*this);
   }
 
  public:  // Overrides
-  std::string type_string() const override { return "array of " + prototype_.type_string(); }
+  [[nodiscard]] std::string type_string() const override { return "array of " + prototype_.type_string(); }
 
-  Json json_schema() const override {
+  [[nodiscard]] Json json_schema() const override {
     Json j;
     if (option_require_all_) {
       j = this->json_schema_array();
@@ -203,7 +203,7 @@ class Array : public Base<Array<T, N, P>> {
     this->deserialize_into(c, *ptr_);
   }
 
-  Json serialize(const Type& xs) const {
+  [[nodiscard]] Json serialize(const Type& xs) const {
     Json j = Json::array();
     serialize_into(j, xs);
     return j;
@@ -225,7 +225,7 @@ class Array : public Base<Array<T, N, P>> {
    * Because it's not pre-existing, we can't guarantee that it will be
    * initialized and therefore only support setting the full array.
    */
-  Type deserialize(const Conf& c) const {
+  [[nodiscard]] Type deserialize(const Conf& c) const {
     Type array;
     this->deserialize_into(c, array);
     return array;
@@ -244,7 +244,7 @@ class Array : public Base<Array<T, N, P>> {
   void reset_ptr() override { ptr_ = nullptr; }
 
  private:
-  Json json_schema_array() const {
+  [[nodiscard]] Json json_schema_array() const {
     return Json::object({
         {"type", "array"},
         {"items", prototype_.json_schema()},
@@ -253,7 +253,7 @@ class Array : public Base<Array<T, N, P>> {
     });
   }
 
-  Json json_schema_object() const {
+  [[nodiscard]] Json json_schema_object() const {
     return Json::object({
         {"type", "object"},
         {"additionalProperties", false},