Additional review feedback

Outside of minor typos/updates, the bulk of this change is switching
how we collect used variables to be more efficient/avoid generating
useless garbage.

Author: Sylvain Lebresne

composition-js/src/validate.ts

@@ -31,7 +31,8 @@ import {
   selectionSetOf,
   typenameFieldName,
   validateSupergraph,
-  VariableDefinitions
+  VariableDefinitions,
+  isOutputType
 } from "@apollo/federation-internals";
 import {
   Edge,
@@ -210,7 +211,8 @@ function buildWitnessNextStep(edges: Edge[], index: number): SelectionSet | unde
     // ellipsis instead make it immediately clear after which part of the query there is an issue.
     const lastType = edges[edges.length -1].tail.type;
     // Note that vertex types are named type and output ones, so if it's not a leaf it is guaranteed to be selectable.
-    return isLeafType(lastType) ? undefined : new SelectionSet(lastType as CompositeType);
+    assert(isOutputType(lastType), 'Should not have input types as vertex types');
+    return isLeafType(lastType) ? undefined : new SelectionSet(lastType);
   }
 
   const edge = edges[index];
@@ -240,11 +242,15 @@ function buildWitnessNextStep(edges: Edge[], index: number): SelectionSet | unde
 }
 
 function buildWitnessField(definition: FieldDefinition<any>): Field {
+  if (definition.arguments().length === 0) {
+    return new Field(definition);
+  }
+
   const args = Object.create(null);
   for (const argDef of definition.arguments()) {
     args[argDef.name] = generateWitnessValue(argDef.type!);
   }
-  return new Field(definition, args, new VariableDefinitions());
+  return new Field(definition, args);
 }
 
 function generateWitnessValue(type: InputType): any {

internals-js/src/definitions.ts

@@ -32,7 +32,16 @@ import {
   removeAllCoreFeatures,
 } from "./coreSpec";
 import { assert, mapValues, MapWithCachedArrays, removeArrayElement } from "./utils";
-import { withDefaultValues, valueEquals, valueToString, valueToAST, variablesInValue, valueFromAST, valueNodeToConstValueNode, argumentsEquals } from "./values";
+import {
+  withDefaultValues,
+  valueEquals,
+  valueToString,
+  valueToAST,
+  valueFromAST,
+  valueNodeToConstValueNode,
+  argumentsEquals,
+  collectVariablesInValue
+} from "./values";
 import { removeInaccessibleElements } from "./inaccessibleSpec";
 import { printDirectiveDefinition, printSchema } from './print';
 import { sameType } from './types';
@@ -399,8 +408,10 @@ export class DirectiveTargetElement<T extends DirectiveTargetElement<T>> {
       : ' ' + this.appliedDirectives.join(' ');
   }
 
-  variablesInAppliedDirectives(): Variables {
-    return this.appliedDirectives.reduce((acc: Variables, d) => mergeVariables(acc, variablesInArguments(d.arguments())), []);
+  collectVariablesInAppliedDirectives(collector: VariableCollector) {
+    for (const applied of this.appliedDirectives) {
+      collector.collectInArguments(applied.arguments());
+    }
   }
 }
 
@@ -3054,7 +3065,7 @@ export class Directive<
   // applied to a field that is part of an extension, the field will have its extension set, but not the underlying directive.
   private _extension?: Extension<any>;
 
-  constructor(readonly name: string, private _args: TArgs) {
+  constructor(readonly name: string, private _args: TArgs = Object.create(null)) {
     super();
   }
 
@@ -3299,38 +3310,38 @@ export class Variable {
 
 export type Variables = readonly Variable[];
 
-export function mergeVariables(v1s: Variables, v2s: Variables): Variables {
-  if (v1s.length == 0) {
-    return v2s;
+export class VariableCollector {
+  private readonly _variables = new Map<string, Variable>();
+
+  add(variable: Variable) {
+    this._variables.set(variable.name, variable);
   }
-  if (v2s.length == 0) {
-    return v1s;
+
+  addAll(variables: Variables) {
+    for (const variable of variables) {
+      this.add(variable);
+    }
   }
-  const res: Variable[] = v1s.concat();
-  for (const v of v2s) {
-    if (!containsVariable(v1s, v)) {
-      res.push(v);
+
+  collectInArguments(args: {[key: string]: any}) {
+    for (const value of Object.values(args)) {
+      collectVariablesInValue(value, this);
     }
   }
-  return res;
-}
 
-export function containsVariable(variables: Variables, toCheck: Variable): boolean {
-  return variables.some(v => v.name == toCheck.name);
+  variables() {
+    return mapValues(this._variables);
+  }
+
+  toString(): string {
+    return this.variables().toString();
+  }
 }
 
 export function isVariable(v: any): v is Variable {
   return v instanceof Variable;
 }
 
-export function variablesInArguments(args: {[key: string]: any}): Variables {
-  let variables: Variables = [];
-  for (const value of Object.values(args)) {
-    variables = mergeVariables(variables, variablesInValue(value));
-  }
-  return variables;
-}
-
 export class VariableDefinition extends DirectiveTargetElement<VariableDefinition> {
   constructor(
     schema: Schema,