Skip to content

[MsgPack] Use JSON schema boolean resolution rules #170561

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

Open
slinder1 wants to merge 2 commits into
base: main
Choose a base branch
from
+101 −4
Open

[MsgPack] Use JSON schema boolean resolution rules #170561

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3b29886
[MsgPack] Use JSON schema boolean resolution rules
slinder1 Oct 1, 2025
8fb1156
Rename test and add !bool tag cases
slinder1 Dec 4, 2025
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
19 changes: 15 additions & 4 deletions llvm/lib/BinaryFormat/MsgPackDocumentYAML.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ struct ScalarDocNode : DocNode {
StringRef getYAMLTag() const;
};

bool isJSONSchemaBoolLiteral(StringRef S) {
return S == "true" || S == "false";
}

} // namespace

/// Convert this DocNode to a string, assuming it is scalar.
Expand Down Expand Up @@ -84,11 +88,18 @@ StringRef DocNode::fromString(StringRef S, StringRef Tag) {
*this = getDocument()->getNode();
return "";
}
if (Tag == "!bool" || Tag == "") {
if (Tag == "!bool") {
*this = getDocument()->getNode(false);
StringRef Err = yaml::ScalarTraits<bool>::input(S, nullptr, getBool());
if (Err == "" || Tag != "")
return Err;
return yaml::ScalarTraits<bool>::input(S, nullptr, getBool());
}
// FIXME: This enforces the "JSON Schema" tag resolution for boolean literals,
// defined at https://yaml.org/spec/1.2.2/#json-schema which we adopt because
// the more general pre-1.2 resolution includes many common strings (e.g. "n",
// "no", "y", "yes", ...). This should be handled at the YAMLTraits level, but
// that change would have a much broader impact.
if (Tag == "" && isJSONSchemaBoolLiteral(S)) {
*this = getDocument()->getNode(S == "true");
return "";
}
if (Tag == "!float" || Tag == "") {
*this = getDocument()->getNode(0.0);
Expand Down
74 changes: 74 additions & 0 deletions llvm/unittests/BinaryFormat/MsgPackDocumentTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,3 +420,77 @@ TEST(MsgPackDocument, TestInputYAMLMap) {
ASSERT_EQ(SS.getKind(), Type::String);
ASSERT_EQ(SS.getString(), "2");
}

TEST(MsgPackDocument, TestInputYAMLBoolean) {
Document Doc;
auto GetFirst = [](Document &Doc) { return Doc.getRoot().getArray()[0]; };
auto ToYAML = [](Document &Doc) {
std::string S;
raw_string_ostream OS(S);
Doc.toYAML(OS);
return S;
};

bool Ok;

Ok = Doc.fromYAML("- n\n");
ASSERT_TRUE(Ok);
ASSERT_EQ(GetFirst(Doc).getKind(), Type::String);
ASSERT_EQ(GetFirst(Doc).getString(), "n");
ASSERT_EQ(ToYAML(Doc), "---\n- n\n...\n");

Ok = Doc.fromYAML("- y\n");
ASSERT_TRUE(Ok);
ASSERT_EQ(GetFirst(Doc).getKind(), Type::String);
ASSERT_EQ(GetFirst(Doc).getString(), "y");
ASSERT_EQ(ToYAML(Doc), "---\n- y\n...\n");

Ok = Doc.fromYAML("- no\n");
ASSERT_TRUE(Ok);
ASSERT_EQ(GetFirst(Doc).getKind(), Type::String);
ASSERT_EQ(GetFirst(Doc).getString(), "no");
ASSERT_EQ(ToYAML(Doc), "---\n- no\n...\n");

Ok = Doc.fromYAML("- yes\n");
ASSERT_TRUE(Ok);
ASSERT_EQ(GetFirst(Doc).getKind(), Type::String);
ASSERT_EQ(GetFirst(Doc).getString(), "yes");
ASSERT_EQ(ToYAML(Doc), "---\n- yes\n...\n");

Ok = Doc.fromYAML("- false\n");
ASSERT_TRUE(Ok);
ASSERT_EQ(GetFirst(Doc).getKind(), Type::Boolean);
ASSERT_EQ(GetFirst(Doc).getBool(), false);
ASSERT_EQ(ToYAML(Doc), "---\n- false\n...\n");

Ok = Doc.fromYAML("- true\n");
ASSERT_TRUE(Ok);
ASSERT_EQ(GetFirst(Doc).getKind(), Type::Boolean);
ASSERT_EQ(GetFirst(Doc).getBool(), true);
ASSERT_EQ(ToYAML(Doc), "---\n- true\n...\n");

Ok = Doc.fromYAML("- !str false\n");
ASSERT_TRUE(Ok);
ASSERT_EQ(GetFirst(Doc).getKind(), Type::String);
ASSERT_EQ(GetFirst(Doc).getString(), "false");
ASSERT_EQ(ToYAML(Doc), "---\n- !str 'false'\n...\n");

Ok = Doc.fromYAML("- !str true\n");
ASSERT_TRUE(Ok);
ASSERT_EQ(GetFirst(Doc).getKind(), Type::String);
ASSERT_EQ(GetFirst(Doc).getString(), "true");
ASSERT_EQ(ToYAML(Doc), "---\n- !str 'true'\n...\n");

// FIXME: A fix for these requires changes in YAMLParser/YAMLTraits.
Copy link
Contributor

@perlfu perlfu Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain in more detail what fix is required?
Is there a reason it is not part of this PR?

Copy link
Contributor Author

@slinder1 slinder1 Dec 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It requires a change in YAMLParser/YAMLTraits directly and I wanted to avoid the fallout. If we would rather not have a half measure like this I can spend some time trying to understand the YAML issue more directly.

Edit: to answer your first question more directly, the YAML spec(s) are complicated and I didn't want to wade into that swamp. I believe the change as-is at least works towards conforming to the JSON schema for bools, and I don't see how it can make the situation worse in this regard, but it is not a complete solution.

Ok = Doc.fromYAML("- \"false\"\n");
ASSERT_TRUE(Ok);
ASSERT_EQ(GetFirst(Doc).getKind(), Type::Boolean);
ASSERT_EQ(GetFirst(Doc).getBool(), false);
ASSERT_EQ(ToYAML(Doc), "---\n- false\n...\n");

Ok = Doc.fromYAML("- \"true\"\n");
ASSERT_TRUE(Ok);
ASSERT_EQ(GetFirst(Doc).getKind(), Type::Boolean);
ASSERT_EQ(GetFirst(Doc).getBool(), true);
ASSERT_EQ(ToYAML(Doc), "---\n- true\n...\n");
}