fix no-duplicate-fields, compare selections with fragment spreads and inline fragments

.changeset/clean-hairs-count.md

@@ -0,0 +1,5 @@
+---
+'@graphql-eslint/eslint-plugin': patch
+---
+
+fix `no-duplicate-fields`, compare selections with fragment spreads and inline fragments

packages/plugin/src/estree-parser/converter.ts

@@ -7,11 +7,10 @@ import {
   Kind,
   DocumentNode,
   ASTVisitor,
-  Location,
 } from 'graphql';
-import { SourceLocation, Comment } from 'estree';
-import { extractCommentsFromAst } from './utils';
-import { GraphQLESTreeNode, TypeInformation } from './estree-ast';
+import type { Comment } from 'estree';
+import { extractCommentsFromAst, convertLocation } from './utils';
+import type { GraphQLESTreeNode, TypeInformation } from './estree-ast';
 
 export function convertToESTree<T extends ASTNode>(node: T, typeInfo?: TypeInfo) {
   const visitor: ASTVisitor = { leave: convertNode(typeInfo) };
@@ -25,32 +24,6 @@ function hasTypeField<T extends ASTNode>(node: T): node is T & { readonly type:
   return 'type' in node && Boolean(node.type);
 }
 
-function convertLocation(location: Location): SourceLocation {
-  const { startToken, endToken, source, start, end } = location;
-  /*
-   * ESLint has 0-based column number
-   * https://eslint.org/docs/developer-guide/working-with-rules#contextreport
-   */
-  const loc = {
-    start: {
-      /*
-       * Kind.Document has startToken: { line: 0, column: 0 }, we set line as 1 and column as 0
-       */
-      line: startToken.line === 0 ? 1 : startToken.line,
-      column: startToken.column === 0 ? 0 : startToken.column - 1,
-    },
-    end: {
-      line: endToken.line,
-      column: endToken.column - 1,
-    },
-    source: source.body,
-  };
-  if (loc.start.column === loc.end.column) {
-    loc.end.column += end - start;
-  }
-  return loc;
-}
-
 const convertNode =
   (typeInfo?: TypeInfo) =>
   <T extends ASTNode>(node: T, key: string | number, parent: any): GraphQLESTreeNode<T> => {

packages/plugin/src/estree-parser/utils.ts

@@ -9,7 +9,7 @@ import {
   Source,
   Lexer,
 } from 'graphql';
-import type { Comment } from 'estree';
+import type { Comment, SourceLocation } from 'estree';
 import type { AST } from 'eslint';
 import { valueFromASTUntyped } from 'graphql/utilities/valueFromASTUntyped';
 
@@ -125,3 +125,29 @@ export function extractCommentsFromAst(loc: Location): Comment[] {
   }
   return comments;
 }
+
+export function convertLocation(location: Location): SourceLocation {
+  const { startToken, endToken, source, start, end } = location;
+  /*
+   * ESLint has 0-based column number
+   * https://eslint.org/docs/developer-guide/working-with-rules#contextreport
+   */
+  const loc = {
+    start: {
+      /*
+       * Kind.Document has startToken: { line: 0, column: 0 }, we set line as 1 and column as 0
+       */
+      line: startToken.line === 0 ? 1 : startToken.line,
+      column: startToken.column === 0 ? 0 : startToken.column - 1,
+    },
+    end: {
+      line: endToken.line,
+      column: endToken.column - 1,
+    },
+    source: source.body,
+  };
+  if (loc.start.column === loc.end.column) {
+    loc.end.column += end - start;
+  }
+  return loc;
+}

packages/plugin/src/rules/no-duplicate-fields.ts

@@ -1,12 +1,17 @@
-import { Kind, NameNode } from 'graphql';
-import { GraphQLESLintRule } from '../types';
-import { GraphQLESTreeNode } from '../estree-parser';
+import { ASTNode, ASTVisitor, FragmentDefinitionNode, Kind, NameNode, visit } from 'graphql';
+import type { GraphQLESLintRule, ReportDescriptor } from '../types';
+import type { SiblingOperations } from '../sibling-operations';
+import { logger, requireSiblingsOperations } from '../utils';
+import { convertLocation, GraphQLESTreeNode } from '../estree-parser';
 
 const RULE_ID = 'no-duplicate-fields';
 
+type MetaInfo = { fragmentName?: string; inlineFragmentName?: string };
+
 const rule: GraphQLESLintRule = {
   meta: {
     type: 'suggestion',
+    // eslint-disable-next-line eslint-plugin/require-meta-has-suggestions
     hasSuggestions: true,
     docs: {
       description: `Checks for duplicate fields in selection set, variables in operation definition, or in arguments set of a field.`,
@@ -57,56 +62,118 @@ const rule: GraphQLESLintRule = {
       ],
     },
     messages: {
-      [RULE_ID]: '{{ type }} `{{ fieldName }}` defined multiple times.',
+      [RULE_ID]: '{{ type }} `{{ fieldName }}` already defined{{ addition }}.',
     },
     schema: [],
   },
   create(context) {
-    function checkNode(usedFields: Set<string>, node: GraphQLESTreeNode<NameNode>): void {
+    let siblings: SiblingOperations | null = null;
+
+    try {
+      siblings = requireSiblingsOperations(RULE_ID, context);
+    } catch {
+      logger.warn(
+        `Rule "${RULE_ID}" works best with siblings operations loaded. For more info: https://bit.ly/graphql-eslint-operations`
+      );
+    }
+
+    function checkNode(
+      usedFields: Map<string, MetaInfo>,
+      node: GraphQLESTreeNode<NameNode> | NameNode,
+      parent = (node as any).parent,
+      meta: MetaInfo = {}
+    ): void {
       const fieldName = node.value;
-      if (usedFields.has(fieldName)) {
-        const { parent } = node as any;
-        context.report({
-          node,
-          messageId: RULE_ID,
-          data: {
-            type: parent.type,
-            fieldName,
-          },
-          suggest: [
-            {
-              desc: `Remove \`${fieldName}\` ${parent.type.toLowerCase()}`,
-              fix(fixer) {
-                return fixer.remove(parent.type === Kind.VARIABLE ? parent.parent : parent);
-              },
+      const field = usedFields.get(fieldName);
+      if (!field) {
+        usedFields.set(fieldName, meta);
+        return;
+      }
+
+      let addition = '';
+      if (field.fragmentName) {
+        addition = ` in \`${field.fragmentName}\` fragment`;
+      } else if (field.inlineFragmentName) {
+        addition = ` in \`${field.inlineFragmentName}\` inline fragment`;
+      }
+      const problem: ReportDescriptor = {
+        loc: 'startToken' in node.loc ? convertLocation(node.loc) : node.loc,
+        messageId: RULE_ID,
+        data: {
+          type: parent.kind,
+          fieldName,
+          addition,
+        },
+      };
+
+      if ('type' in node) {
+        problem.suggest = [
+          {
+            desc: `Remove \`${fieldName}\` ${parent.type.toLowerCase()}`,
+            fix(fixer) {
+              return fixer.remove(parent.type === Kind.VARIABLE ? parent.parent : parent);
             },
-          ],
-        });
-      } else {
-        usedFields.add(fieldName);
+          },
+        ];
       }
+
+      context.report(problem);
     }
 
     return {
       OperationDefinition(node) {
-        const set = new Set<string>();
+        const set = new Map();
         for (const varDef of node.variableDefinitions) {
           checkNode(set, varDef.variable.name);
         }
       },
       Field(node) {
-        const set = new Set<string>();
+        const set = new Map();
         for (const arg of node.arguments) {
           checkNode(set, arg.name);
         }
       },
-      SelectionSet(node) {
-        const set = new Set<string>();
-        for (const selection of node.selections) {
-          if (selection.kind === Kind.FIELD) {
-            checkNode(set, selection.alias || selection.name);
-          }
-        }
+      SelectionSet(selectionSet) {
+        const set = new Map();
+
+        const visitor: ASTVisitor = {
+          Field(node, _, _2, _3, ancestors): false | void {
+            const parent = ancestors[ancestors.length - 2] as ASTNode;
+            let meta: MetaInfo;
+            if (parent) {
+              if (parent.kind === Kind.INLINE_FRAGMENT) {
+                meta = { inlineFragmentName: parent.typeCondition.name.value };
+              } else if (parent.kind === Kind.FRAGMENT_DEFINITION) {
+                meta = { fragmentName: parent.name.value };
+              }
+            }
+            checkNode(set, node.alias || node.name, node, meta);
+            if (node.selectionSet) {
+              // Skip visiting node if field contains selections
+              return false;
+            }
+          },
+          FragmentSpread(spread): FragmentDefinitionNode | void {
+            const [fragment] = siblings.getFragment(spread.name.value);
+            if (fragment) {
+              return {
+                ...fragment.document,
+                selectionSet: {
+                  ...fragment.document.selectionSet,
+                  selections: fragment.document.selectionSet.selections.map(node => {
+                    const enhancedNode = { ...node };
+                    if ('name' in enhancedNode) {
+                      enhancedNode.name = { ...enhancedNode.name, loc: spread.name.loc };
+                    }
+                    return enhancedNode;
+                  }),
+                },
+              };
+            }
+          },
+        };
+
+        visit(selectionSet.rawNode(), visitor);
       },
     };
   },

packages/plugin/src/rules/require-id-when-available.ts

@@ -9,6 +9,7 @@ import {
   visit,
   visitWithTypeInfo,
 } from 'graphql';
+import type { AST } from 'eslint';
 import type * as ESTree from 'estree';
 import { asArray } from '@graphql-tools/utils';
 import { ARRAY_DEFAULT_OPTIONS, requireGraphQLSchemaFromContext, requireSiblingsOperations } from '../utils';
@@ -132,7 +133,7 @@ const rule: GraphQLESLintRule<[RequireIdWhenAvailableRuleConfig], true> = {
             if (parent.kind === Kind.FRAGMENT_DEFINITION) {
               checkedFragmentSpreads.add(parent.name.value);
             } else if (parent.kind !== Kind.INLINE_FRAGMENT) {
-              checkSelections(node, typeInfo.getType(), selection.loc.start, parent, checkedFragmentSpreads);
+              checkSelections(node, typeInfo.getType(), selection.name.loc, parent, checkedFragmentSpreads);
             }
           },
         });
@@ -146,7 +147,7 @@ const rule: GraphQLESLintRule<[RequireIdWhenAvailableRuleConfig], true> = {
       type: GraphQLOutputType,
       // Fragment can be placed in separate file
       // Provide actual fragment spread location instead of location in fragment
-      loc: ESTree.Position,
+      loc: AST.SourceLocation | ESTree.Position,
       // Can't access to node.parent in GraphQL AST.Node, so pass as argument
       parent: any,
       checkedFragmentSpreads = new Set<string>()

packages/plugin/src/rules/selection-set-depth.ts

@@ -85,9 +85,9 @@ const rule: GraphQLESLintRule<[SelectionSetDepthRuleConfig]> = {
 
     try {
       siblings = requireSiblingsOperations(RULE_ID, context);
-    } catch (e) {
+    } catch {
       logger.warn(
-        `Rule "${RULE_ID}" works best with siblings operations loaded. For more info: http://bit.ly/graphql-eslint-operations`
+        `Rule "${RULE_ID}" works best with siblings operations loaded. For more info: https://bit.ly/graphql-eslint-operations`
       );
     }
 

packages/plugin/tests/__snapshots__/examples.spec.md

@@ -185,13 +185,21 @@ Array [
         message: Anonymous GraphQL operations are forbidden. Make sure to name your query!,
         ruleId: @graphql-eslint/no-anonymous-operations,
       },
+      Object {
+        message: Field \`name\` already defined.,
+        ruleId: @graphql-eslint/no-duplicate-fields,
+      },
+      Object {
+        message: Field \`name\` already defined.,
+        ruleId: @graphql-eslint/no-duplicate-fields,
+      },
     ],
   },
   Object {
     filePath: examples/graphql-config/operations/user.fragment.graphql,
     messages: Array [
       Object {
-        message: Field \`name\` defined multiple times.,
+        message: Field \`name\` already defined.,
         ruleId: @graphql-eslint/no-duplicate-fields,
       },
     ],