From 79db94de1d8417ae9e1251eee2e39b92cb8c92a0 Mon Sep 17 00:00:00 2001
From: Kevin Gibbons <bakkot@gmail.com>
Date: Thu, 9 Dec 2021 19:37:20 -0800
Subject: [PATCH] better errors for issues with clauses (#387)

---
 src/Clause.ts                             |  2 +-
 src/Spec.ts                               |  2 +-
 src/clauseNums.ts                         | 53 +++++++++++++++++------
 test/baselines/sources/malformed.bad.html |  9 +---
 test/clauseIds.js                         | 43 +++++-------------
 test/errors.js                            | 36 +++++++++++++++
 6 files changed, 92 insertions(+), 53 deletions(-)

diff --git a/src/Clause.ts b/src/Clause.ts
index cf2e302e..b6ae5b2c 100644
--- a/src/Clause.ts
+++ b/src/Clause.ts
@@ -222,7 +222,7 @@ export default class Clause extends Builder {
 
     let nextNumber = '';
     if (node.nodeName !== 'EMU-INTRO') {
-      nextNumber = clauseNumberer.next(clauseStack.length, node.nodeName === 'EMU-ANNEX').value;
+      nextNumber = clauseNumberer.next(clauseStack, node);
     }
     const parent = clauseStack[clauseStack.length - 1] || null;
 
diff --git a/src/Spec.ts b/src/Spec.ts
index c96abe26..087b9ec9 100644
--- a/src/Spec.ts
+++ b/src/Spec.ts
@@ -446,7 +446,7 @@ export default class Spec {
       importStack: [],
       clauseStack: [],
       tagStack: [],
-      clauseNumberer: ClauseNumbers(),
+      clauseNumberer: ClauseNumbers(this),
       inNoAutolink: false,
       inAlg: false,
       inNoEmd: false,
diff --git a/src/clauseNums.ts b/src/clauseNums.ts
index 684efed4..dcec5c41 100644
--- a/src/clauseNums.ts
+++ b/src/clauseNums.ts
@@ -1,39 +1,66 @@
+import type Clause from './Clause';
+import type { Spec } from './ecmarkup';
+
 /*@internal*/
 export interface ClauseNumberIterator {
-  next(level: number, annex: boolean): IteratorResult<string>;
+  next(clauseStack: Clause[], node: HTMLElement): string;
 }
 
 /*@internal*/
-export default function iterator(): ClauseNumberIterator {
+export default function iterator(spec: Spec): ClauseNumberIterator {
   const ids: (string | number)[] = [];
   let inAnnex = false;
   let currentLevel = 0;
 
   return {
-    next(level: number, annex: boolean) {
-      if (inAnnex && !annex) throw new Error('Clauses cannot follow annexes');
-      if (level - currentLevel > 1) throw new Error('Skipped clause');
+    next(clauseStack: Clause[], node: HTMLElement) {
+      const annex = node.nodeName === 'EMU-ANNEX';
+      const level = clauseStack.length;
+      if (inAnnex && !annex) {
+        spec.warn({
+          type: 'node',
+          node,
+          ruleId: 'clause-after-annex',
+          message: 'clauses cannot follow annexes',
+        });
+      }
+      if (level - currentLevel > 1) {
+        spec.warn({
+          type: 'node',
+          node,
+          ruleId: 'skipped-caluse',
+          message: 'clause is being numbered without numbering its parent clause',
+        });
+      }
 
       const nextNum = annex ? nextAnnexNum : nextClauseNum;
 
       if (level === currentLevel) {
-        ids[currentLevel] = nextNum(level);
+        ids[currentLevel] = nextNum(clauseStack, node);
       } else if (level > currentLevel) {
-        ids.push(nextNum(level));
+        ids.push(nextNum(clauseStack, node));
       } else {
         ids.length = level + 1;
-        ids[level] = nextNum(level);
+        ids[level] = nextNum(clauseStack, node);
       }
 
       currentLevel = level;
 
-      return { value: ids.join('.'), done: false };
+      return ids.join('.');
     },
   };
 
-  function nextAnnexNum(level: number): string | number {
+  function nextAnnexNum(clauseStack: Clause[], node: HTMLElement): string | number {
+    const level = clauseStack.length;
     if (!inAnnex) {
-      if (level > 0) throw new Error('First annex must be at depth 0');
+      if (level > 0) {
+        spec.warn({
+          type: 'node',
+          node,
+          ruleId: 'annex-depth',
+          message: 'first annex must be at depth 0',
+        });
+      }
       inAnnex = true;
 
       return 'A';
@@ -43,10 +70,10 @@ export default function iterator(): ClauseNumberIterator {
       return String.fromCharCode((<string>ids[0]).charCodeAt(0) + 1);
     }
 
-    return nextClauseNum(level);
+    return nextClauseNum(clauseStack);
   }
 
-  function nextClauseNum(level: number) {
+  function nextClauseNum({ length: level }: { length: number }) {
     if (ids[level] === undefined) return 1;
     return <number>ids[level] + 1;
   }
diff --git a/test/baselines/sources/malformed.bad.html b/test/baselines/sources/malformed.bad.html
index 7715f2cc..620e8160 100644
--- a/test/baselines/sources/malformed.bad.html
+++ b/test/baselines/sources/malformed.bad.html
@@ -1,7 +1,2 @@
-<emu-annex id="sec-annex">
-  <h1>Annex</h1>
-</emu-annex>
-<emu-clause id="sec-clause">
-  <h1>Clause</h1>
-  <p>A clause cannot follow an annex and therefore this is malformed.</p>
-</emu-clause>
+<!-- emu-import requires href -->
+<emu-import></emu-import>
diff --git a/test/clauseIds.js b/test/clauseIds.js
index 132a4872..2240667a 100644
--- a/test/clauseIds.js
+++ b/test/clauseIds.js
@@ -11,36 +11,17 @@ describe('clause id generation', () => {
   });
 
   specify('generating clause ids', () => {
-    assert.strictEqual(iter.next(0).value, '1');
-    assert.strictEqual(iter.next(1).value, '1.1');
-    assert.strictEqual(iter.next(1).value, '1.2');
-    assert.strictEqual(iter.next(2).value, '1.2.1');
-    assert.strictEqual(iter.next(0).value, '2');
-    assert.strictEqual(iter.next(0, true).value, 'A');
-    assert.strictEqual(iter.next(1, true).value, 'A.1');
-    assert.strictEqual(iter.next(1, true).value, 'A.2');
-    assert.strictEqual(iter.next(2, true).value, 'A.2.1');
-    assert.strictEqual(iter.next(0, true).value, 'B');
-  });
-
-  specify('error thrown for skipping clauses', () => {
-    assert.throws(() => {
-      iter.next(2);
-    }, /Skipped clause/);
-  });
-
-  specify('error thrown for non-annex following annex', () => {
-    assert.throws(() => {
-      iter.next(0);
-      iter.next(0, true);
-      iter.next(0);
-    }, /Clauses cannot follow annexes/);
-  });
-
-  specify('error thrown for annex not starting at depth 0', () => {
-    assert.throws(() => {
-      iter.next(0);
-      iter.next(1, true);
-    }, /First annex must be at depth 0/);
+    const CLAUSE = { nodeName: 'EMU-CLAUSE' };
+    const ANNEX = { nodeName: 'EMU-ANNEX' };
+    assert.strictEqual(iter.next([], CLAUSE), '1');
+    assert.strictEqual(iter.next([{}], CLAUSE), '1.1');
+    assert.strictEqual(iter.next([{}], CLAUSE), '1.2');
+    assert.strictEqual(iter.next([{}, {}], CLAUSE), '1.2.1');
+    assert.strictEqual(iter.next([], CLAUSE), '2');
+    assert.strictEqual(iter.next([], ANNEX), 'A');
+    assert.strictEqual(iter.next([{}], ANNEX), 'A.1');
+    assert.strictEqual(iter.next([{}], ANNEX), 'A.2');
+    assert.strictEqual(iter.next([{}, {}], ANNEX), 'A.2.1');
+    assert.strictEqual(iter.next([], ANNEX), 'B');
   });
 });
diff --git a/test/errors.js b/test/errors.js
index ed6f5683..1264d784 100644
--- a/test/errors.js
+++ b/test/errors.js
@@ -812,4 +812,40 @@ ${M}      </pre>
       { lintSpec: true }
     );
   });
+
+  it('clause after annex', async () => {
+    await assertError(
+      positioned`
+       <emu-annex id="sec-a">
+        <h1>Annex</h1>
+        </emu-annex>
+        ${M}<emu-clause id="sec-c">
+          <h1>Clause</h1>
+        </emu-clause>
+      `,
+      {
+        ruleId: 'clause-after-annex',
+        nodeType: 'emu-clause',
+        message: 'clauses cannot follow annexes',
+      }
+    );
+  });
+
+  it('nested annex', async () => {
+    await assertError(
+      positioned`
+        <emu-clause id="sec-c">
+          <h1>Clause</h1>
+          ${M}<emu-annex id="sec-a">
+            <h1>Annex</h1>
+          </emu-annex>
+        </emu-clause>
+      `,
+      {
+        ruleId: 'annex-depth',
+        nodeType: 'emu-annex',
+        message: 'first annex must be at depth 0',
+      }
+    );
+  });
 });