Fix recursion bug in ast-conversion (#1076)

* Ensure recursion works as expected

* fix test safety for when parse fails

* use a cache to keep track of resolved aliases

Author: pjsk-stripe

src/languageservice/parser/ast-converter.ts

@@ -32,14 +32,18 @@ import {
 
 type NodeRange = [number, number, number];
 
-const maxRefCount = 1000;
-let refDepth = 0;
-const seenAlias = new Set<Alias>();
+// Exported for tests
+export const aliasDepth = {
+  maxRefCount: 1000,
+  currentRefDepth: 0,
+  aliasResolutionCache: new Map<Alias, ASTNode>(),
+};
 
 export function convertAST(parent: ASTNode, node: YamlNode, doc: Document, lineCounter: LineCounter): ASTNode | undefined {
   if (!parent) {
     // first invocation
-    refDepth = 0;
+    aliasDepth.currentRefDepth = 0;
+    aliasDepth.aliasResolutionCache = new Map();
   }
 
   if (!node) {
@@ -57,11 +61,8 @@ export function convertAST(parent: ASTNode, node: YamlNode, doc: Document, lineC
   if (isScalar(node)) {
     return convertScalar(node, parent);
   }
-  if (isAlias(node) && !seenAlias.has(node) && refDepth < maxRefCount) {
-    seenAlias.add(node);
-    const converted = convertAlias(node, parent, doc, lineCounter);
-    seenAlias.delete(node);
-    return converted;
+  if (isAlias(node) && aliasDepth.currentRefDepth < aliasDepth.maxRefCount) {
+    return convertAlias(node, parent, doc, lineCounter);
   } else {
     return;
   }
@@ -154,15 +155,23 @@ function convertScalar(node: Scalar, parent: ASTNode): ASTNode {
 }
 
 function convertAlias(node: Alias, parent: ASTNode, doc: Document, lineCounter: LineCounter): ASTNode {
-  refDepth++;
+  if (aliasDepth.aliasResolutionCache.has(node)) {
+    return aliasDepth.aliasResolutionCache.get(node);
+  }
+
+  aliasDepth.currentRefDepth++;
   const resolvedNode = node.resolve(doc);
+  let ans: ASTNode;
   if (resolvedNode) {
-    return convertAST(parent, resolvedNode, doc, lineCounter);
+    ans = convertAST(parent, resolvedNode, doc, lineCounter);
   } else {
     const resultNode = new StringASTNodeImpl(parent, node, ...toOffsetLength(node.range));
     resultNode.value = node.source;
-    return resultNode;
+    ans = resultNode;
   }
+  aliasDepth.currentRefDepth--;
+  aliasDepth.aliasResolutionCache.set(node, ans);
+  return ans;
 }
 
 export function toOffsetLength(range: NodeRange): [number, number] {

test/yamlParser.test.ts

@@ -5,7 +5,8 @@
 import * as assert from 'assert';
 import { expect } from 'chai';
 import { ArrayASTNode, ObjectASTNode, PropertyASTNode } from '../src/languageservice/jsonASTTypes';
-import { parse } from './../src/languageservice/parser/yamlParser07';
+import { parse, YAMLDocument } from './../src/languageservice/parser/yamlParser07';
+import { aliasDepth } from '../src/languageservice/parser/ast-converter';
 
 describe('YAML parser', () => {
   describe('YAML parser', function () {
@@ -243,6 +244,73 @@ describe('YAML parser', () => {
       );
       assert(parsedDocument.documents[0].errors.length === 0, JSON.stringify(parsedDocument.documents[0].errors));
     });
+
+    it('parse aliases up to a depth', () => {
+      // If maxRefCount is set to 1, it will only resolve one layer of aliases, which means
+      // `b` below will inherit `a`, but `c` will not inherit `a`.
+      let parsedDocument: YAMLDocument;
+      try {
+        aliasDepth.maxRefCount = 1;
+        parsedDocument = parse(`
+a: &a
+  foo: "web"
+b: &b
+  <<: *a
+c: &c
+  <<: *b
+`);
+      } finally {
+        aliasDepth.maxRefCount = 1000;
+      }
+
+      const anode: ObjectASTNode = (parsedDocument.documents[0].root.children[0] as PropertyASTNode).valueNode as ObjectASTNode;
+      const aval = anode.properties[0].valueNode;
+
+      const bnode: ObjectASTNode = (parsedDocument.documents[0].root.children[1] as PropertyASTNode).valueNode as ObjectASTNode;
+      const bvalprops: PropertyASTNode = (bnode.properties[0].valueNode as ObjectASTNode).properties[0];
+      const bval = bvalprops.valueNode;
+
+      const cnode: ObjectASTNode = (parsedDocument.documents[0].root.children[2] as PropertyASTNode).valueNode as ObjectASTNode;
+      const cvalprops: PropertyASTNode = (cnode.properties[0].valueNode as ObjectASTNode).properties[0];
+      const cval = cvalprops.valueNode;
+
+      assert(aval?.value === 'web');
+      assert(bval?.value === 'web');
+      assert(cval?.value === undefined);
+    });
+
+    it('parse aliases up to a depth for multiple objects', () => {
+      // In the below configuration, `c` will not inherit `a` because of depth issues
+      // but the following object `o` will still resolve correctly.
+      let parsedDocument: YAMLDocument;
+      try {
+        aliasDepth.maxRefCount = 1;
+        parsedDocument = parse(`
+a: &a
+  foo: "web"
+b: &b
+  <<: *a
+c: &c
+  <<: *b
+
+o: &o
+  <<: *a
+`);
+      } finally {
+        aliasDepth.maxRefCount = 1000;
+      }
+
+      const onode: ObjectASTNode = (parsedDocument.documents[0].root.children[3] as PropertyASTNode).valueNode as ObjectASTNode;
+      const ovalprops: PropertyASTNode = (onode.properties[0].valueNode as ObjectASTNode).properties[0];
+      const oval = ovalprops.valueNode;
+
+      const cnode: ObjectASTNode = (parsedDocument.documents[0].root.children[2] as PropertyASTNode).valueNode as ObjectASTNode;
+      const cvalprops: PropertyASTNode = (cnode.properties[0].valueNode as ObjectASTNode).properties[0];
+      const cval = cvalprops.valueNode;
+
+      assert(cval?.value === undefined);
+      assert(oval?.value === 'web');
+    });
   });
 
   describe('YAML parser bugs', () => {