Check return types (#615)

Author: bakkot

spec/index.html

@@ -212,6 +212,7 @@ <h1>Structured Headers</h1>
             <li><b>for:</b> The type of value to which a clause of type "concrete method" or "internal method" applies.</li>
             <li><b>redefinition:</b> If "true", the name of the operation will not automatically link (i.e., it will not automatically be given an aoid).</li>
             <li><b>skip global checks:</b> If "true", disables consistency checks for this AO which require knowing every callsite.</li>
+            <li><b>skip return checks:</b> If "true", disables checking that the returned values from this AO correspond to its declared return type. Adding this to an AO which does not require it will produce a warning.</li>
           </ul>
         </li>
       </ol>

src/Algorithm.ts

@@ -68,72 +68,6 @@ export default class Algorithm extends Builder {
         namespace,
       }));
       spec._ntStringRefs = spec._ntStringRefs.concat(nonterminals);
-
-      const returnType = clause?.signature?.return;
-      let containsAnyCompletionyThings = false;
-      if (returnType?.kind != null) {
-        function checkForCompletionyStuff(list: emd.OrderedListNode) {
-          for (const step of list.contents) {
-            if (
-              step.contents[0].name === 'text' &&
-              /^(note|assert):/i.test(step.contents[0].contents)
-            ) {
-              continue;
-            }
-            if (
-              step.contents.some(
-                c => c.name === 'text' && /a new (\w+ )?Abstract Closure/i.test(c.contents),
-              )
-            ) {
-              continue;
-            }
-            for (const part of step.contents) {
-              if (part.name !== 'text') {
-                continue;
-              }
-              const completionyThing = part.contents.match(
-                /\b(ReturnIfAbrupt\b|(^|(?<=, ))[tT]hrow (a\b|the\b|$)|[rR]eturn (Normal|Throw|Return)?Completion\(|[rR]eturn( a| a new| the)? Completion Record\b|the result of evaluating\b)|(?<=[\s(])\?\s/,
-              );
-              if (completionyThing != null) {
-                if (returnType?.kind === 'completion') {
-                  containsAnyCompletionyThings = true;
-                } else {
-                  spec.warn({
-                    type: 'contents',
-                    ruleId: 'completiony-thing-in-non-completion-algorithm',
-                    message:
-                      'this would return a Completion Record, but the containing AO is declared not to return a Completion Record',
-                    node,
-                    nodeRelativeLine: part.location.start.line,
-                    nodeRelativeColumn: part.location.start.column + completionyThing.index!,
-                  });
-                }
-              }
-            }
-            if (step.sublist?.name === 'ol') {
-              checkForCompletionyStuff(step.sublist);
-            }
-          }
-        }
-        checkForCompletionyStuff(emdTree.contents);
-
-        // TODO: remove 'GeneratorYield' when the spec is more coherent (https://github.com/tc39/ecma262/pull/2429)
-        // TODO: remove SDOs after doing the work necessary to coordinate the `containsAnyCompletionyThings` bit across all the piecewise components of an SDO's definition
-        if (
-          !['Completion', 'GeneratorYield'].includes(clause.aoid!) &&
-          returnType?.kind === 'completion' &&
-          !containsAnyCompletionyThings &&
-          !['sdo', 'internal method', 'concrete method'].includes(clause.type!)
-        ) {
-          spec.warn({
-            type: 'node',
-            ruleId: 'completion-algorithm-lacks-completiony-thing',
-            message:
-              'this algorithm is declared as returning a Completion Record, but there is no step which might plausibly return an abrupt completion',
-            node,
-          });
-        }
-      }
     }
 
     const rawHtml = emd.emit(emdTree);

src/Biblio.ts

@@ -271,8 +271,10 @@ export default class Biblio {
         delete copy.location;
         // @ts-ignore
         delete copy.referencingIds;
-        // @ts-ignore
-        delete copy._node;
+        for (const key of Object.keys(copy)) {
+          // @ts-ignore
+          if (key.startsWith('_')) delete copy[key];
+        }
         return copy;
       }),
     };
@@ -344,6 +346,7 @@ export interface AlgorithmBiblioEntry extends BiblioEntryBase {
   signature: null | Signature;
   effects: string[];
   skipGlobalChecks?: boolean;
+  /** @internal*/ _skipReturnChecks?: boolean;
   /** @internal*/ _node?: Element;
 }
 
@@ -398,7 +401,7 @@ export type PartialBiblioEntry = Unkey<BiblioEntry, NonExportedKeys>;
 
 export type ExportedBiblio = {
   location: string;
-  entries: PartialBiblioEntry[];
+  entries: Unkey<PartialBiblioEntry, `_${string}`>[];
 };
 
 function dumpEnv(env: EnvRec) {

src/Clause.ts

@@ -55,6 +55,7 @@ export default class Clause extends Builder {
   /** @internal */ readonly effects: string[]; // this is held by identity and mutated by Spec.ts
   /** @internal */ signature: Signature | null;
   /** @internal */ skipGlobalChecks: boolean;
+  /** @internal */ skipReturnChecks: boolean;
 
   constructor(spec: Spec, node: HTMLElement, parent: Clause, number: string) {
     super(spec, node);
@@ -67,6 +68,7 @@ export default class Clause extends Builder {
     this.examples = [];
     this.effects = [];
     this.skipGlobalChecks = false;
+    this.skipReturnChecks = false;
 
     // namespace is either the entire spec or the parent clause's namespace.
     let parentNamespace = spec.namespace;
@@ -206,6 +208,7 @@ export default class Clause extends Builder {
       effects,
       redefinition,
       skipGlobalChecks,
+      skipReturnChecks,
     } = parseStructuredHeaderDl(this.spec, type, dl);
 
     const paras = formatPreamble(
@@ -237,6 +240,7 @@ export default class Clause extends Builder {
     }
 
     this.skipGlobalChecks = skipGlobalChecks;
+    this.skipReturnChecks = skipReturnChecks;
 
     this.effects.push(...effects);
     for (const effect of effects) {
@@ -373,6 +377,7 @@ export default class Clause extends Builder {
           signature,
           effects: clause.effects,
           _node: clause.node,
+          _skipReturnChecks: clause.skipReturnChecks,
         };
         if (clause.skipGlobalChecks) {
           op.skipGlobalChecks = true;

src/expr-parser.ts

@@ -7,7 +7,7 @@ const periodSpaceMatcher = /(?<period>\.(?= ))/u;
 const periodSpaceOrEOFMatcher = /(?<period>\.(?= |$))/u;
 
 type SimpleLocation = { start: { offset: number }; end: { offset: number } };
-type BareText = {
+export type BareText = {
   name: 'text';
   contents: string;
   location: SimpleLocation;

src/header-parser.ts

@@ -407,12 +407,14 @@ export function parseStructuredHeaderDl(
   effects: string[];
   redefinition: boolean;
   skipGlobalChecks: boolean;
+  skipReturnChecks: boolean;
 } {
   let description = null;
   let _for = null;
   let redefinition: boolean | null = null;
   let effects: string[] = [];
   let skipGlobalChecks: boolean | null = null;
+  let skipReturnChecks: boolean | null = null;
   for (let i = 0; i < dl.children.length; ++i) {
     const dt = dl.children[i];
     if (dt.tagName !== 'DT') {
@@ -482,6 +484,7 @@ export function parseStructuredHeaderDl(
         }
         break;
       }
+      // TODO figure out how to de-dupe the code for boolean attributes
       case 'redefinition': {
         if (redefinition != null) {
           spec.warn({
@@ -538,6 +541,34 @@ export function parseStructuredHeaderDl(
         }
         break;
       }
+      case 'skip return checks': {
+        if (skipReturnChecks != null) {
+          spec.warn({
+            type: 'node',
+            ruleId: 'header-format',
+            message: `duplicate "skip return checks" attribute`,
+            node: dt,
+          });
+        }
+        const contents = (dd.textContent ?? '').trim();
+        if (contents === 'true') {
+          skipReturnChecks = true;
+        } else if (contents === 'false') {
+          skipReturnChecks = false;
+        } else {
+          spec.warn({
+            type: 'contents',
+            ruleId: 'header-format',
+            message: `unknown value for "skip return checks" attribute (expected "true" or "false", got ${JSON.stringify(
+              contents,
+            )})`,
+            node: dd,
+            nodeRelativeLine: 1,
+            nodeRelativeColumn: 1,
+          });
+        }
+        break;
+      }
       case '': {
         spec.warn({
           type: 'node',
@@ -564,6 +595,7 @@ export function parseStructuredHeaderDl(
     effects,
     redefinition: redefinition ?? false,
     skipGlobalChecks: skipGlobalChecks ?? false,
+    skipReturnChecks: skipReturnChecks ?? false,
   };
 }
 

src/type-logic.ts

@@ -2,7 +2,7 @@ import type Biblio from './Biblio';
 import type { Type as BiblioType } from './Biblio';
 import type { Expr, NonSeq } from './expr-parser';
 
-type Type =
+export type Type =
   | { kind: 'unknown' } // top
   | { kind: 'never' } // bottom
   | { kind: 'union'; of: NonUnion[] } // constraint: nothing in the union dominates anything else in the union
@@ -80,6 +80,7 @@ const dominateGraph: Partial<Record<Type['kind'], Type['kind'][]>> = {
   bigint: ['concrete bigint'],
   boolean: ['concrete boolean'],
 };
+
 /*
 The type lattice used here is very simple (aside from explicit unions).
 As such we mostly only need to define the `dominates` relationship and apply trivial rules:
@@ -140,6 +141,7 @@ export function dominates(a: Type, b: Type): boolean {
   }
   return false;
 }
+
 function addToUnion(types: NonUnion[], type: NonUnion): Type {
   if (type.kind === 'normal completion') {
     const existingNormalCompletionIndex = types.findIndex(t => t.kind === 'normal completion');
@@ -583,8 +585,7 @@ export function typeFromExprType(type: BiblioType): Type {
       break;
     }
     case 'unused': {
-      // this is really only a return type, but might as well handle it
-      return { kind: 'enum value', value: '~unused~' };
+      return { kind: 'enum value', value: 'unused' };
     }
   }
   return { kind: 'unknown' };
@@ -600,15 +601,24 @@ export function isCompletion(
   );
 }
 
+export function isPossiblyAbruptCompletion(
+  type: Type,
+): type is Type & { kind: 'abrupt completion' | 'union' } {
+  return (
+    type.kind === 'abrupt completion' ||
+    (type.kind === 'union' && type.of.some(isPossiblyAbruptCompletion))
+  );
+}
+
 export function stripWhitespace(items: NonSeq[]) {
   items = [...items];
-  while (items[0]?.name === 'text' && /^\s+$/.test(items[0].contents)) {
+  while (items[0]?.name === 'text' && /^\s*$/.test(items[0].contents)) {
     items.shift();
   }
   while (
     items[items.length - 1]?.name === 'text' &&
     // @ts-expect-error
-    /^\s+$/.test(items[items.length - 1].contents)
+    /^\s*$/.test(items[items.length - 1].contents)
   ) {
     items.pop();
   }