[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>

6 files changed, 100 insertions, 159 deletions

diff --git a/toolsrc/include/vcpkg/base/fwd/json.h b/toolsrc/include/vcpkg/base/fwd/json.h
index 7071103c1..73826127c 100644
--- a/toolsrc/include/vcpkg/base/fwd/json.h
+++ b/toolsrc/include/vcpkg/base/fwd/json.h
@@ -25,13 +25,13 @@ namespace vcpkg::Json
 
         virtual Span<const StringView> valid_fields() const;
 
-        virtual Optional<Type> visit_null(Reader&, StringView);
-        virtual Optional<Type> visit_boolean(Reader&, StringView, bool);
-        virtual Optional<Type> visit_integer(Reader& r, StringView field_name, int64_t i);
-        virtual Optional<Type> visit_number(Reader&, StringView, double);
-        virtual Optional<Type> visit_string(Reader&, StringView, StringView);
-        virtual Optional<Type> visit_array(Reader&, StringView, const Array&);
-        virtual Optional<Type> visit_object(Reader&, StringView, const Object&);
+        virtual Optional<Type> visit_null(Reader&);
+        virtual Optional<Type> visit_boolean(Reader&, bool);
+        virtual Optional<Type> visit_integer(Reader& r, int64_t i);
+        virtual Optional<Type> visit_number(Reader&, double);
+        virtual Optional<Type> visit_string(Reader&, StringView);
+        virtual Optional<Type> visit_array(Reader&, const Array&);
+        virtual Optional<Type> visit_object(Reader&, const Object&);
 
     protected:
         IDeserializer() = default;
diff --git a/toolsrc/include/vcpkg/base/json.h b/toolsrc/include/vcpkg/base/json.h
index 7cdcb0162..e4ce3210b 100644
--- a/toolsrc/include/vcpkg/base/json.h
+++ b/toolsrc/include/vcpkg/base/json.h
@@ -120,7 +120,7 @@ namespace vcpkg::Json
         static Value boolean(bool) noexcept;
         static Value integer(int64_t i) noexcept;
         static Value number(double d) noexcept;
-        static Value string(StringView) noexcept;
+        static Value string(std::string s) noexcept;
         static Value array(Array&&) noexcept;
         static Value array(const Array&) noexcept;
         static Value object(Object&&) noexcept;
@@ -297,113 +297,94 @@ namespace vcpkg::Json
     }
 
     template<class Type>
-    Optional<Type> IDeserializer<Type>::visit_null(Reader&, StringView)
+    Optional<Type> IDeserializer<Type>::visit_null(Reader&)
     {
         return nullopt;
     }
     template<class Type>
-    Optional<Type> IDeserializer<Type>::visit_boolean(Reader&, StringView, bool)
+    Optional<Type> IDeserializer<Type>::visit_boolean(Reader&, bool)
     {
         return nullopt;
     }
     template<class Type>
-    Optional<Type> IDeserializer<Type>::visit_integer(Reader& r, StringView field_name, int64_t i)
+    Optional<Type> IDeserializer<Type>::visit_integer(Reader& r, int64_t i)
     {
-        return this->visit_number(r, field_name, static_cast<double>(i));
+        return this->visit_number(r, static_cast<double>(i));
     }
     template<class Type>
-    Optional<Type> IDeserializer<Type>::visit_number(Reader&, StringView, double)
+    Optional<Type> IDeserializer<Type>::visit_number(Reader&, double)
     {
         return nullopt;
     }
     template<class Type>
-    Optional<Type> IDeserializer<Type>::visit_string(Reader&, StringView, StringView)
+    Optional<Type> IDeserializer<Type>::visit_string(Reader&, StringView)
     {
         return nullopt;
     }
     template<class Type>
-    Optional<Type> IDeserializer<Type>::visit_array(Reader&, StringView, const Array&)
+    Optional<Type> IDeserializer<Type>::visit_array(Reader&, const Array&)
     {
         return nullopt;
     }
     template<class Type>
-    Optional<Type> IDeserializer<Type>::visit_object(Reader&, StringView, const Object&)
+    Optional<Type> IDeserializer<Type>::visit_object(Reader&, const Object&)
     {
         return nullopt;
     }
 
     VCPKG_MSVC_WARNING(pop)
 
-    struct ReaderError
+    struct Reader
     {
-        virtual void add_missing_field(std::string&& type, std::string&& key) = 0;
-        virtual void add_expected_type(std::string&& key, std::string&& expected_type) = 0;
-        virtual void add_extra_fields(std::string&& type, std::vector<std::string>&& fields) = 0;
-        virtual void add_mutually_exclusive_fields(std::string&& type, std::vector<std::string>&& fields) = 0;
-
-        virtual ~ReaderError() = default;
-    };
+        const std::vector<std::string>& errors() const { return m_errors; }
+        std::vector<std::string>& errors() { return m_errors; }
 
-    struct BasicReaderError : ReaderError
-    {
-        virtual void add_missing_field(std::string&& type, std::string&& key) override
-        {
-            missing_fields.emplace_back(std::move(type), std::move(key));
-        }
-        virtual void add_expected_type(std::string&& key, std::string&& expected_type) override
+        void add_missing_field_error(StringView type, StringView key, StringView key_type)
         {
-            expected_types.emplace_back(std::move(key), std::move(expected_type));
+            m_errors.push_back(
+                Strings::concat(path(), " (", type, "): ", "missing required field '", key, "' (", key_type, ")"));
         }
-        virtual void add_extra_fields(std::string&& type, std::vector<std::string>&& fields) override
+        void add_expected_type_error(StringView expected_type)
         {
-            extra_fields.emplace_back(std::move(type), std::move(fields));
+            m_errors.push_back(Strings::concat(path(), ": mismatched type: expected ", expected_type));
         }
-        virtual void add_mutually_exclusive_fields(std::string&& type, std::vector<std::string>&& fields) override
+        void add_extra_fields_error(StringView type, std::vector<std::string>&& fields)
         {
-            mutually_exclusive_fields.emplace_back(std::move(type), std::move(fields));
+            for (auto&& field : fields)
+                m_errors.push_back(Strings::concat(path(), " (", type, "): ", "unexpected field '", field, '\''));
         }
 
-        bool has_error() const
-        {
-            return !missing_fields.empty() || !expected_types.empty() || !extra_fields.empty() ||
-                   !mutually_exclusive_fields.empty();
-        }
-
-        std::vector<std::pair<std::string, std::string>> missing_fields;
-        std::vector<std::pair<std::string, std::string>> expected_types;
-        std::vector<std::pair<std::string, std::vector<std::string>>> extra_fields;
-        std::vector<std::pair<std::string, std::vector<std::string>>> mutually_exclusive_fields;
-    };
-
-    struct Reader
-    {
-        explicit Reader(ReaderError* err) : err(err) { }
-
-        ReaderError& error() const { return *err; }
+        std::string path() const noexcept;
 
     private:
-        ReaderError* err;
+        std::vector<std::string> m_errors;
+        struct Path
+        {
+            int64_t index = -1;
+            StringView field;
+        };
+        std::vector<Path> m_path;
 
         template<class Type>
-        Optional<Type> internal_visit(const Value& value, StringView key, IDeserializer<Type>& visitor)
+        Optional<Type> internal_visit(const Value& value, IDeserializer<Type>& visitor)
         {
             switch (value.kind())
             {
-                case ValueKind::Null: return visitor.visit_null(*this, key);
-                case ValueKind::Boolean: return visitor.visit_boolean(*this, key, value.boolean());
-                case ValueKind::Integer: return visitor.visit_integer(*this, key, value.integer());
-                case ValueKind::Number: return visitor.visit_number(*this, key, value.number());
-                case ValueKind::String: return visitor.visit_string(*this, key, value.string());
-                case ValueKind::Array: return visitor.visit_array(*this, key, value.array());
+                case ValueKind::Null: return visitor.visit_null(*this);
+                case ValueKind::Boolean: return visitor.visit_boolean(*this, value.boolean());
+                case ValueKind::Integer: return visitor.visit_integer(*this, value.integer());
+                case ValueKind::Number: return visitor.visit_number(*this, value.number());
+                case ValueKind::String: return visitor.visit_string(*this, value.string());
+                case ValueKind::Array: return visitor.visit_array(*this, value.array());
                 case ValueKind::Object:
                 {
                     const auto& obj = value.object();
                     check_for_unexpected_fields(obj, visitor.valid_fields(), visitor.type_name());
-                    return visitor.visit_object(*this, key, value.object());
+                    return visitor.visit_object(*this, obj);
                 }
             }
 
-            vcpkg::Checks::exit_fail(VCPKG_LINE_INFO);
+            vcpkg::Checks::unreachable(VCPKG_LINE_INFO);
         }
 
         // returns whether the field was found, not whether it was valid
@@ -416,7 +397,8 @@ namespace vcpkg::Json
                 return false;
             }
 
-            Optional<Type> opt = internal_visit(*value, key, visitor);
+            m_path.push_back({-1, key});
+            Optional<Type> opt = internal_visit(*value, visitor);
 
             if (auto val = opt.get())
             {
@@ -424,149 +406,108 @@ namespace vcpkg::Json
             }
             else
             {
-                err->add_expected_type(key.to_string(), visitor.type_name().to_string());
+                add_expected_type_error(visitor.type_name().to_string());
             }
-
+            m_path.pop_back();
             return true;
         }
 
-        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;
-        }
-
         // checks that an object doesn't contain any fields which both:
         // * don't start with a `$`
         // * are not in `valid_fields`
         // if known_fields.empty(), then it's treated as if all field names are valid
-        void 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())
-            {
-                error().add_extra_fields(type_name.to_string(), std::move(extra_fields));
-            }
-        }
+        void check_for_unexpected_fields(const Object& obj, Span<const StringView> valid_fields, StringView type_name);
 
     public:
-        template<class Type>
+        template<class Type, class Deserializer>
         void required_object_field(
-            StringView type, const Object& obj, StringView key, Type& place, IDeserializer<Type>& visitor)
+            StringView type, const Object& obj, StringView key, Type& place, Deserializer&& visitor)
         {
             if (!internal_field(obj, key, place, visitor))
             {
-                err->add_missing_field(type.to_string(), key.to_string());
+                this->add_missing_field_error(type, key, visitor.type_name());
             }
         }
-        template<class Type>
-        void required_object_field(
-            StringView type, const Object& obj, StringView key, Type& place, IDeserializer<Type>&& visitor)
-        {
-            required_object_field(type, obj, key, place, visitor);
-        }
 
         // returns whether key \in obj
-        template<class Type>
-        bool optional_object_field(const Object& obj, StringView key, Type& place, IDeserializer<Type>& visitor)
+        template<class Type, class Deserializer>
+        bool optional_object_field(const Object& obj, StringView key, Type& place, Deserializer&& visitor)
         {
             return internal_field(obj, key, place, visitor);
         }
-        template<class Type>
-        bool optional_object_field(const Object& obj, StringView key, Type& place, IDeserializer<Type>&& visitor)
-        {
-            return optional_object_field(obj, key, place, visitor);
-        }
 
         template<class Type>
-        Optional<Type> visit_value(const Value& value, StringView key, IDeserializer<Type>& visitor)
+        Optional<Type> visit_value(const Value& value, IDeserializer<Type>& visitor)
         {
-            return internal_visit(value, key, visitor);
+            return internal_visit(value, visitor);
         }
         template<class Type>
-        Optional<Type> visit_value(const Value& value, StringView key, IDeserializer<Type>&& visitor)
+        Optional<Type> visit_value(const Value& value, IDeserializer<Type>&& visitor)
         {
-            return visit_value(value, key, visitor);
+            return visit_value(value, visitor);
         }
 
         template<class Type>
-        Optional<Type> visit_value(const Array& value, StringView key, IDeserializer<Type>& visitor)
+        Optional<Type> visit_value(const Array& value, IDeserializer<Type>& visitor)
         {
-            return visitor.visit_array(*this, key, value);
+            return visitor.visit_array(*this, value);
         }
         template<class Type>
-        Optional<Type> visit_value(const Array& value, StringView key, IDeserializer<Type>&& visitor)
+        Optional<Type> visit_value(const Array& value, IDeserializer<Type>&& visitor)
         {
-            return visit_value(value, key, visitor);
+            return visit_value(value, visitor);
         }
 
         template<class Type>
-        Optional<Type> visit_value(const Object& value, StringView key, IDeserializer<Type>& visitor)
+        Optional<Type> visit_value(const Object& value, IDeserializer<Type>& visitor)
         {
             check_for_unexpected_fields(value, visitor.valid_fields(), visitor.type_name());
-            return visitor.visit_object(*this, key, value);
+            return visitor.visit_object(*this, value);
         }
         template<class Type>
-        Optional<Type> visit_value(const Object& value, StringView key, IDeserializer<Type>&& visitor)
+        Optional<Type> visit_value(const Object& value, IDeserializer<Type>&& visitor)
         {
-            return visit_value(value, key, visitor);
+            return visit_value(value, visitor);
         }
 
         template<class Type>
-        Optional<std::vector<Type>> array_elements(const Array& arr, StringView key, IDeserializer<Type>& visitor)
+        Optional<std::vector<Type>> array_elements(const Array& arr, IDeserializer<Type>& visitor)
         {
             std::vector<Type> result;
-            for (const auto& el : arr)
+            m_path.emplace_back();
+            for (size_t i = 0; i < arr.size(); ++i)
             {
-                auto opt = internal_visit(el, key, visitor);
+                m_path.back().index = static_cast<int64_t>(i);
+                auto opt = internal_visit(arr[i], visitor);
                 if (auto p = opt.get())
                 {
                     result.push_back(std::move(*p));
                 }
                 else
                 {
-                    return nullopt;
+                    this->add_expected_type_error(visitor.type_name());
+                    for (++i; i < arr.size(); ++i)
+                    {
+                        m_path.back().index = static_cast<int64_t>(i);
+                        auto opt2 = internal_visit(arr[i], visitor);
+                        if (!opt2) this->add_expected_type_error(visitor.type_name());
+                    }
                 }
             }
+            m_path.pop_back();
             return std::move(result);
         }
         template<class Type>
-        Optional<std::vector<Type>> array_elements(const Array& arr, StringView key, IDeserializer<Type>&& visitor)
+        Optional<std::vector<Type>> array_elements(const Array& arr, IDeserializer<Type>&& visitor)
         {
-            return array_elements(arr, key, visitor);
+            return array_elements(arr, visitor);
         }
     };
 
     struct StringDeserializer final : IDeserializer<std::string>
     {
         virtual StringView type_name() const override { return type_name_; }
-        virtual Optional<std::string> visit_string(Reader&, StringView, StringView sv) override
-        {
-            return sv.to_string();
-        }
+        virtual Optional<std::string> visit_string(Reader&, StringView sv) override { return sv.to_string(); }
 
         explicit StringDeserializer(StringView type_name_) : type_name_(type_name_) { }
 
@@ -577,14 +518,14 @@ namespace vcpkg::Json
     struct PathDeserializer final : IDeserializer<fs::path>
     {
         virtual StringView type_name() const override { return "a path"; }
-        virtual Optional<fs::path> visit_string(Reader&, StringView, StringView sv) override { return fs::u8path(sv); }
+        virtual Optional<fs::path> visit_string(Reader&, StringView sv) override { return fs::u8path(sv); }
     };
 
     struct NaturalNumberDeserializer final : IDeserializer<int>
     {
         virtual StringView type_name() const override { return "a natural number"; }
 
-        virtual Optional<int> visit_integer(Reader&, StringView, int64_t value) override
+        virtual Optional<int> visit_integer(Reader&, int64_t value) override
         {
             if (value > std::numeric_limits<int>::max() || value < 0)
             {
@@ -598,7 +539,7 @@ namespace vcpkg::Json
     {
         virtual StringView type_name() const override { return "a boolean"; }
 
-        virtual Optional<bool> visit_boolean(Reader&, StringView, bool b) override { return b; }
+        virtual Optional<bool> visit_boolean(Reader&, bool b) override { return b; }
     };
 
     enum class AllowEmpty : bool
@@ -619,13 +560,13 @@ namespace vcpkg::Json
         {
         }
 
-        virtual Optional<type> visit_array(Reader& r, StringView key, const Array& arr) override
+        virtual Optional<type> visit_array(Reader& r, const Array& arr) override
         {
             if (allow_empty_ == AllowEmpty::No && arr.size() == 0)
             {
                 return nullopt;
             }
-            return r.array_elements(arr, key, underlying_visitor_);
+            return r.array_elements(arr, underlying_visitor_);
         }
 
     private:
@@ -638,16 +579,16 @@ namespace vcpkg::Json
     {
         virtual StringView type_name() const override { return "a string or array of strings"; }
 
-        virtual Optional<std::vector<std::string>> visit_string(Reader&, StringView, StringView sv) override
+        virtual Optional<std::vector<std::string>> visit_string(Reader&, StringView sv) override
         {
             std::vector<std::string> out;
             out.push_back(sv.to_string());
             return out;
         }
 
-        virtual Optional<std::vector<std::string>> visit_array(Reader& r, StringView key, const Array& arr) override
+        virtual Optional<std::vector<std::string>> visit_array(Reader& r, const Array& arr) override
         {
-            return r.array_elements(arr, key, StringDeserializer{"a string"});
+            return r.array_elements(arr, StringDeserializer{"a string"});
         }
     };
 
@@ -658,7 +599,7 @@ namespace vcpkg::Json
         // [a-z0-9]+(-[a-z0-9]+)*, plus not any of {prn, aux, nul, con, lpt[1-9], com[1-9], core, default}
         static bool is_ident(StringView sv);
 
-        virtual Optional<std::string> visit_string(Json::Reader&, StringView, StringView sv) override
+        virtual Optional<std::string> visit_string(Json::Reader&, StringView sv) override
         {
             if (is_ident(sv))
             {
@@ -693,7 +634,7 @@ namespace vcpkg::Json
             return true;
         }
 
-        virtual Optional<std::string> visit_string(Json::Reader&, StringView, StringView sv) override
+        virtual Optional<std::string> visit_string(Json::Reader&, StringView sv) override
         {
             if (!is_package_name(sv))
             {
diff --git a/toolsrc/include/vcpkg/configuration.h b/toolsrc/include/vcpkg/configuration.h
index 44440fa04..a4ed38223 100644
--- a/toolsrc/include/vcpkg/configuration.h
+++ b/toolsrc/include/vcpkg/configuration.h
@@ -29,7 +29,7 @@ namespace vcpkg
             return t;
         }
 
-        virtual Optional<Configuration> visit_object(Json::Reader& r, StringView, const Json::Object& obj) override;
+        virtual Optional<Configuration> visit_object(Json::Reader& r, const Json::Object& obj) override;
 
         ConfigurationDeserializer(const VcpkgCmdArguments& args);
 
diff --git a/toolsrc/include/vcpkg/paragraphparser.h b/toolsrc/include/vcpkg/paragraphparser.h
index 4403b3aea..22060f32f 100644
--- a/toolsrc/include/vcpkg/paragraphparser.h
+++ b/toolsrc/include/vcpkg/paragraphparser.h
@@ -20,11 +20,13 @@ namespace vcpkg::Parse
         std::map<std::string, std::vector<std::string>> extra_fields;
         std::map<std::string, std::string> expected_types;
         std::map<std::string, std::vector<std::string>> mutually_exclusive_fields;
+        std::vector<std::string> other_errors;
         std::string error;
 
         bool has_error() const
         {
-            return !missing_fields.empty() || !extra_fields.empty() || !expected_types.empty() || !error.empty();
+            return !missing_fields.empty() || !extra_fields.empty() || !expected_types.empty() ||
+                   !other_errors.empty() || !error.empty();
         }
     };
 
diff --git a/toolsrc/include/vcpkg/registries.h b/toolsrc/include/vcpkg/registries.h
index 78c055dae..100bee170 100644
--- a/toolsrc/include/vcpkg/registries.h
+++ b/toolsrc/include/vcpkg/registries.h
@@ -50,10 +50,8 @@ namespace vcpkg
         virtual StringView type_name() const override;
         virtual Span<const StringView> valid_fields() const override;
 
-        virtual Optional<std::unique_ptr<RegistryImpl>> visit_null(Json::Reader&, StringView) override;
-        virtual Optional<std::unique_ptr<RegistryImpl>> visit_object(Json::Reader&,
-                                                                     StringView,
-                                                                     const Json::Object&) override;
+        virtual Optional<std::unique_ptr<RegistryImpl>> visit_null(Json::Reader&) override;
+        virtual Optional<std::unique_ptr<RegistryImpl>> visit_object(Json::Reader&, const Json::Object&) override;
     };
 
     struct RegistryDeserializer final : Json::IDeserializer<Registry>
@@ -63,7 +61,7 @@ namespace vcpkg
         virtual StringView type_name() const override;
         virtual Span<const StringView> valid_fields() const override;
 
-        virtual Optional<Registry> visit_object(Json::Reader&, StringView, const Json::Object&) override;
+        virtual Optional<Registry> visit_object(Json::Reader&, const Json::Object&) override;
     };
 
     // this type implements the registry fall back logic from the registries RFC:
diff --git a/toolsrc/include/vcpkg/vcpkgpaths.h b/toolsrc/include/vcpkg/vcpkgpaths.h
index ae2ee3823..8abc84129 100644
--- a/toolsrc/include/vcpkg/vcpkgpaths.h
+++ b/toolsrc/include/vcpkg/vcpkgpaths.h
@@ -108,7 +108,7 @@ namespace vcpkg
         const std::string& get_tool_version(const std::string& tool) const;
 
         Optional<const Json::Object&> get_manifest() const;
-        Optional<const Json::JsonStyle&> get_manifest_style() const;
+        Optional<const fs::path&> get_manifest_path() const;
         const Configuration& get_configuration() const;
 
         /// <summary>Retrieve a toolset matching a VS version</summary>