Partially fix some bugs handling the load of cross-edition files.

This will now work as long as all files sharing a package agree on file-level edition and options

Author: mkruskal-google

cli/targets/proto.js

@@ -102,7 +102,7 @@ function buildRoot(root) {
     if (pkg.length)
         out.push("", "package " + pkg.join(".") + ";");
 
-    buildOptions(ptr, ["edition", "syntax"]);
+    buildOptions(ptr, ["edition"]);
     ptr.nestedArray.forEach(build);
 }
 

src/index-light.js

@@ -98,7 +98,7 @@ protobuf.types            = require("./types");
 protobuf.util             = require("./util");
 
 // Set up possibly cyclic reflection dependencies
-protobuf.ReflectionObject._configure(protobuf.Root);
+protobuf.ReflectionObject._configure(protobuf.Root, protobuf.Namespace);
 protobuf.Namespace._configure(protobuf.Type, protobuf.Service, protobuf.Enum);
 protobuf.Root._configure(protobuf.Type);
 protobuf.Field._configure(protobuf.Type);

src/object.js

@@ -6,7 +6,7 @@ ReflectionObject.className = "ReflectionObject";
 const OneOf = require("./oneof");
 var util = require("./util");
 
-var Root; // cyclic
+var Root, Namespace; // cyclic
 
 /* eslint-disable no-warning-comments */
 // TODO: Replace with embedded proto.
@@ -163,7 +163,8 @@ ReflectionObject.prototype.onRemove = function onRemove(parent) {
 ReflectionObject.prototype.resolve = function resolve() {
     if (this.resolved)
         return this;
-    if (this instanceof Root || this.parent && this.parent.resolved) {
+    var edition = this.getOption("edition");
+    if ((this instanceof Namespace && edition) || (this.parent && this.parent.resolved)) {
         this._resolveFeatures();
         this.resolved = true;
     }
@@ -177,24 +178,27 @@ ReflectionObject.prototype.resolve = function resolve() {
 ReflectionObject.prototype._resolveFeatures = function _resolveFeatures() {
     var defaults = {};
 
-    var edition = this.root.getOption("edition");
-    if (this instanceof Root) {
-        if (this.root.getOption("syntax") === "proto3") {
+    var protoFeatures = Object.assign(Object.assign({}, this._protoFeatures), this._inferLegacyProtoFeatures(edition));
+
+    var edition = this.getOption("edition");
+    if (this instanceof Namespace && edition) {
+        // For a namespace marked with a specific edition, reset defaults.
+        if (edition === "proto2") {
+            defaults = Object.assign({}, proto2Defaults);
+        } else if (edition === "proto3") {
             defaults = Object.assign({}, proto3Defaults);
         } else if (edition === "2023") {
             defaults = Object.assign({}, editions2023Defaults);
         } else {
-            defaults = Object.assign({}, proto2Defaults);
+            throw new Error("Unknown edition: " + edition);
         }
+        this._features = Object.assign(defaults, protoFeatures || {});
+        return;
     }
 
-    var protoFeatures = Object.assign(Object.assign({}, this._protoFeatures), this._inferLegacyProtoFeatures(edition));
-
-    if (this instanceof Root) {
-        this._features = Object.assign(defaults, protoFeatures || {});
     // fields in Oneofs aren't actually children of them, so we have to
     // special-case it
-    } else if (this.partOf instanceof OneOf) {
+    if (this.partOf instanceof OneOf) {
         var lexicalParentFeaturesCopy = Object.assign({}, this.partOf._features);
         this._features = Object.assign(lexicalParentFeaturesCopy, protoFeatures || {});
     } else if (this.declaringField) {
@@ -241,8 +245,10 @@ ReflectionObject.prototype.getOption = function getOption(name) {
  * @returns {ReflectionObject} `this`
  */
 ReflectionObject.prototype.setOption = function setOption(name, value, ifNotSet) {
-    if (!ifNotSet || !this.options || this.options[name] === undefined)
+    if (!ifNotSet || !this.options || this.options[name] === undefined) {
+        if (this.getOption(name) !== value) this.resolved = false;
         (this.options || (this.options = {}))[name] = value;
+    }
     return this;
 };
 
@@ -318,6 +324,7 @@ ReflectionObject.prototype.toString = function toString() {
 };
 
 // Sets up cyclic dependencies (called in index-light)
-ReflectionObject._configure = function(Root_) {
+ReflectionObject._configure = function(Root_, Namespace_) {
     Root = Root_;
+    Namespace = Namespace_;
 };

src/parse.js

@@ -33,7 +33,6 @@ var base10Re    = /^[1-9][0-9]*$/,
  * @property {string|undefined} package Package name, if declared
  * @property {string[]|undefined} imports Imports, if any
  * @property {string[]|undefined} weakImports Weak imports, if any
- * @property {string|undefined} syntax Syntax, if specified (either `"proto2"` or `"proto3"`)
  * @property {Root} root Populated root instance
  */
 
@@ -81,9 +80,9 @@ function parse(source, root, options) {
         pkg,
         imports,
         weakImports,
-        syntax,
-        edition = false,
-        isProto3 = false;
+        edition = "proto2",
+        isProto3 = false,
+        isProto2 = true;
 
     var ptr = root;
 
@@ -145,7 +144,7 @@ function parse(source, root, options) {
                 try {
                     target.push([ start = parseId(next()), skip("to", true) ? parseId(next()) : start ]);
                 } catch (err) {
-                    if (typeRefRe.test(token) && edition) {
+                    if (typeRefRe.test(token) && (!isProto2 && !isProto3)) {
                         target.push(token);
                     } else {
                         throw err;
@@ -228,7 +227,6 @@ function parse(source, root, options) {
     }
 
     function parsePackage() {
-
         /* istanbul ignore if */
         if (pkg !== undefined)
             throw illegal("package");
@@ -240,6 +238,7 @@ function parse(source, root, options) {
             throw illegal(pkg, "name");
 
         ptr = ptr.define(pkg);
+        ptr.setOption("edition", edition);
         skip(";");
     }
 
@@ -265,23 +264,26 @@ function parse(source, root, options) {
 
     function parseSyntax() {
         skip("=");
-        syntax = readString();
-        isProto3 = syntax === "proto3";
+        edition = readString();
+        isProto3 = edition === "proto3";
+        isProto2 = edition === "proto2";
 
         /* istanbul ignore if */
-        if (!isProto3 && syntax !== "proto2")
-            throw illegal(syntax, "syntax");
+        if (!isProto3 && !isProto2)
+            throw illegal(edition, "syntax");
 
         // Syntax is needed to understand the meaning of the optional field rule
         // Otherwise the meaning is ambiguous between proto2 and proto3
-        root.setOption("syntax", syntax);
+        root.setOption("edition", edition);
 
         skip(";");
     }
 
     function parseEdition() {
         skip("=");
         edition = readString();
+        isProto3 = false;
+        isProto2 = false;
         const supportedEditions = ["2023"];
 
         /* istanbul ignore if */
@@ -361,7 +363,7 @@ function parse(source, root, options) {
                     break;
 
                 case "required":
-                    if (edition)
+                    if (!isProto2)
                         throw illegal(token);
                 /* eslint-disable no-fallthrough */
                 case "repeated":
@@ -372,7 +374,7 @@ function parse(source, root, options) {
                     /* istanbul ignore if */
                     if (isProto3) {
                         parseField(type, "proto3_optional");
-                    } else if (edition) {
+                    } else if (!isProto2) {
                         throw illegal(token);
                     } else {
                         parseField(type, "optional");
@@ -393,7 +395,7 @@ function parse(source, root, options) {
 
                 default:
                     /* istanbul ignore if */
-                    if (!isProto3 && !edition || !typeRefRe.test(token)) {
+                    if (isProto2 || !typeRefRe.test(token)) {
                         throw illegal(token);
                     }
 
@@ -854,7 +856,7 @@ function parse(source, root, options) {
 
                 default:
                     /* istanbul ignore if */
-                    if (!isProto3 && !edition || !typeRefRe.test(token))
+                    if (isProto2 || !typeRefRe.test(token))
                         throw illegal(token);
                     push(token);
                     parseField(parent, "optional", reference);
@@ -924,7 +926,6 @@ function parse(source, root, options) {
         "package"     : pkg,
         "imports"     : imports,
          weakImports  : weakImports,
-         syntax       : syntax,
          root         : root
     };
 }

src/root.js

@@ -35,6 +35,9 @@ function Root(options) {
      * @type {string[]}
      */
     this.files = [];
+
+    // Default to proto2 if not specified.
+    this.setOption("edition", "proto2", true);
 }
 
 /**

tests/feature_resolution_editions.js

@@ -448,12 +448,12 @@ tape.test("feature resolution editions precedence", function(test) {
 
         test.same(root.lookup("Message").lookupEnum("SomeEnumInMessage")._features,
         {
-            enum_type: 'CLOSED',
+            enum_type: 'OPEN',
             field_presence: 'EXPLICIT',
-            json_format: 'LEGACY_BEST_EFFORT',
+            json_format: 'ALLOW',
             message_encoding: 'LENGTH_PREFIXED',
-            repeated_field_encoding: 'EXPANDED',
-            utf8_validation: 'NONE',
+            repeated_field_encoding: 'PACKED',
+            utf8_validation: 'VERIFY',
             amazing_feature: 'G'
         })
         test.end();
@@ -547,3 +547,145 @@ tape.test("feature resolution inferred proto2 presence", function(test) {
 
     test.end();
 });
+
+tape.test("feature resolution mixed syntax different package", function(test) {
+    var root = protobuf.parse(`syntax = "proto2";
+    package proto2;
+    message Message {
+        optional int32 default = 1;
+        required int32 required = 2;
+        repeated int32 unpacked = 3;
+    }`).root;
+    protobuf.parse(`syntax = "proto3";
+    package proto3;
+    message Message {
+        optional int32 explicit = 1;
+        int32 implicit = 2;
+        repeated int32 packed = 3;
+    }`, root);
+    root.resolveAll();
+
+    var proto2 = root.lookup("proto2.Message");
+    test.ok(proto2.fields.default.hasPresence, "proto2 uses explicit presence");
+    test.ok(proto2.fields.required.required, "proto2 has required fields");
+    test.notOk(proto2.fields.unpacked.packed, "proto2 is expanded by default");
+
+    var proto3 = root.lookup("proto3.Message");
+    test.ok(proto3.fields.explicit.hasPresence, "proto3 optional has explicit presence");
+    test.notOk(proto3.fields.implicit.hasPresence, "proto3 is implicit presence by default");
+    test.ok(proto3.fields.packed.packed, "proto3 is packed by default");
+
+    test.end();
+});
+
+tape.test("feature resolution mixed file features different package", function(test) {
+    var root = protobuf.parse(`edition = "2023";
+    package expanded;
+    option features.repeated_field_encoding = EXPANDED;
+    message Message {
+        repeated int32 expanded = 1;
+        repeated int32 packed = 2 [features.repeated_field_encoding = PACKED];
+    }`).root;
+    protobuf.parse(`edition = "2023";
+    package packed;
+    option features.repeated_field_encoding = PACKED;
+    message Message {
+        repeated int32 packed = 1;
+        repeated int32 expanded = 2 [features.repeated_field_encoding = EXPANDED];
+    }`, root);
+    root.resolveAll();
+
+    var expanded = root.lookup("expanded.Message");
+    test.notOk(expanded.fields.expanded.packed, "expanded by default");
+    test.ok(expanded.fields.packed.packed, "packed override");
+
+    var packed = root.lookup("packed.Message");
+    test.ok(packed.fields.packed.packed, "packed by default");
+    test.notOk(packed.fields.expanded.packed, "expanded override");
+
+    test.end();
+});
+
+/*
+// These cases are bugged, because they dump file features into the same namespace!
+
+tape.test("feature resolution mixed file features same package", function(test) {
+    var root = protobuf.parse(`edition = "2023";
+    option features.repeated_field_encoding = EXPANDED;
+    message Message1 {
+        repeated int32 expanded = 1;
+        int32 explicit = 2;
+    }`).root;
+    protobuf.parse(`edition = "2023";
+    option features.field_presence = IMPLICIT;
+    message Message2 {
+        repeated int32 packed = 1;
+        int32 implicit = 3;
+    }`, root);
+    root.resolveAll();
+
+    var msg1 = root.lookup("Message1");
+    test.notOk(msg1.fields.expanded.packed, "expanded by default");
+    test.ok(msg1.fields.explicit.hasPresence, "explicit by default");
+
+    var msg2 = root.lookup("Message2");
+    test.ok(msg2.fields.packed.packed, "packed by default");
+    test.notOk(msg2.fields.implicit.hasPresence, "implicit by default");
+
+    test.end();
+});
+
+tape.test("feature resolution mixed file features same package", function(test) {
+    var root = protobuf.parse(`edition = "2023";
+    option features.repeated_field_encoding = EXPANDED;
+    message Message1 {
+        repeated int32 expanded = 1;
+        repeated int32 packed = 2 [features.repeated_field_encoding = PACKED];
+    }`).root;
+    protobuf.parse(`edition = "2023";
+    option features.repeated_field_encoding = PACKED;
+    message Message2 {
+        repeated int32 packed = 1;
+        repeated int32 expanded = 2 [features.repeated_field_encoding = EXPANDED];
+    }`, root);
+    root.resolveAll();
+
+    var expanded = root.lookup("Message1");
+    test.notOk(expanded.fields.expanded.packed, "expanded by default");
+    test.ok(expanded.fields.packed.packed, "packed override");
+
+    var packed = root.lookup("Message2");
+    test.ok(packed.fields.packed.packed, "packed by default");
+    test.notOk(packed.fields.expanded.packed, "expanded override");
+
+    test.end();
+});
+
+tape.test("feature resolution mixed syntax same package", function(test) {
+    var root = protobuf.parse(`syntax = "proto2";
+    message Message2 {
+        optional int32 default = 1;
+        required int32 required = 2;
+        repeated int32 unpacked = 3;
+    }`).root;
+    protobuf.parse(`syntax = "proto3";
+    message Message3 {
+        optional int32 explicit = 1;
+        int32 implicit = 2;
+        repeated int32 packed = 3;
+    }`, root);
+    root.resolveAll();
+
+    var proto2 = root.lookup("Message2");
+    test.ok(proto2.fields.default.hasPresence, "proto2 uses explicit presence");
+    test.ok(proto2.fields.required.required, "proto2 has required fields");
+    test.notOk(proto2.fields.unpacked.packed, "proto2 is expanded by default");
+
+    var proto3 = root.lookup("Message3");
+    test.ok(proto3.fields.explicit.hasPresence, "proto3 optional has explicit presence");
+    test.notOk(proto3.fields.implicit.hasPresence, "proto3 is implicit presence by default");
+    test.ok(proto3.fields.packed.packed, "proto3 is packed by default");
+
+    test.end();
+});
+*/