[vcpkg] Improve Json error messages (#12981)

* [vcpkg] Fix error reporting on json parse failure

* [vcpkg] Track manifest path for use in diagnostics

* [vcpkg] Use by-value for consumer API. Improve trailing comma diagnostic.

* [vcpkg] Track errors directly inside Json::Reader

* [vcpkg] Fixup use of .u8string()

Co-authored-by: Robert Schumacher <roschuma@microsoft.com>

8 files changed, 137 insertions, 115 deletions

diff --git a/toolsrc/src/vcpkg-test/json.cpp b/toolsrc/src/vcpkg-test/json.cpp
index 0b9941861..6858a5ac8 100644
--- a/toolsrc/src/vcpkg-test/json.cpp
+++ b/toolsrc/src/vcpkg-test/json.cpp
@@ -28,8 +28,8 @@ static std::string mystringify(const Value& val) { return Json::stringify(val, J
 
 TEST_CASE ("JSON stringify weird strings", "[json]")
 {
-    vcpkg::StringView str = U8_STR("😀 😁 😂 🤣 😃 😄 😅 😆 😉");
-    REQUIRE(mystringify(Value::string(str)) == ('"' + str.to_string() + "\"\n"));
+    std::string str = U8_STR("😀 😁 😂 🤣 😃 😄 😅 😆 😉");
+    REQUIRE(mystringify(Value::string(str)) == ('"' + str + "\"\n"));
     REQUIRE(mystringify(Value::string("\xED\xA0\x80")) == "\"\\ud800\"\n"); // unpaired surrogate
 }
 
@@ -228,3 +228,14 @@ TEST_CASE ("JSON parse full file", "[json]")
     }
     REQUIRE(res);
 }
+
+TEST_CASE ("JSON track newlines", "[json]")
+{
+    auto res = Json::parse("{\n,", fs::u8path("filename"));
+    REQUIRE(!res);
+    REQUIRE(res.error()->format() ==
+            R"(Error: filename:2:1: Unexpected character; expected property name
+   on expression: ,
+                  ^
+)");
+}
diff --git a/toolsrc/src/vcpkg/base/json.cpp b/toolsrc/src/vcpkg/base/json.cpp
index 58121ac8e..495a36e32 100644
--- a/toolsrc/src/vcpkg/base/json.cpp
+++ b/toolsrc/src/vcpkg/base/json.cpp
@@ -253,15 +253,15 @@ namespace vcpkg::Json
         val.underlying_ = std::make_unique<ValueImpl>(ValueKindConstant<VK::Number>(), d);
         return val;
     }
-    Value Value::string(StringView sv) noexcept
+    Value Value::string(std::string s) noexcept
     {
-        if (!Unicode::utf8_is_valid_string(sv.begin(), sv.end()))
+        if (!Unicode::utf8_is_valid_string(s.data(), s.data() + s.size()))
         {
-            Debug::print("Invalid string: ", sv, '\n');
-            vcpkg::Checks::exit_with_message(VCPKG_LINE_INFO, "Invalid utf8 passed to Value::string(StringView)");
+            Debug::print("Invalid string: ", s, '\n');
+            vcpkg::Checks::exit_with_message(VCPKG_LINE_INFO, "Invalid utf8 passed to Value::string(std::string)");
         }
         Value val;
-        val.underlying_ = std::make_unique<ValueImpl>(ValueKindConstant<VK::String>(), sv.to_string());
+        val.underlying_ = std::make_unique<ValueImpl>(ValueKindConstant<VK::String>(), std::move(s));
         return val;
     }
     Value Value::array(Array&& arr) noexcept
@@ -489,10 +489,6 @@ namespace vcpkg::Json
             {
                 return code_point == '-' || is_digit(code_point);
             }
-            static bool is_keyword_start(char32_t code_point) noexcept
-            {
-                return code_point == 'f' || code_point == 'n' || code_point == 't';
-            }
 
             static unsigned char from_hex_digit(char32_t code_point) noexcept
             {
@@ -807,6 +803,7 @@ namespace vcpkg::Json
                     }
                     else if (current == ',')
                     {
+                        auto comma_loc = cur_loc();
                         next();
                         skip_whitespace();
                         current = cur();
@@ -817,7 +814,7 @@ namespace vcpkg::Json
                         }
                         if (current == ']')
                         {
-                            add_error("Trailing comma in array");
+                            add_error("Trailing comma in array", comma_loc);
                             return Value::array(std::move(arr));
                         }
                     }
@@ -903,6 +900,7 @@ namespace vcpkg::Json
                     }
                     else if (current == ',')
                     {
+                        auto comma_loc = cur_loc();
                         next();
                         skip_whitespace();
                         current = cur();
@@ -913,7 +911,7 @@ namespace vcpkg::Json
                         }
                         else if (current == '}')
                         {
-                            add_error("Trailing comma in an object");
+                            add_error("Trailing comma in an object", comma_loc);
                             return Value();
                         }
                     }
@@ -1260,4 +1258,57 @@ namespace vcpkg::Json
     }
     // } auto stringify()
 
+    static std::vector<std::string> invalid_json_fields(const Json::Object& obj,
+                                                        Span<const StringView> known_fields) noexcept
+    {
+        const auto field_is_unknown = [known_fields](StringView sv) {
+            // allow directives
+            if (sv.size() != 0 && *sv.begin() == '$')
+            {
+                return false;
+            }
+            return std::find(known_fields.begin(), known_fields.end(), sv) == known_fields.end();
+        };
+
+        std::vector<std::string> res;
+        for (const auto& kv : obj)
+        {
+            if (field_is_unknown(kv.first))
+            {
+                res.push_back(kv.first.to_string());
+            }
+        }
+
+        return res;
+    }
+
+    void Reader::check_for_unexpected_fields(const Object& obj,
+                                             Span<const StringView> valid_fields,
+                                             StringView type_name)
+    {
+        if (valid_fields.size() == 0)
+        {
+            return;
+        }
+
+        auto extra_fields = invalid_json_fields(obj, valid_fields);
+        if (!extra_fields.empty())
+        {
+            add_extra_fields_error(type_name.to_string(), std::move(extra_fields));
+        }
+    }
+
+    std::string Reader::path() const noexcept
+    {
+        std::string p("$");
+        for (auto&& s : m_path)
+        {
+            if (s.index < 0)
+                Strings::append(p, '.', s.field);
+            else
+                Strings::append(p, '[', s.index, ']');
+        }
+        return p;
+    }
+
 }
diff --git a/toolsrc/src/vcpkg/base/parse.cpp b/toolsrc/src/vcpkg/base/parse.cpp
index 669321c05..c6a7de83d 100644
--- a/toolsrc/src/vcpkg/base/parse.cpp
+++ b/toolsrc/src/vcpkg/base/parse.cpp
@@ -66,10 +66,15 @@ namespace vcpkg::Parse
         {
             return Unicode::end_of_file;
         }
+        auto ch = *m_it;
         // See https://www.gnu.org/prep/standards/standards.html#Errors
-        advance_rowcol(*m_it, m_row, m_column);
+        advance_rowcol(ch, m_row, m_column);
 
         ++m_it;
+        if (ch == '\n')
+        {
+            m_start_of_line = m_it;
+        }
         if (m_it != m_it.end() && Unicode::utf16_is_surrogate_code_point(*m_it))
         {
             m_it = m_it.end();
diff --git a/toolsrc/src/vcpkg/configuration.cpp b/toolsrc/src/vcpkg/configuration.cpp
index 1bce07acf..9363abe80 100644
--- a/toolsrc/src/vcpkg/configuration.cpp
+++ b/toolsrc/src/vcpkg/configuration.cpp
@@ -5,9 +5,7 @@
 
 namespace vcpkg
 {
-    Optional<Configuration> ConfigurationDeserializer::visit_object(Json::Reader& r,
-                                                                    StringView,
-                                                                    const Json::Object& obj)
+    Optional<Configuration> ConfigurationDeserializer::visit_object(Json::Reader& r, const Json::Object& obj)
     {
         RegistrySet registries;
 
diff --git a/toolsrc/src/vcpkg/install.cpp b/toolsrc/src/vcpkg/install.cpp
index 06ae14ed9..0a16294a1 100644
--- a/toolsrc/src/vcpkg/install.cpp
+++ b/toolsrc/src/vcpkg/install.cpp
@@ -788,13 +788,15 @@ namespace vcpkg::Install
             {
                 pkgsconfig = fs::u8path(it_pkgsconfig->second);
             }
-
-            auto maybe_manifest_scf = SourceControlFile::parse_manifest_file("manifest", *manifest);
+            auto manifest_path = paths.get_manifest_path().value_or_exit(VCPKG_LINE_INFO);
+            auto maybe_manifest_scf = SourceControlFile::parse_manifest_file(manifest_path, *manifest);
             if (!maybe_manifest_scf)
             {
                 print_error_message(maybe_manifest_scf.error());
-                Checks::exit_with_message(
-                    VCPKG_LINE_INFO, "Failed to parse manifest %s/vcpkg.json.", fs::u8string(paths.manifest_root_dir));
+                System::print2(
+                    "See https://github.com/Microsoft/vcpkg/tree/master/docs/specifications/manifests.md for "
+                    "more information.\n");
+                Checks::exit_fail(VCPKG_LINE_INFO);
             }
             auto& manifest_scf = *maybe_manifest_scf.value_or_exit(VCPKG_LINE_INFO);
 
diff --git a/toolsrc/src/vcpkg/registries.cpp b/toolsrc/src/vcpkg/registries.cpp
index eda20f1b9..aa9a3a336 100644
--- a/toolsrc/src/vcpkg/registries.cpp
+++ b/toolsrc/src/vcpkg/registries.cpp
@@ -45,13 +45,9 @@ namespace vcpkg
         return t;
     }
 
-    Optional<std::unique_ptr<RegistryImpl>> RegistryImplDeserializer::visit_null(Json::Reader&, StringView)
-    {
-        return nullptr;
-    }
+    Optional<std::unique_ptr<RegistryImpl>> RegistryImplDeserializer::visit_null(Json::Reader&) { return nullptr; }
 
     Optional<std::unique_ptr<RegistryImpl>> RegistryImplDeserializer::visit_object(Json::Reader& r,
-                                                                                   StringView,
                                                                                    const Json::Object& obj)
     {
         std::string kind;
@@ -62,14 +58,14 @@ namespace vcpkg
         {
             if (obj.contains(PATH))
             {
-                r.error().add_extra_fields(type_name().to_string(), {PATH});
+                r.add_extra_fields_error("a builtin registry", {PATH});
             }
             return static_cast<std::unique_ptr<RegistryImpl>>(std::make_unique<BuiltinRegistry>());
         }
         else if (kind == KIND_DIRECTORY)
         {
             fs::path path;
-            r.required_object_field(type_name(), obj, PATH, path, Json::PathDeserializer{});
+            r.required_object_field("a directory registry", obj, PATH, path, Json::PathDeserializer{});
 
             return static_cast<std::unique_ptr<RegistryImpl>>(std::make_unique<DirectoryRegistry>(std::move(path)));
         }
@@ -93,9 +89,9 @@ namespace vcpkg
         return t;
     }
 
-    Optional<Registry> RegistryDeserializer::visit_object(Json::Reader& r, StringView key, const Json::Object& obj)
+    Optional<Registry> RegistryDeserializer::visit_object(Json::Reader& r, const Json::Object& obj)
     {
-        auto impl = RegistryImplDeserializer{}.visit_object(r, key, obj);
+        auto impl = RegistryImplDeserializer{}.visit_object(r, obj);
 
         if (!impl.has_value())
         {
diff --git a/toolsrc/src/vcpkg/sourceparagraph.cpp b/toolsrc/src/vcpkg/sourceparagraph.cpp
index 94a5dbe41..27861f873 100644
--- a/toolsrc/src/vcpkg/sourceparagraph.cpp
+++ b/toolsrc/src/vcpkg/sourceparagraph.cpp
@@ -361,7 +361,7 @@ namespace vcpkg
     {
         virtual StringView type_name() const override { return "a platform expression"; }
 
-        virtual Optional<PlatformExpression::Expr> visit_string(Json::Reader&, StringView, StringView sv) override
+        virtual Optional<PlatformExpression::Expr> visit_string(Json::Reader&, StringView sv) override
         {
             auto opt =
                 PlatformExpression::parse_platform_expression(sv, PlatformExpression::MultipleBinaryOperators::Deny);
@@ -398,7 +398,7 @@ namespace vcpkg
             return t;
         }
 
-        virtual Optional<Dependency> visit_string(Json::Reader&, StringView, StringView sv) override
+        virtual Optional<Dependency> visit_string(Json::Reader&, StringView sv) override
         {
             if (!Json::PackageNameDeserializer::is_package_name(sv))
             {
@@ -410,7 +410,7 @@ namespace vcpkg
             return dep;
         }
 
-        virtual Optional<Dependency> visit_object(Json::Reader& r, StringView, const Json::Object& obj) override
+        virtual Optional<Dependency> visit_object(Json::Reader& r, const Json::Object& obj) override
         {
             Dependency dep;
 
@@ -462,7 +462,6 @@ namespace vcpkg
         }
 
         virtual Optional<std::unique_ptr<FeatureParagraph>> visit_object(Json::Reader& r,
-                                                                         StringView,
                                                                          const Json::Object& obj) override
         {
             auto feature = std::make_unique<FeatureParagraph>();
@@ -516,7 +515,7 @@ namespace vcpkg
 #include "spdx-licenses.inc"
             ;
 
-        virtual Optional<std::string> visit_string(Json::Reader&, StringView, StringView sv) override
+        virtual Optional<std::string> visit_string(Json::Reader&, StringView sv) override
         {
             Mode mode = Mode::ExpectExpression;
             size_t open_parens = 0;
@@ -677,7 +676,6 @@ namespace vcpkg
         }
 
         virtual Optional<std::unique_ptr<SourceControlFile>> visit_object(Json::Reader& r,
-                                                                          StringView,
                                                                           const Json::Object& obj) override
         {
             auto control_file = std::make_unique<SourceControlFile>();
@@ -752,44 +750,16 @@ namespace vcpkg
     Parse::ParseExpected<SourceControlFile> SourceControlFile::parse_manifest_file(const fs::path& path_to_manifest,
                                                                                    const Json::Object& manifest)
     {
-        struct JsonErr final : Json::ReaderError
-        {
-            ParseControlErrorInfo pcei;
+        Json::Reader reader;
 
-            void add_missing_field(std::string&& type, std::string&& key) override
-            {
-                pcei.missing_fields[std::move(type)].push_back(std::move(key));
-            }
-            void add_expected_type(std::string&& key, std::string&& expected_type) override
-            {
-                pcei.expected_types.emplace(std::move(key), std::move(expected_type));
-            }
-            void add_extra_fields(std::string&& type, std::vector<std::string>&& fields) override
-            {
-                if (!fields.empty())
-                {
-                    auto& fields_for_type = pcei.extra_fields[std::move(type)];
-                    fields_for_type.insert(fields_for_type.end(), fields.begin(), fields.end());
-                }
-            }
-            void add_mutually_exclusive_fields(std::string&& type, std::vector<std::string>&& fields) override
-            {
-                if (!fields.empty())
-                {
-                    auto& fields_for_type = pcei.mutually_exclusive_fields[std::move(type)];
-                    fields_for_type.insert(fields_for_type.end(), fields.begin(), fields.end());
-                }
-            }
-        } err = {};
-        auto visit = Json::Reader{&err};
-
-        err.pcei.name = fs::u8string(path_to_manifest);
-
-        auto res = visit.visit_value(manifest, "$", ManifestDeserializer{});
+        auto res = reader.visit_value(manifest, ManifestDeserializer{});
 
-        if (err.pcei.has_error())
+        if (!reader.errors().empty())
         {
-            return std::make_unique<ParseControlErrorInfo>(std::move(err.pcei));
+            auto err = std::make_unique<ParseControlErrorInfo>();
+            err->name = fs::u8string(path_to_manifest);
+            err->other_errors = std::move(reader.errors());
+            return std::move(err);
         }
         else if (auto p = res.get())
         {
@@ -813,6 +783,13 @@ namespace vcpkg
                 System::print2(
                     System::Color::error, "Error: while loading ", error_info->name, ":\n", error_info->error, '\n');
             }
+
+            if (!error_info->other_errors.empty())
+            {
+                System::print2(System::Color::error, "Errors occurred while parsing ", error_info->name, "\n");
+                for (auto&& msg : error_info->other_errors)
+                    System::print2("    ", msg, '\n');
+            }
         }
 
         bool have_remaining_fields = false;
@@ -839,9 +816,6 @@ namespace vcpkg
             System::print2("This is the list of valid fields for CONTROL files (case-sensitive): \n\n    ",
                            Strings::join("\n    ", get_list_of_valid_fields()),
                            "\n\n");
-            System::print2("And this is the list of valid fields for manifest files: \n\n    ",
-                           Strings::join("\n    ", ManifestDeserializer{}.valid_fields()),
-                           "\n\n");
 #if defined(_WIN32)
             auto bootstrap = ".\\bootstrap-vcpkg.bat";
 #else
diff --git a/toolsrc/src/vcpkg/vcpkgpaths.cpp b/toolsrc/src/vcpkg/vcpkgpaths.cpp
index 677ac1a09..9f4ffce6b 100644
--- a/toolsrc/src/vcpkg/vcpkgpaths.cpp
+++ b/toolsrc/src/vcpkg/vcpkgpaths.cpp
@@ -84,50 +84,22 @@ namespace
 
 namespace vcpkg
 {
-    static Configuration deserialize_configuration(const Json::Object& obj, const VcpkgCmdArguments& args)
+    static Configuration deserialize_configuration(const Json::Object& obj,
+                                                   const VcpkgCmdArguments& args,
+                                                   const fs::path& filepath)
     {
-        Json::BasicReaderError err;
-        Json::Reader reader{&err};
+        Json::Reader reader;
         auto deserializer = ConfigurationDeserializer(args);
 
-        auto parsed_config_opt = reader.visit_value(obj, "$", deserializer);
-        if (err.has_error())
+        auto parsed_config_opt = reader.visit_value(obj, deserializer);
+        if (!reader.errors().empty())
         {
-            if (!err.missing_fields.empty())
-            {
-                System::print2(System::Color::error, "Error: missing fields in configuration:\n");
-                for (const auto& missing : err.missing_fields)
-                {
-                    System::printf(
-                        System::Color::error, "    %s was expected to have: %s\n", missing.first, missing.second);
-                }
-            }
-            if (!err.expected_types.empty())
-            {
-                System::print2(System::Color::error, "Error: Invalid types in configuration:\n");
-                for (const auto& expected : err.expected_types)
-                {
-                    System::printf(
-                        System::Color::error, "    %s was expected to be %s\n", expected.first, expected.second);
-                }
-            }
-            if (!err.extra_fields.empty())
-            {
-                System::print2(System::Color::error, "Error: Invalid fields in configuration:\n");
-                for (const auto& extra : err.extra_fields)
-                {
-                    System::printf(System::Color::error,
-                                   "    %s had invalid fields: %s\n",
-                                   extra.first,
-                                   Strings::join(", ", extra.second));
-                }
-            }
-            if (!err.mutually_exclusive_fields.empty())
-            {
-                // this should never happen
-                Checks::unreachable(VCPKG_LINE_INFO);
-            }
+            System::print2(System::Color::error, "Errors occurred while parsing ", fs::u8string(filepath), "\n");
+            for (auto&& msg : reader.errors())
+                System::print2("    ", msg, '\n');
 
+            System::print2("See https://github.com/Microsoft/vcpkg/tree/master/docs/specifications/registries.md for "
+                           "more information.\n");
             Checks::exit_fail(VCPKG_LINE_INFO);
         }
 
@@ -157,7 +129,7 @@ namespace vcpkg
         if (!manifest_opt.has_value())
         {
             Checks::exit_with_message(VCPKG_LINE_INFO,
-                                      "Failed to parse manifest at %s: %s",
+                                      "Failed to parse manifest at %s:\n%s",
                                       fs::u8string(manifest_path),
                                       manifest_opt.error()->format());
         }
@@ -216,7 +188,7 @@ namespace vcpkg
         }
         auto config_obj = std::move(parsed_config.first.object());
 
-        return {std::move(config_dir), deserialize_configuration(config_obj, args)};
+        return {std::move(config_dir), deserialize_configuration(config_obj, args, path_to_config)};
     }
 
     namespace details
@@ -244,6 +216,7 @@ namespace vcpkg
             fs::SystemHandle file_lock_handle;
 
             Optional<std::pair<Json::Object, Json::JsonStyle>> m_manifest_doc;
+            fs::path m_manifest_path;
             Configuration m_config;
         };
     }
@@ -305,6 +278,7 @@ namespace vcpkg
             }
 
             m_pimpl->m_manifest_doc = load_manifest(filesystem, manifest_root_dir);
+            m_pimpl->m_manifest_path = manifest_root_dir / fs::u8path("vcpkg.json");
         }
         else
         {
@@ -495,6 +469,17 @@ If you wish to silence this error and use classic mode, you can:
             return nullopt;
         }
     }
+    Optional<const fs::path&> VcpkgPaths::get_manifest_path() const
+    {
+        if (m_pimpl->m_manifest_doc)
+        {
+            return m_pimpl->m_manifest_path;
+        }
+        else
+        {
+            return nullopt;
+        }
+    }
 
     const Configuration& VcpkgPaths::get_configuration() const { return m_pimpl->m_config; }