Merge pull request #14 from ToruNiina/error-message

improve error message quality

README.md

@@ -8,7 +8,7 @@ toml11
 
 c++11 header-only toml parser depending only on c++ standard library.
 
-compatible to the latest version of TOML v0.5.0 after version 2.0.0.
+compatible to the latest version of [TOML v0.5.0](https://github.com/toml-lang/toml/blob/master/versions/en/toml-v0.5.0.md) after version 2.0.0.
 
 Are you looking for pre-C++11 compatible toml parser? Try [Boost.toml](https://github.com/ToruNiina/Boost.toml)! It has almost the same functionality as this library and works with C++98 & Boost.
 
@@ -64,6 +64,19 @@ terminate called after throwing an instance of 'toml::syntax_error'
    |        ^------ expected newline, but got '='.        # error reason
 ```
 
+If you (mistakenly) duplicate tables and got an error, you may want to see where the other is. toml11 shows both at the same time.
+
+```console
+terminate called after throwing an instance of 'toml::syntax_error'
+  what():  [error] toml::insert_value: table ("table") already exists.
+ --> duplicate-table.toml
+ 1 | [table]
+   | ~~~~~~~ table already exists here
+ ...
+ 3 | [table]
+   | ~~~~~~~ table defined twice
+```
+
 Since the error message generation is generally a difficult task, the current status is not ideal. toml11 needs your help. If you encounter a weird error message, please let us know and contribute to improve the quality!
 
 ### Getting a toml value
@@ -389,6 +402,59 @@ terminate called after throwing an instance of 'std::range_error'
    |                     ~~~~~~~~~ should be in [0x00..0x10FFFF]
 ```
 
+### Formatting your error
+
+When you encounter an error after you read the toml value, you may want to
+show the error with the value.
+
+toml11 provides you a function that formats user-defined error message with
+related values. With a code like the following,
+
+```cpp
+const auto value = toml::find<int>(data, "num");
+if(value < 0)
+{
+    std::cerr << toml::format_error("[error] value should be positive",
+                                    data.at("num"), "positive number required")
+              << std::endl;
+}
+```
+
+you will get an error message like this.
+
+```console
+[error] value should be positive
+ --> example.toml
+ 3 | num = -42
+   |       ~~~ positive number required
+```
+
+When you pass two values to `toml::format_error`,
+
+```cpp
+const auto min = toml::find<int>(range, "min");
+const auto max = toml::find<int>(range, "max");
+if(max < min)
+{
+    std::cerr << toml::format_error("[error] max should be larger than min",
+                                    data.at("min"), "minimum number here",
+                                    data.at("max"), "maximum number here");
+              << std::endl;
+}
+```
+
+you will get an error message like this.
+
+```console
+[error] max should be larger than min
+ --> example.toml
+ 3 | min = 54
+   |       ~~ minimum number here
+ ...
+ 4 | max = 42
+   |       ~~ maximum number here
+```
+
 ## Underlying types
 
 The toml types (can be used as `toml::*` in this library) and corresponding `enum` names are listed in the table below.

tests/CMakeLists.txt

@@ -26,6 +26,7 @@ set(TEST_NAMES
     test_from_toml
     test_parse_file
     test_parse_unicode
+    test_error_detection
 )
 
 CHECK_CXX_COMPILER_FLAG("-Wall" COMPILER_SUPPORTS_WALL)

tests/test_error_detection.cpp

@@ -0,0 +1,199 @@
+#define BOOST_TEST_MODULE "test_error_detection"
+#ifdef UNITTEST_FRAMEWORK_LIBRARY_EXIST
+#include <boost/test/unit_test.hpp>
+#else
+#define BOOST_TEST_NO_LIB
+#include <boost/test/included/unit_test.hpp>
+#endif
+#include <toml.hpp>
+#include <iostream>
+#include <fstream>
+
+BOOST_AUTO_TEST_CASE(test_detect_empty_key)
+{
+    std::istringstream stream(std::string("= \"value\""));
+    bool exception_thrown = false;
+    try
+    {
+        toml::parse(stream, "test_detect_empty_key");
+    }
+    catch(const toml::syntax_error& syn)
+    {
+        // to see the error message
+        std::cerr << syn.what() << std::endl;
+        exception_thrown = true;
+    }
+    BOOST_CHECK(exception_thrown);
+}
+
+BOOST_AUTO_TEST_CASE(test_detect_missing_value)
+{
+    std::istringstream stream(std::string("a ="));
+    bool exception_thrown = false;
+    try
+    {
+        toml::parse(stream, "test_detect_missing_value");
+    }
+    catch(const toml::syntax_error& syn)
+    {
+        std::cerr << syn.what() << std::endl;
+        exception_thrown = true;
+    }
+    BOOST_CHECK(exception_thrown);
+}
+
+BOOST_AUTO_TEST_CASE(test_detect_too_many_value)
+{
+    std::istringstream stream(std::string("a = 1 = \"value\""));
+    bool exception_thrown = false;
+    try
+    {
+        toml::parse(stream, "test_detect_too_many_value");
+    }
+    catch(const toml::syntax_error& syn)
+    {
+        std::cerr << syn.what() << std::endl;
+        exception_thrown = true;
+    }
+    BOOST_CHECK(exception_thrown);
+}
+
+BOOST_AUTO_TEST_CASE(test_detect_duplicate_table)
+{
+    std::istringstream stream(std::string(
+            "[table]\n"
+            "a = 42\n"
+            "[table]\n"
+            "b = 42\n"
+            ));
+    bool exception_thrown = false;
+    try
+    {
+        toml::parse(stream, "test_detect_duplicate_table");
+    }
+    catch(const toml::syntax_error& syn)
+    {
+        std::cerr << syn.what() << std::endl;
+        exception_thrown = true;
+    }
+    BOOST_CHECK(exception_thrown);
+}
+
+BOOST_AUTO_TEST_CASE(test_detect_conflict_array_table)
+{
+    std::istringstream stream(std::string(
+            "[[table]]\n"
+            "a = 42\n"
+            "[table]\n"
+            "b = 42\n"
+            ));
+    bool exception_thrown = false;
+    try
+    {
+        toml::parse(stream, "test_detect_conflict_array_table");
+    }
+    catch(const toml::syntax_error& syn)
+    {
+        std::cerr << syn.what() << std::endl;
+        exception_thrown = true;
+    }
+    BOOST_CHECK(exception_thrown);
+}
+
+BOOST_AUTO_TEST_CASE(test_detect_conflict_table_array)
+{
+    std::istringstream stream(std::string(
+            "[table]\n"
+            "a = 42\n"
+            "[[table]]\n"
+            "b = 42\n"
+            ));
+    bool exception_thrown = false;
+    try
+    {
+        toml::parse(stream, "test_detect_conflict_table_array");
+    }
+    catch(const toml::syntax_error& syn)
+    {
+        std::cerr << syn.what() << std::endl;
+        exception_thrown = true;
+    }
+    BOOST_CHECK(exception_thrown);
+}
+
+BOOST_AUTO_TEST_CASE(test_detect_duplicate_value)
+{
+    std::istringstream stream(std::string(
+            "a = 1\n"
+            "a = 2\n"
+            ));
+    bool exception_thrown = false;
+    try
+    {
+        toml::parse(stream, "test_detect_duplicate_value");
+    }
+    catch(const toml::syntax_error& syn)
+    {
+        std::cerr << syn.what() << std::endl;
+        exception_thrown = true;
+    }
+    BOOST_CHECK(exception_thrown);
+}
+
+BOOST_AUTO_TEST_CASE(test_detect_conflicting_value)
+{
+    std::istringstream stream(std::string(
+            "a.b   = 1\n"
+            "a.b.c = 2\n"
+            ));
+    bool exception_thrown = false;
+    try
+    {
+        toml::parse(stream, "test_detect_conflicting_value");
+    }
+    catch(const toml::syntax_error& syn)
+    {
+        std::cerr << syn.what() << std::endl;
+        exception_thrown = true;
+    }
+    BOOST_CHECK(exception_thrown);
+}
+
+BOOST_AUTO_TEST_CASE(test_detect_inhomogeneous_array)
+{
+    std::istringstream stream(std::string(
+            "a = [1, 1.0]\n"
+            ));
+    bool exception_thrown = false;
+    try
+    {
+        toml::parse(stream, "test_detect_inhomogeneous_array");
+    }
+    catch(const toml::syntax_error& syn)
+    {
+        std::cerr << syn.what() << std::endl;
+        exception_thrown = true;
+    }
+    BOOST_CHECK(exception_thrown);
+}
+
+BOOST_AUTO_TEST_CASE(test_detect_appending_array_of_table)
+{
+    std::istringstream stream(std::string(
+            "a = [{b = 1}]\n"
+            "[[a]]\n"
+            "b = 2\n"
+            ));
+    bool exception_thrown = false;
+    try
+    {
+        toml::parse(stream, "test_detect_appending_array_of_table");
+    }
+    catch(const toml::syntax_error& syn)
+    {
+        std::cerr << syn.what() << std::endl;
+        exception_thrown = true;
+    }
+    BOOST_CHECK(exception_thrown);
+}
+

tests/test_get.cpp

@@ -116,6 +116,14 @@ BOOST_AUTO_TEST_CASE(test_get_exact)
         tab["key3"] = toml::value(123);
         BOOST_CHECK(tab == toml::get<toml::table>(v));
     }
+    {
+        toml::value v1(42);
+        BOOST_CHECK(v1 == toml::get<toml::value>(v1));
+
+        toml::value v2(54);
+        toml::get<toml::value>(v1) = v2;
+        BOOST_CHECK(v2 == toml::get<toml::value>(v1));
+    }
 }
 
 BOOST_AUTO_TEST_CASE(test_get_integer_type)

tests/test_get_related_func.cpp

@@ -68,9 +68,14 @@ BOOST_AUTO_TEST_CASE(test_expect)
     {
         toml::value v1(42);
         toml::value v2(3.14);
-        BOOST_CHECK_EQUAL(42, toml::expect<int>(v1).unwrap_or(0));
-        BOOST_CHECK_EQUAL( 0, toml::expect<int>(v2).unwrap_or(0));
-        BOOST_CHECK_EQUAL("42",   toml::expect<int>(v1).map([](int i){return std::to_string(i);}).unwrap_or(std::string("none")));
-        BOOST_CHECK_EQUAL("none", toml::expect<int>(v2).map([](int i){return std::to_string(i);}).unwrap_or(std::string("none")));
+        const auto v1_or_0 = toml::expect<int>(v1).unwrap_or(0);
+        const auto v2_or_0 = toml::expect<int>(v2).unwrap_or(0);
+        BOOST_CHECK_EQUAL(42, v1_or_0);
+        BOOST_CHECK_EQUAL( 0, v2_or_0);
+
+        const auto v1_or_none = toml::expect<int>(v1).map([](int i){return std::to_string(i);}).unwrap_or(std::string("none"));
+        const auto v2_or_none = toml::expect<int>(v2).map([](int i){return std::to_string(i);}).unwrap_or(std::string("none"));
+        BOOST_CHECK_EQUAL("42",   v1_or_none);
+        BOOST_CHECK_EQUAL("none", v2_or_none);
     }
 }

tests/test_result.cpp

@@ -409,3 +409,33 @@ BOOST_AUTO_TEST_CASE(test_or_else)
         BOOST_CHECK_EQUAL(mapped.unwrap_err(), "hogehoge");
     }
 }
+
+BOOST_AUTO_TEST_CASE(test_and_or_other)
+{
+    {
+        const toml::result<int, std::string> r1(toml::ok(42));
+        const toml::result<int, std::string> r2(toml::err<std::string>("foo"));
+
+        BOOST_CHECK_EQUAL(r1,    r1.or_other(r2));
+        BOOST_CHECK_EQUAL(r2,    r1.and_other(r2));
+        BOOST_CHECK_EQUAL(42,    r1.or_other(r2).unwrap());
+        BOOST_CHECK_EQUAL("foo", r1.and_other(r2).unwrap_err());
+    }
+    {
+        auto r1_gen = []() -> toml::result<int, std::string> {
+            return toml::ok(42);
+        };
+        auto r2_gen = []() -> toml::result<int, std::string> {
+            return toml::err<std::string>("foo");
+        };
+        const auto r3 = r1_gen();
+        const auto r4 = r2_gen();
+
+        BOOST_CHECK_EQUAL(r3, r1_gen().or_other (r2_gen()));
+        BOOST_CHECK_EQUAL(r4, r1_gen().and_other(r2_gen()));
+        BOOST_CHECK_EQUAL(42,    r1_gen().or_other (r2_gen()).unwrap());
+        BOOST_CHECK_EQUAL("foo", r1_gen().and_other(r2_gen()).unwrap_err());
+    }
+}
+
+

toml/get.hpp

@@ -33,6 +33,30 @@ inline T&& get(value&& v)
     return std::move(v.cast<detail::toml_value_t<T>::value>());
 }
 
+// ============================================================================
+// T == toml::value; identity transformation.
+
+template<typename T, typename std::enable_if<
+    std::is_same<T, ::toml::value>::value, std::nullptr_t>::type = nullptr>
+inline T& get(value& v)
+{
+    return v;
+}
+
+template<typename T, typename std::enable_if<
+    std::is_same<T, ::toml::value>::value, std::nullptr_t>::type = nullptr>
+inline T const& get(const value& v)
+{
+    return v;
+}
+
+template<typename T, typename std::enable_if<
+    std::is_same<T, ::toml::value>::value, std::nullptr_t>::type = nullptr>
+inline T&& get(value&& v)
+{
+    return std::move(v);
+}
+
 // ============================================================================
 // integer convertible from toml::Integer
 
@@ -456,52 +480,23 @@ auto get_or(toml::value&& v, const toml::key& ky, T&& opt)
 // expect
 
 template<typename T>
-auto expect(const toml::value& v)
-    -> result<decltype(::toml::get<T>(v)), std::string>
+result<T, std::string> expect(const toml::value& v) noexcept
 {
     try
     {
         return ok(get<T>(v));
     }
-    catch(const type_error& te)
+    catch(const std::exception& e)
     {
-        return err(te.what());
+        return err(e.what());
     }
 }
 template<typename T>
-auto expect(toml::value& v)
-    -> result<decltype(::toml::get<T>(v)), std::string>
+result<T, std::string> expect(const toml::value& v, const toml::key& k) noexcept
 {
     try
     {
-        return ok(get<T>(v));
-    }
-    catch(const type_error& te)
-    {
-        return err(te.what());
-    }
-}
-template<typename T>
-auto expect(toml::value&& v)
-    -> result<decltype(::toml::get<T>(std::move(v))), std::string>
-{
-    try
-    {
-        return ok(get<T>(std::move(v)));
-    }
-    catch(const type_error& te)
-    {
-        return err(te.what());
-    }
-}
-
-template<typename T>
-auto expect(const toml::value& v, const toml::key& k)
-    -> result<decltype(::toml::get<T>(v, k)), std::string>
-{
-    try
-    {
-        return ok(get<T>(v, k));
+        return ok(find<T>(v, k));
     }
     catch(const std::exception& e)
     {
@@ -509,65 +504,12 @@ auto expect(const toml::value& v, const toml::key& k)
     }
 }
 template<typename T>
-auto expect(toml::value& v, const toml::key& k)
-    -> result<decltype(::toml::get<T>(v, k)), std::string>
+result<T, std::string> expect(const toml::table& t, const toml::key& k,
+        std::string tablename = "unknown table") noexcept
 {
     try
     {
-        return ok(get<T>(v, k));
-    }
-    catch(const std::exception& e)
-    {
-        return err(e.what());
-    }
-}
-template<typename T>
-auto expect(toml::value&& v, const toml::key& k)
-    -> result<decltype(::toml::get<T>(std::move(v), k)), std::string>
-{
-    try
-    {
-        return ok(get<T>(std::move(v), k));
-    }
-    catch(const std::exception& e)
-    {
-        return err(e.what());
-    }
-}
-
-template<typename T>
-auto expect(const toml::table& t, const toml::key& k, std::string tn)
-    -> result<decltype(::toml::get<T>(t, k, std::move(tn))), std::string>
-{
-    try
-    {
-        return ok(get<T>(t, k, std::move(tn)));
-    }
-    catch(const std::exception& e)
-    {
-        return err(e.what());
-    }
-}
-template<typename T>
-auto expect(toml::table& t, const toml::key& k, std::string tn)
-    -> result<decltype(::toml::get<T>(t, k, std::move(tn))), std::string>
-{
-    try
-    {
-        return ok(get<T>(t, k, std::move(tn)));
-    }
-    catch(const std::exception& e)
-    {
-        return err(e.what());
-    }
-}
-template<typename T>
-auto expect(toml::table&& t, const toml::key& k, std::string tn)
-    -> result<decltype(::toml::get<T>(std::move(t), k, std::move(tn))), std::string>
-{
-    try
-    {
-        return ok(get<T>(std::move(t), k, std::move(tn)));
+        return ok(find<T>(t, k, std::move(tablename)));
     }
     catch(const std::exception& e)
     {

toml/parser.hpp

@@ -881,7 +881,7 @@ parse_array(location<Container>& loc)
                 throw syntax_error(format_underline(
                     "[error] toml::parse_array: type of elements should be the "
                     "same each other.", region<Container>(loc, first, loc.iter()),
-                    "inhomogenous types"));
+                    "inhomogeneous types"));
             }
             retval.push_back(std::move(val.unwrap()));
         }
@@ -971,7 +971,8 @@ parse_key_value_pair(location<Container>& loc)
         }
         else
         {
-            msg = val.unwrap_err();
+            msg = format_underline("[error] toml::parse_key_value_pair: "
+                    "invalid value format", loc, val.unwrap_err());
         }
         loc.iter() = first;
         return err(msg);
@@ -1035,9 +1036,9 @@ insert_nested_key(table& root, const toml::value& v,
                             "[error] toml::insert_value: array of table (\"",
                             format_dotted_keys(first, last), "\") collides with"
                             " existing value"), get_region(tab->at(k)),
-                            concat_to_string("this ", tab->at(k).type(), "value"
-                            "already exists"), get_region(v), "while inserting"
-                            "this array-of-tables"));
+                            concat_to_string("this ", tab->at(k).type(),
+                            " value already exists"), get_region(v),
+                            "while inserting this array-of-tables"));
                     }
                     array& a = tab->at(k).template cast<toml::value_t::Array>();
                     if(!(a.front().is(value_t::Table)))
@@ -1046,9 +1047,34 @@ insert_nested_key(table& root, const toml::value& v,
                             "[error] toml::insert_value: array of table (\"",
                             format_dotted_keys(first, last), "\") collides with"
                             " existing value"), get_region(tab->at(k)),
-                            concat_to_string("this ", tab->at(k).type(), "value"
-                            "already exists"), get_region(v), "while inserting"
-                            "this array-of-tables"));
+                            concat_to_string("this ", tab->at(k).type(),
+                            " value already exists"), get_region(v),
+                            "while inserting this array-of-tables"));
+                    }
+                    // avoid conflicting array of table like the following.
+                    // ```toml
+                    // a = [{b = 42}] # define a as an array of *inline* tables
+                    // [[a]]          # a is an array of *multi-line* tables
+                    // b = 54
+                    // ```
+                    // Here, from the type information, these cannot be detected
+                    // bacause inline table is also a table.
+                    // But toml v0.5.0 explicitly says it is invalid. The above
+                    // array-of-tables has a static size and appending to the
+                    // array is invalid.
+                    // In this library, multi-line table value has a region
+                    // that points to the key of the table (e.g. [[a]]). By
+                    // comparing the first two letters in key, we can detect
+                    // the array-of-table is inline or multiline.
+                    if(detail::get_region(a.front()).str().substr(0,2) != "[[")
+                    {
+                        throw syntax_error(format_underline(concat_to_string(
+                            "[error] toml::insert_value: array of table (\"",
+                            format_dotted_keys(first, last), "\") collides with"
+                            " existing array-of-tables"), get_region(tab->at(k)),
+                            concat_to_string("this ", tab->at(k).type(),
+                            " value has static size"), get_region(v),
+                            "appending this to the statically sized array"));
                     }
                     a.push_back(v);
                     return ok(true);

toml/region.hpp

@@ -262,10 +262,8 @@ inline std::string format_underline(const std::string& message,
         std::max(line_number1.size(), line_number2.size());
 
     std::ostringstream retval;
-    retval << message;
-    retval << newline;
-    retval << " --> ";
-    retval << reg1.name() << newline;;
+    retval << message << newline;
+    retval << " --> " << reg1.name() << newline;
 //  ---------------------------------------
     retval << ' ' << std::setw(line_num_width) << line_number1;
     retval << " | " << line1 << newline;
@@ -276,7 +274,14 @@ inline std::string format_underline(const std::string& message,
     retval << ' ';
     retval << comment_for_underline1 << newline;
 //  ---------------------------------------
+    if(reg2.name() != reg1.name())
+    {
+        retval << " --> " << reg2.name() << newline;
+    }
+    else
+    {
         retval << " ..." << newline;
+    }
     retval << ' ' << std::setw(line_num_width) << line_number2;
     retval << " | " << line2 << newline;
     retval << make_string(line_num_width + 1, ' ');

toml/result.hpp

@@ -396,23 +396,20 @@ struct result
         return std::move(this->succ.value);
     }
 
-    template<typename U>
-    value_type&       unwrap_or(U& opt) &
+    value_type&       unwrap_or(value_type& opt) &
     {
         if(is_err()) {return opt;}
         return this->succ.value;
     }
-    template<typename U>
-    value_type const& unwrap_or(U const& opt) const&
+    value_type const& unwrap_or(value_type const& opt) const&
     {
         if(is_err()) {return opt;}
         return this->succ.value;
     }
-    template<typename U>
-    value_type&&      unwrap_or(U&& opt) &&
+    value_type        unwrap_or(value_type opt) &&
     {
-        if(is_err()) {return std::move(opt);}
-        return std::move(this->succ.value);
+        if(is_err()) {return opt;}
+        return this->succ.value;
     }
 
     error_type&       unwrap_err() &
@@ -592,6 +589,26 @@ struct result
         return ok(std::move(this->as_ok()));
     }
 
+    // if *this is error, returns *this. otherwise, returns other.
+    result and_other(const result& other) const&
+    {
+        return this->is_err() ? *this : other;
+    }
+    result and_other(result&& other) &&
+    {
+        return this->is_err() ? std::move(*this) : std::move(other);
+    }
+
+    // if *this is okay, returns *this. otherwise, returns other.
+    result or_other(const result& other) const&
+    {
+        return this->is_ok() ? *this : other;
+    }
+    result or_other(result&& other) &&
+    {
+        return this->is_ok() ? std::move(*this) : std::move(other);
+    }
+
     void swap(result<T, E>& other)
     {
         result<T, E> tmp(std::move(*this));
@@ -638,5 +655,22 @@ void swap(result<T, E>& lhs, result<T, E>& rhs)
     return;
 }
 
+// this might be confusing because it eagerly evaluated, while in the other
+// cases operator && and || are short-circuited.
+//
+// template<typename T, typename E>
+// inline result<T, E>
+// operator&&(const result<T, E>& lhs, const result<T, E>& rhs) noexcept
+// {
+//     return lhs.is_ok() ? rhs : lhs;
+// }
+//
+// template<typename T, typename E>
+// inline result<T, E>
+// operator||(const result<T, E>& lhs, const result<T, E>& rhs) noexcept
+// {
+//     return lhs.is_ok() ? lhs : rhs;
+// }
+
 } // toml11
 #endif// TOML11_RESULT_H

toml/value.hpp

@@ -800,5 +800,19 @@ inline bool operator>=(const toml::value& lhs, const toml::value& rhs)
     return !(lhs < rhs);
 }
 
+inline std::string format_error(const std::string& err_msg,
+        const toml::value& v, const std::string& comment)
+{
+    return detail::format_underline(err_msg, detail::get_region(v), comment);
+}
+
+inline std::string format_error(const std::string& err_msg,
+        const toml::value& v1, const std::string& comment1,
+        const toml::value& v2, const std::string& comment2)
+{
+    return detail::format_underline(err_msg, detail::get_region(v1), comment1,
+                                             detail::get_region(v2), comment2);
+}
+
 }// toml
 #endif// TOML11_VALUE