fix: change tree traversal order and feature resolution algorithm

sofisl · sofisl · commit d2d47d9e4fbe · 2024-10-04T15:08:15.000-07:00
diff --git a/src/enum.js b/src/enum.js
@@ -57,11 +57,17 @@ function Enum(name, values, options, comment, comments, valuesOptions) {
     this.valuesOptions = valuesOptions;
 
     /**
-     * Values features, if any
+     * Resolved values features, if any
      * @type {Object<string, Object<string, *>>|undefined}
      */
     this._valuesFeatures = {};
 
+    /**
+     * Unresolved values features, if any
+     * @type {Object<string, Object<string, *>>|undefined}
+     */
+    this._proto_valuesFeatures = {};
+
     /**
      * Reserved ranges, if any.
      * @type {Array.<number[]|string>}
@@ -78,6 +84,28 @@ function Enum(name, values, options, comment, comments, valuesOptions) {
                 this.valuesById[ this.values[keys[i]] = values[keys[i]] ] = keys[i];
 }
 
+/**
+ * Resolves value features
+ * @returns {Enum} `this`
+ */
+Enum.prototype.resolve = function resolve() {
+
+    if (this.resolved)
+        return this;
+
+    for (var key of Object.keys(this._proto_valuesFeatures)) {
+
+        if (this.parent) {
+            var parentFeaturesCopy = Object.assign({}, this.parent._features);
+            this._valuesFeatures[key] = Object.assign(parentFeaturesCopy, this._proto_valuesFeatures[key] || {});
+        } else {
+            this._valuesFeatures[key] = Object.assign({}, this._proto_valuesFeatures[key]);
+        }
+    }
+    return ReflectionObject.prototype.resolve.call(this);
+};
+
+
 /**
  * Enum descriptor.
  * @interface IEnum
@@ -158,14 +186,19 @@ Enum.prototype.add = function add(name, id, comment, options) {
         for (var key of Object.keys(this.valuesOptions)) {
             var features = Array.isArray(this.valuesOptions[key]) ? this.valuesOptions[key].find(x => {return Object.prototype.hasOwnProperty.call(x, "features");}) : this.valuesOptions[key] === "features";
             if (features) {
-                if (!this._valuesFeatures) {
-                    this._valuesFeatures = {};
-                }
-                this._valuesFeatures[key] = features.features || {};
+                this._proto_valuesFeatures[key] = features.features;
+            } else {
+                this._proto_valuesFeatures[key] = {};
             }
         }
     }
 
+    for (var key of Object.keys(this.values)) {
+        if (!this._proto_valuesFeatures[key]) {
+            this._proto_valuesFeatures[key] = {}
+        }
+    }
+
     this.comments[name] = comment || null;
     return this;
 };
diff --git a/src/namespace.js b/src/namespace.js
@@ -302,12 +302,13 @@ Namespace.prototype.define = function define(path, json) {
  */
 Namespace.prototype.resolveAll = function resolveAll() {
     var nested = this.nestedArray, i = 0;
+    this.resolve();
     while (i < nested.length)
         if (nested[i] instanceof Namespace)
             nested[i++].resolveAll();
         else
             nested[i++].resolve();
-    return this.resolve();
+    return this;
 };
 
 /**
diff --git a/src/object.js b/src/object.js
@@ -7,6 +7,7 @@ var util = require("./util");
 
 var Root; // cyclic
 
+// TODO: Replace with embedded proto.
 var editions2023Defaults = {enum_type: "OPEN", field_presence: "EXPLICIT", json_format: "ALLOW", message_encoding: "LENGTH_PREFIXED", repeated_field_encoding: "PACKED", utf8_validation: "VERIFY"};
 var proto2Defaults = {enum_type: "CLOSED", field_presence: "EXPLICIT", json_format: "LEGACY_BEST_EFFORT", message_encoding: "LENGTH_PREFIXED", repeated_field_encoding: "EXPANDED", utf8_validation: "NONE"};
 var proto3Defaults = {enum_type: "OPEN", field_presence: "IMPLICIT", json_format: "ALLOW", message_encoding: "LENGTH_PREFIXED", repeated_field_encoding: "PACKED", utf8_validation: "VERIFY"};
@@ -51,7 +52,7 @@ function ReflectionObject(name, options) {
     this._features = {};
 
     /**
-     * Resolved Features.
+     * Unresolved Features.
      */
     this._proto_features = null;
 
@@ -160,10 +161,10 @@ ReflectionObject.prototype.onRemove = function onRemove(parent) {
 ReflectionObject.prototype.resolve = function resolve() {
     if (this.resolved)
         return this;
-    if (this.root instanceof Root || this.parent) {
+    if (this instanceof Root || this.parent && this.parent.resolved)
         this._resolveFeatures();
+    if (this.root instanceof Root)
         this.resolved = true;
-    }
     return this;
 };
 
@@ -174,28 +175,24 @@ ReflectionObject.prototype.resolve = function resolve() {
 ReflectionObject.prototype._resolveFeatures = function _resolveFeatures() {
     var defaults = {};
 
-    if (this.root.getOption("syntax") === "proto2") {
-        defaults = Object.assign({}, proto2Defaults);
-    } else if (this.root.getOption("syntax") === "proto3") {
-        defaults = Object.assign({}, proto3Defaults);
-    } else if (this.root.getOption("edition") === "2023") {
-        defaults = Object.assign({}, editions2023Defaults);
+    if (this instanceof Root) {
+        if (this.root.getOption("syntax") === "proto2") {
+            defaults = Object.assign({}, proto2Defaults);
+        } else if (this.root.getOption("syntax") === "proto3") {
+            defaults = Object.assign({}, proto3Defaults);
+        } else if (this.root.getOption("edition") === "2023") {
+            defaults = Object.assign({}, editions2023Defaults);
+        }
     }
 
-    if (this.parent) {
-        // This is an annoying workaround since we can't use the spread operator
-        // (Breaks the bundler and eslint)
-        // If we don't create a shallow copy, we end up also altering the parent's
-        // features
-        var parentFeaturesMerged = Object.assign(defaults, this.parent._proto_features);
-        this._features = Object.assign(parentFeaturesMerged, this._proto_features || {});
-        this._proto_features = this._features;
-        this.parent._resolveFeatures();
-    } else {
+    if (this instanceof Root) {
         this._features = Object.assign(defaults, this._proto_features || {});
+    } else if (this.parent) {
+        var parentFeaturesCopy = Object.assign({}, this.parent._features);
+        this._features = Object.assign(parentFeaturesCopy, this._proto_features || {});
+    } else {
+        this._features = Object.assign({}, this._proto_features);
     }
-    this._proto_features = this._features;
-
 };
 
 /**
diff --git a/src/parse.js b/src/parse.js
@@ -147,6 +147,8 @@ function parse(source, root, options) {
                 } catch (err) {
                     if (typeRefRe.test(token) && edition) {
                         target.push(token);
+                    } else {
+                        throw err;
                     }
                 }
             }
diff --git a/src/service.js b/src/service.js
@@ -106,10 +106,11 @@ Service.prototype.get = function get(name) {
  * @override
  */
 Service.prototype.resolveAll = function resolveAll() {
+    Namespace.prototype.resolve.call(this);
     var methods = this.methodsArray;
     for (var i = 0; i < methods.length; ++i)
         methods[i].resolve();
-    return Namespace.prototype.resolve.call(this);
+    return this;
 };
 
 /**
diff --git a/src/type.js b/src/type.js
@@ -299,13 +299,14 @@ Type.prototype.toJSON = function toJSON(toJSONOptions) {
  * @override
  */
 Type.prototype.resolveAll = function resolveAll() {
+    Namespace.prototype.resolveAll.call(this);
     var fields = this.fieldsArray, i = 0;
     while (i < fields.length)
         fields[i++].resolve();
     var oneofs = this.oneofsArray; i = 0;
     while (i < oneofs.length)
         oneofs[i++].resolve();
-    return Namespace.prototype.resolveAll.call(this);
+    return this;
 };
 
 /**
diff --git a/tests/feature_resolution_editions.js b/tests/feature_resolution_editions.js

Original file line number	Diff line number	Diff line change
`@@ -147,6 +147,8 @@ function parse(source, root, options) {`
`147`	`147`	`} catch (err) {`
`148`	`148`	`if (typeRefRe.test(token) && edition) {`
`149`	`149`	`target.push(token);`
	`150`	`+ } else {`
	`151`	`+ throw err;`
`150`	`152`	`}`
`151`	`153`	`}`
`152`	`154`	`}`