Reject (pxf.required) with (pxf.default) at bind time

CHANGELOG.md

@@ -13,6 +13,16 @@ format changes.
 
 ### Fixed
 
+- **`(pxf.required)` with `(pxf.default)` on one field is a bind-time
+  violation** (#50), as draft -01 now requires (protowire#453).
+  `ValidateFile` reports it as `ViolationKind::kRequiredOption`, over the
+  import closure, and `Unmarshal` and `UnmarshalFull` refuse such a schema.
+  Such a schema used to bind here, and which option won depended on the
+  engine. `(pxf.required) = false` beside a default stays accepted. The
+  bind-failure `Status` now starts `PXF schema violations:`, as in the Go
+  reference, instead of `PXF schema reserved-name violations:`, because
+  it no longer lists only reserved names.
+
 - **`check_decode`'s `Tree` has `branches` and `tags`** (protowire#449).
   Without them the PB leg skipped both fields as unknown, so the corpus's
   map-depth rows could not reach the decoder's map path. The decoders

include/protowire/pxf/schema.h

@@ -35,6 +35,11 @@ enum class ViolationKind : uint8_t {
   // field that is not a repeated message-typed field, naming a field the
   // element message lacks, or naming one that is not a singular string.
   kKeyOption,
+  // (pxf.required) = true beside (pxf.default) on one field, which draft
+  // -01 §annotation-extensions ("Required and Default") forbids: the first
+  // rejects a document where the field is absent, and the second treats it
+  // as supplied.
+  kRequiredOption,
 };
 
 const char* ViolationKindName(ViolationKind k);
@@ -46,7 +51,7 @@ struct Violation {
   std::string element;  // fully-qualified protobuf name (e.g. "trades.v1.Side.null")
   std::string name;     // bare reserved identifier, or the (pxf.key) value for kKeyOption
   ViolationKind kind = ViolationKind::kField;
-  std::string detail;  // human-readable explanation; set for kKeyOption
+  std::string detail;  // human-readable explanation; set for kKeyOption and kRequiredOption
 
   // One-line human-readable description, e.g.
   //   "trades.proto: message field \"trades.v1.X.null\" uses PXF-reserved name \"null\" (draft
@@ -57,10 +62,12 @@ struct Violation {
 // ValidateDescriptor walks the file containing `desc` together with its
 // transitive imports, and returns every bind-time violation in that
 // closure: reserved-name collisions among messages, oneofs, and enum
-// values, and invalid (pxf.key) placements (draft -01 §3.13). The
-// returned vector is sorted by declaring file path and then by element
-// fully-qualified name, for stable output. An empty vector means the
-// schema is conformant.
+// values; invalid (pxf.key) placements (draft -01 §3.13); and
+// (pxf.required) with (pxf.default) on one field (draft -01
+// §annotation-extensions, "Required and Default"). (pxf.required) = false
+// demands nothing, so a default beside it stands. The returned vector is
+// sorted by declaring file path and then by element fully-qualified name,
+// for stable output. An empty vector means the schema is conformant.
 //
 // Scope is the import closure, per draft -01 § Scope of Bind-Time
 // Checks: a misplaced annotation on a message type declared in an

src/pxf/decode_fast.cc

@@ -1776,15 +1776,15 @@ Status PostDecode(Message* msg,
 
 namespace {
 
-// CheckSchema runs the PXF schema reserved-name check (draft §3.13) on
-// the descriptor of `msg`. Returns a Status with the joined violation
+// CheckSchema runs the PXF schema's bind-time checks on the descriptor
+// of `msg`. Returns a Status with the joined violation
 // list on conflict, OK otherwise. Skipped when opts.skip_validate is
 // true.
 Status CheckSchema(const Message* msg, const UnmarshalOptions& opts) {
   if (opts.skip_validate) return Status::OK();
   auto vs = ValidateDescriptor(msg->GetDescriptor());
   if (vs.empty()) return Status::OK();
-  std::string msg_str = "PXF schema reserved-name violations:\n  ";
+  std::string msg_str = "PXF schema violations:\n  ";
   for (size_t i = 0; i < vs.size(); ++i) {
     if (i != 0) msg_str.append("\n  ");
     msg_str.append(vs[i].ToString());

src/pxf/schema.cc

@@ -90,6 +90,15 @@ void WalkMessages(const std::string& path, const pb::Descriptor* md, std::vector
     if (auto key = KeyFieldName(f); key.has_value()) {
       CheckKeyOption(path, f, *key, out);
     }
+    if (IsRequired(f) && GetDefault(f).has_value()) {
+      out->push_back(Violation{path,
+                               std::string(f->full_name()),
+                               "",
+                               ViolationKind::kRequiredOption,
+                               "(pxf.required) and (pxf.default) are exclusive on one field: the "
+                               "first rejects a document where the field is absent, and the second "
+                               "treats it as supplied"});
+    }
   }
   // Skip synthetic oneofs (those generated for proto3 optional fields).
   // libprotobuf exposes a real oneof count and a "real" count; the
@@ -123,6 +132,8 @@ const char* ViolationKindName(ViolationKind k) {
       return "enum value";
     case ViolationKind::kKeyOption:
       return "(pxf.key) placement";
+    case ViolationKind::kRequiredOption:
+      return "(pxf.required) placement";
   }
   return "unknown";
 }
@@ -132,6 +143,10 @@ std::string Violation::ToString() const {
     return file + ": field \"" + element + "\" carries (pxf.key) = \"" + name + "\": " + detail +
            " (draft -01 §3.13)";
   }
+  if (kind == ViolationKind::kRequiredOption) {
+    return file + ": field \"" + element + "\": invalid (pxf.required): " + detail +
+           " (draft -01 §annotation-extensions)";
+  }
   return file + ": " + ViolationKindName(kind) + " \"" + element + "\" uses PXF-reserved name \"" +
          name + "\" (draft §3.13)";
 }

test/pxf_schema_test.cc

@@ -278,6 +278,7 @@ TEST_F(PxfSchema, ViolationKindNameRendersExpectedLabels) {
   EXPECT_STREQ(ViolationKindName(ViolationKind::kField), "message field");
   EXPECT_STREQ(ViolationKindName(ViolationKind::kOneof), "oneof");
   EXPECT_STREQ(ViolationKindName(ViolationKind::kEnumValue), "enum value");
+  EXPECT_STREQ(ViolationKindName(ViolationKind::kRequiredOption), "(pxf.required) placement");
 }
 
 // ---- Unmarshal-time integration (the decode gate) ------------------------
@@ -298,8 +299,7 @@ message Row {
 
   auto st = protowire::pxf::Unmarshal("a = 1\n", msg.get());
   ASSERT_FALSE(st.ok());
-  EXPECT_NE(std::string(st.message()).find("PXF schema reserved-name violations"),
-            std::string::npos);
+  EXPECT_NE(std::string(st.message()).find("PXF schema violations"), std::string::npos);
 }
 
 TEST_F(PxfSchema, UnmarshalSkipsValidateWhenOptionSet) {
@@ -342,8 +342,7 @@ message Row {
   std::unique_ptr<pb::Message> msg(factory.GetPrototype(fd->message_type(0))->New());
   auto r = protowire::pxf::UnmarshalFull("a = 1\n", msg.get());
   ASSERT_FALSE(r.ok());
-  EXPECT_NE(std::string(r.status().message()).find("PXF schema reserved-name violations"),
-            std::string::npos);
+  EXPECT_NE(std::string(r.status().message()).find("PXF schema violations"), std::string::npos);
 }
 
 // ---- import closure (draft -01 § Scope of Bind-Time Checks; #34) ---------
@@ -390,6 +389,99 @@ message Root { Imported inner = 1; }
   EXPECT_NE(st.message().find("b_closure.proto"), std::string::npos) << st.ToString();
 }
 
+// ---- (pxf.required) with (pxf.default) (draft -01 "Required and Default") -
+
+// The pair is refused when the schema is bound, whatever the document
+// says, and the Status names the field. The document here supplies the
+// field, so a check deferred to an absent field would miss it.
+TEST_F(PxfSchema, RequiredWithDefaultRejected) {
+  const pb::FileDescriptor* fd = CompileFiles({{"req_def.proto", R"src(
+syntax = "proto3";
+package reqdef.v1;
+import "pxf/annotations.proto";
+message Row {
+  string f = 1 [(pxf.required) = true, (pxf.default) = "x"];
+  message Inner {
+    string empty = 1 [(pxf.required) = true, (pxf.default) = ""];
+  }
+}
+)src"}},
+                                              "req_def.proto");
+  ASSERT_NE(fd, nullptr) << errors_.last_;
+  auto vs = ValidateFile(fd);
+  ASSERT_EQ(vs.size(), 2u);
+  EXPECT_EQ(vs[0].element, "reqdef.v1.Row.Inner.empty");
+  EXPECT_EQ(vs[0].kind, ViolationKind::kRequiredOption);
+  EXPECT_EQ(vs[1].element, "reqdef.v1.Row.f");
+  EXPECT_EQ(vs[1].kind, ViolationKind::kRequiredOption);
+  EXPECT_NE(vs[1].ToString().find("(pxf.required) and (pxf.default) are exclusive on one field"),
+            std::string::npos)
+      << vs[1].ToString();
+
+  pb::DynamicMessageFactory factory(importer_->pool());
+  const pb::Descriptor* row = fd->message_type(0);
+  std::unique_ptr<pb::Message> msg(factory.GetPrototype(row)->New());
+  auto st = protowire::pxf::Unmarshal("f = \"given\"\n", msg.get());
+  ASSERT_FALSE(st.ok());
+  EXPECT_NE(st.message().find("reqdef.v1.Row.f"), std::string::npos) << st.ToString();
+  std::unique_ptr<pb::Message> full(factory.GetPrototype(row)->New());
+  auto r = protowire::pxf::UnmarshalFull("f = \"given\"\n", full.get());
+  ASSERT_FALSE(r.ok());
+  EXPECT_NE(r.status().message().find("are exclusive on one field"), std::string::npos)
+      << r.status().ToString();
+}
+
+// (pxf.required) = false requires nothing, so it does not conflict with a
+// default, and the default still applies.
+TEST_F(PxfSchema, RequiredFalseBesideDefaultAccepted) {
+  const pb::FileDescriptor* fd = CompileFiles({{"req_false.proto", R"src(
+syntax = "proto3";
+package reqfalse.v1;
+import "pxf/annotations.proto";
+message Row {
+  string f = 1 [(pxf.required) = false, (pxf.default) = "x"];
+}
+)src"}},
+                                              "req_false.proto");
+  ASSERT_NE(fd, nullptr) << errors_.last_;
+  EXPECT_TRUE(ValidateFile(fd).empty());
+
+  pb::DynamicMessageFactory factory(importer_->pool());
+  const pb::Descriptor* row = fd->message_type(0);
+  std::unique_ptr<pb::Message> msg(factory.GetPrototype(row)->New());
+  auto r = protowire::pxf::UnmarshalFull("", msg.get());
+  ASSERT_TRUE(r.ok()) << r.status().ToString();
+  EXPECT_EQ(msg->GetReflection()->GetString(*msg, row->FindFieldByName("f")), "x");
+}
+
+// The pair on a type only an import declares is refused too, even though
+// the bound message never refers to it (draft -01 § Scope of Bind-Time
+// Checks), and is attributed to the importing file's dependency.
+TEST_F(PxfSchema, RequiredWithDefaultInImportReported) {
+  const char* b = R"src(
+syntax = "proto3";
+package reqimp.v1;
+import "pxf/annotations.proto";
+message Unreferenced {
+  string f = 1 [(pxf.required) = true, (pxf.default) = "x"];
+}
+)src";
+  const char* a = R"src(
+syntax = "proto3";
+package reqimp.v1;
+import "b_req.proto";
+message Root { string s = 1; }
+)src";
+  const pb::FileDescriptor* fd =
+      CompileFiles({{"b_req.proto", b}, {"a_req.proto", a}}, "a_req.proto");
+  ASSERT_NE(fd, nullptr) << errors_.last_;
+  auto vs = ValidateDescriptor(fd->message_type(0));
+  ASSERT_EQ(vs.size(), 1u);
+  EXPECT_EQ(vs[0].file, "b_req.proto");
+  EXPECT_EQ(vs[0].element, "reqimp.v1.Unreferenced.f");
+  EXPECT_EQ(vs[0].kind, ViolationKind::kRequiredOption);
+}
+
 // The diamond — A imports B and C, both importing D — reports D's
 // violation once, and the closure's violations sort by file, then element.
 TEST_F(PxfSchema, DiamondImportReportedOnceAndSortedByFile) {