feat(eslint-plugin): [member-ordering] add a required option for requ…

…ired vs. optional member ordering (#5965)

* fix(eslint-plugin): [member-ordering] add requiredFirst as an option which ensures that all required members appear before all optional members.

* fix(eslint-plugin): [member-ordering] adding types so build passes.

* fix(eslint-plugin): [member-ordering] fixing types so build passes.

* fix(eslint-plugin): [member-ordering] refactoring getIndexOfLastRequiredMember to be slightly faster and adding jsdoc comments for it and isMemberOptional.

* fix(eslint-plugin): [member-ordering] additional test cases and handling for them.

* fix(eslint-plugin): [member-ordering] linting fix.

* fix(eslint-plugin): [member-ordering] change requiredFirst to required which takes first or last as a value and adding functionality to check order based on both of these along with additional tests.

* fix(eslint-plugin): [member-ordering] refactoring according to PR comments.

* fix(eslint-plugin): [member-ordering] refactoring for PR and adding another test case.

* fix(eslint-plugin): [member-ordering] refactoring for PR.

* fix(eslint-plugin): [member-ordering] adding test cases for coverage and removing unused code.

* fix(eslint-plugin): [member-ordering] increasing coverage to pass check.

* feat(eslint-plugin): [member-ordering] adding more tests to increase coverage for isMemberOptional function.

* Updated name to optionalityOrder

Co-authored-by: Josh Goldberg <git@joshuakgoldberg.com>

packages/eslint-plugin/docs/rules/member-ordering.md

@@ -24,6 +24,7 @@ type OrderConfig = MemberType[] | SortedOrderConfig | 'never';
 
 interface SortedOrderConfig {
   memberTypes?: MemberType[] | 'never';
+  optionalityOrder?: 'optional-first' | 'required-first';
   order:
     | 'alphabetically'
     | 'alphabetically-case-insensitive'
@@ -44,9 +45,10 @@ You can configure `OrderConfig` options for:
 - **`interfaces`**?: override ordering specifically for interfaces
 - **`typeLiterals`**?: override ordering specifically for type literals
 
-The `OrderConfig` settings for each kind of construct may configure sorting on one or both two levels:
+The `OrderConfig` settings for each kind of construct may configure sorting on up to three levels:
 
 - **`memberTypes`**: organizing on member type groups such as methods vs. properties
+- **`optionalityOrder`**: whether to put all optional members first or all required members first
 - **`order`**: organizing based on member names, such as alphabetically
 
 ### Groups
@@ -911,6 +913,96 @@ interface Foo {
 }
 ```
 
+#### Sorting Optional Members First or Last
+
+The `optionalityOrder` option may be enabled to place all optional members in a group at the beginning or end of that group.
+
+This config places all optional members before all required members:
+
+```jsonc
+// .eslintrc.json
+{
+  "rules": {
+    "@typescript-eslint/member-ordering": [
+      "error",
+      {
+        "default": {
+          "optionalityOrder": "optional-first",
+          "order": "alphabetically"
+        }
+      }
+    ]
+  }
+}
+```
+
+<!--tabs-->
+
+##### ❌ Incorrect
+
+```ts
+interface Foo {
+  a: boolean;
+  b?: number;
+  c: string;
+}
+```
+
+##### ✅ Correct
+
+```ts
+interface Foo {
+  b?: number;
+  a: boolean;
+  c: string;
+}
+```
+
+<!--/tabs-->
+
+This config places all required members before all optional members:
+
+```jsonc
+// .eslintrc.json
+{
+  "rules": {
+    "@typescript-eslint/member-ordering": [
+      "error",
+      {
+        "default": {
+          "optionalityOrder": "required-first",
+          "order": "alphabetically"
+        }
+      }
+    ]
+  }
+}
+```
+
+<!--tabs-->
+
+##### ❌ Incorrect
+
+```ts
+interface Foo {
+  a: boolean;
+  b?: number;
+  c: string;
+}
+```
+
+##### ✅ Correct
+
+```ts
+interface Foo {
+  a: boolean;
+  c: string;
+  b?: number;
+}
+```
+
+<!--/tabs-->
+
 ## All Supported Options
 
 ### Member Types (Granular Form)

packages/eslint-plugin/src/rules/member-ordering.ts

@@ -4,7 +4,10 @@ import naturalCompare from 'natural-compare-lite';
 
 import * as util from '../util';
 
-export type MessageIds = 'incorrectGroupOrder' | 'incorrectOrder';
+export type MessageIds =
+  | 'incorrectGroupOrder'
+  | 'incorrectOrder'
+  | 'incorrectRequiredMembersOrder';
 
 type MemberKind =
   | 'call-signature'
@@ -47,12 +50,15 @@ type Order = AlphabeticalOrder | 'as-written';
 
 interface SortedOrderConfig {
   memberTypes?: MemberType[] | 'never';
+  optionalityOrder?: OptionalityOrder;
   order: Order;
 }
 
 type OrderConfig = MemberType[] | SortedOrderConfig | 'never';
 type Member = TSESTree.ClassElement | TSESTree.TypeElement;
 
+type OptionalityOrder = 'optional-first' | 'required-first';
+
 export type Options = [
   {
     default?: OrderConfig;
@@ -101,6 +107,10 @@ const objectConfig = (memberTypes: MemberType[]): JSONSchema.JSONSchema4 => ({
         'natural-case-insensitive',
       ],
     },
+    optionalityOrder: {
+      type: 'string',
+      enum: ['optional-first', 'required-first'],
+    },
   },
   additionalProperties: false,
 });
@@ -405,6 +415,26 @@ function getMemberName(
   }
 }
 
+/**
+ * Returns true if the member is optional based on the member type.
+ *
+ * @param node the node to be evaluated.
+ *
+ * @returns Whether the member is optional, or false if it cannot be optional at all.
+ */
+function isMemberOptional(node: Member): boolean {
+  switch (node.type) {
+    case AST_NODE_TYPES.TSPropertySignature:
+    case AST_NODE_TYPES.TSMethodSignature:
+    case AST_NODE_TYPES.TSAbstractPropertyDefinition:
+    case AST_NODE_TYPES.PropertyDefinition:
+    case AST_NODE_TYPES.TSAbstractMethodDefinition:
+    case AST_NODE_TYPES.MethodDefinition:
+      return !!node.optional;
+  }
+  return false;
+}
+
 /**
  * Gets the calculated rank using the provided method definition.
  * The algorithm is as follows:
@@ -561,6 +591,7 @@ export default util.createRule<Options, MessageIds>({
         'Member {{member}} should be declared before member {{beforeMember}}.',
       incorrectGroupOrder:
         'Member {{name}} should be declared before all {{rank}} definitions.',
+      incorrectRequiredMembersOrder: `Member {{member}} should be declared after all {{optionalOrRequired}} members.`,
     },
     schema: [
       {
@@ -725,6 +756,59 @@ export default util.createRule<Options, MessageIds>({
       }
     }
 
+    /**
+     * Checks if the order of optional and required members is correct based
+     * on the given 'required' parameter.
+     *
+     * @param members Members to be validated.
+     * @param optionalityOrder Where to place optional members, if not intermixed.
+     *
+     * @return True if all required and optional members are correctly sorted.
+     */
+    function checkRequiredOrder(
+      members: Member[],
+      optionalityOrder: OptionalityOrder | undefined,
+    ): boolean {
+      const switchIndex = members.findIndex(
+        (member, i) =>
+          i && isMemberOptional(member) !== isMemberOptional(members[i - 1]),
+      );
+
+      const report = (member: Member): void =>
+        context.report({
+          messageId: 'incorrectRequiredMembersOrder',
+          loc: member.loc,
+          data: {
+            member: getMemberName(member, context.getSourceCode()),
+            optionalOrRequired:
+              optionalityOrder === 'optional-first' ? 'required' : 'optional',
+          },
+        });
+
+      // if the optionality of the first item is correct (based on optionalityOrder)
+      // then the first 0 inclusive to switchIndex exclusive members all
+      // have the correct optionality
+      if (
+        isMemberOptional(members[0]) !==
+        (optionalityOrder === 'required-first')
+      ) {
+        report(members[0]);
+        return false;
+      }
+
+      for (let i = switchIndex + 1; i < members.length; i++) {
+        if (
+          isMemberOptional(members[i]) !==
+          isMemberOptional(members[switchIndex])
+        ) {
+          report(members[switchIndex]);
+          return false;
+        }
+      }
+
+      return true;
+    }
+
     /**
      * Validates if all members are correctly sorted.
      *
@@ -743,33 +827,62 @@ export default util.createRule<Options, MessageIds>({
 
       // Standardize config
       let order: Order | undefined;
-      let memberTypes;
+      let memberTypes: string | MemberType[] | undefined;
+      let optionalityOrder: OptionalityOrder | undefined;
+
+      // returns true if everything is good and false if an error was reported
+      const checkOrder = (memberSet: Member[]): boolean => {
+        const hasAlphaSort = !!(order && order !== 'as-written');
+
+        // Check order
+        if (Array.isArray(memberTypes)) {
+          const grouped = checkGroupSort(
+            memberSet,
+            memberTypes,
+            supportsModifiers,
+          );
+
+          if (grouped === null) {
+            return false;
+          }
+
+          if (hasAlphaSort) {
+            return !grouped.some(
+              groupMember =>
+                !checkAlphaSort(groupMember, order as AlphabeticalOrder),
+            );
+          }
+        } else if (hasAlphaSort) {
+          return checkAlphaSort(memberSet, order as AlphabeticalOrder);
+        }
+
+        return true;
+      };
 
       if (Array.isArray(orderConfig)) {
         memberTypes = orderConfig;
       } else {
         order = orderConfig.order;
         memberTypes = orderConfig.memberTypes;
+        optionalityOrder = orderConfig.optionalityOrder;
       }
 
-      const hasAlphaSort = !!(order && order !== 'as-written');
+      if (!optionalityOrder) {
+        checkOrder(members);
+        return;
+      }
 
-      // Check order
-      if (Array.isArray(memberTypes)) {
-        const grouped = checkGroupSort(members, memberTypes, supportsModifiers);
+      const switchIndex = members.findIndex(
+        (member, i) =>
+          i && isMemberOptional(member) !== isMemberOptional(members[i - 1]),
+      );
 
-        if (grouped === null) {
+      if (switchIndex !== -1) {
+        if (!checkRequiredOrder(members, optionalityOrder)) {
           return;
         }
-
-        if (hasAlphaSort) {
-          grouped.some(
-            groupMember =>
-              !checkAlphaSort(groupMember, order as AlphabeticalOrder),
-          );
-        }
-      } else if (hasAlphaSort) {
-        checkAlphaSort(members, order as AlphabeticalOrder);
+        checkOrder(members.slice(0, switchIndex));
+        checkOrder(members.slice(switchIndex));
       }
     }
 

packages/eslint-plugin/src/util/misc.ts

@@ -180,6 +180,29 @@ function formatWordList(words: string[]): string {
   return [words.slice(0, -1).join(', '), words.slice(-1)[0]].join(' and ');
 }
 
+/**
+ * Iterates the array in reverse and returns the index of the first element it
+ * finds which passes the predicate function.
+ *
+ * @returns Returns the index of the element if it finds it or -1 otherwise.
+ */
+function findLastIndex<T>(
+  members: T[],
+  predicate: (member: T) => boolean | undefined | null,
+): number {
+  let idx = members.length - 1;
+
+  while (idx >= 0) {
+    const valid = predicate(members[idx]);
+    if (valid) {
+      return idx;
+    }
+    idx--;
+  }
+
+  return -1;
+}
+
 export {
   arrayGroupByToMap,
   arraysAreEqual,
@@ -194,4 +217,5 @@ export {
   MemberNameType,
   RequireKeys,
   upperCaseFirst,
+  findLastIndex,
 };