Merge pull request #14 from ToruNiina/error-message

improve error message quality

tests/CMakeLists.txt

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

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);
+}
+

toml/parser.hpp

@@ -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"
+                            concat_to_string("this ", tab->at(k).type(),
-                            "already exists"), get_region(v), "while inserting"
+                            " value already exists"), get_region(v),
-                            "this array-of-tables"));
+                            "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"
+                            concat_to_string("this ", tab->at(k).type(),
-                            "already exists"), get_region(v), "while inserting"
+                            " value already exists"), get_region(v),
-                            "this array-of-tables"));
+                            "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);