Skip to content

[vcpkg] Improve Json error messages #12981

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ras0219-msft merged 5 commits into from
Sep 7, 2020
Merged

[vcpkg] Improve Json error messages #12981

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4ca7250
[vcpkg] Fix error reporting on json parse failure
ras0219-msft Sep 4, 2020
ad6cae6
[vcpkg] Track manifest path for use in diagnostics
ras0219-msft Sep 4, 2020
aec1dcd
[vcpkg] Use by-value for consumer API. Improve trailing comma diagnos…
ras0219-msft Sep 4, 2020
21736c5
[vcpkg] Track errors directly inside Json::Reader
ras0219-msft Sep 4, 2020
087fbe6
[vcpkg] Fixup use of .u8string()
ras0219-msft Sep 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions toolsrc/include/vcpkg/base/fwd/json.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
229 changes: 85 additions & 144 deletions toolsrc/include/vcpkg/base/json.h
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion toolsrc/include/vcpkg/configuration.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
4 changes: 3 additions & 1 deletion toolsrc/include/vcpkg/paragraphparser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
};

Expand Down
8 changes: 3 additions & 5 deletions toolsrc/include/vcpkg/registries.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion toolsrc/include/vcpkg/vcpkgpaths.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
15 changes: 13 additions & 2 deletions toolsrc/src/vcpkg-test/json.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -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: ,
^
)");
}
73 changes: 62 additions & 11 deletions toolsrc/src/vcpkg/base/json.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
{
Expand Down Expand Up @@ -807,6 +803,7 @@ namespace vcpkg::Json
}
else if (current == ',')
{
auto comma_loc = cur_loc();
next();
skip_whitespace();
current = cur();
Expand All @@ -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));
}
}
Expand Down Expand Up @@ -903,6 +900,7 @@ namespace vcpkg::Json
}
else if (current == ',')
{
auto comma_loc = cur_loc();
next();
skip_whitespace();
current = cur();
Expand All @@ -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();
}
}
Expand Down Expand Up @@ -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;
}

}
7 changes: 6 additions & 1 deletion toolsrc/src/vcpkg/base/parse.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
4 changes: 1 addition & 3 deletions toolsrc/src/vcpkg/configuration.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
10 changes: 6 additions & 4 deletions toolsrc/src/vcpkg/install.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
14 changes: 5 additions & 9 deletions toolsrc/src/vcpkg/registries.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,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;
Expand All @@ -57,14 +53,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)));
}
Expand All @@ -86,9 +82,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())
{
Expand Down
Loading