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,12 @@ function parse(source, root, options) {
             throw illegal(pkg, "name");
 
         ptr = ptr.define(pkg);
+
+        var oldEdition = ptr.getOption("edition");
+        if (oldEdition && oldEdition !== edition) {
+            throw new Error("incompatible editions detected in package " + pkg + ": " + edition + " vs " + oldEdition);
+        }
+        ptr.setOption("edition", edition);
         skip(";");
     }
 
@@ -265,23 +269,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 +368,7 @@ function parse(source, root, options) {
                     break;
 
                 case "required":
-                    if (edition)
+                    if (!isProto2)
                         throw illegal(token);
                 /* eslint-disable no-fallthrough */
                 case "repeated":
@@ -372,7 +379,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 +400,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 +861,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 +931,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);
 }
 
 /**