[compiler] Ignore ESLint suppressions when ValidateMemoDeps enabled

With `ValidateExhaustiveMemoDependencies` we can now check exhaustive dependencies for useMemo and useCallback within the compiler, without relying on the separate exhaustive-deps rule. Until now we've bailed out of any component/hook that suppresses this rule, since the suppression _might_ affect a memoization value. Compiling code with incorrect memo deps can change behavior so this wasn't safe. The downside was that a suppression within a useEffect could prevent memoization, even though non-exhaustive deps for effects do not cause problems for memoization specifically.

So here, we change to ignore ESLint suppressions if we have both the compiler's hooks validation and memo deps validations enabled.

Now we just have to test out the new validation and refine before we can enable this by default.

compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateExhaustiveDependencies.ts

@@ -24,7 +24,6 @@ import {
   InstructionKind,
   isStableType,
   isSubPath,
-  isSubPathIgnoringOptionals,
   isUseRefType,
   LoadGlobal,
   ManualMemoDependency,
@@ -241,10 +240,7 @@ export function validateExhaustiveDependencies(
           manualDependency.root.value.identifier.id ===
             inferredDependency.identifier.id &&
           (areEqualPaths(manualDependency.path, inferredDependency.path) ||
-            isSubPathIgnoringOptionals(
-              manualDependency.path,
-              inferredDependency.path,
-            ))
+            isSubPath(manualDependency.path, inferredDependency.path))
         ) {
           hasMatchingManualDependency = true;
           matched.add(manualDependency);
@@ -265,20 +261,6 @@ export function validateExhaustiveDependencies(
       extra.push(dep);
     }
 
-    /*
-     * For compatiblity with the existing exhaustive-deps rule, we allow
-     * known-stable values as dependencies even if the value is not reactive.
-     * This allows code that takes a dep on a non-reactive setState function
-     * to pass, for example.
-     */
-    retainWhere(extra, dep => {
-      const isNonReactiveStableValue =
-        dep.root.kind === 'NamedLocal' &&
-        !dep.root.value.reactive &&
-        isStableType(dep.root.value.identifier);
-      return !isNonReactiveStableValue;
-    });
-
     if (missing.length !== 0 || extra.length !== 0) {
       let suggestions: Array<CompilerSuggestion> | null = null;
       if (startMemo.depsLoc != null && typeof startMemo.depsLoc !== 'symbol') {

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps.expect.md

@@ -9,29 +9,23 @@ import {Stringify} from 'shared-runtime';
 function Component({x, y, z}) {
   const a = useMemo(() => {
     return x?.y.z?.a;
-    // error: too precise
   }, [x?.y.z?.a.b]);
   const b = useMemo(() => {
     return x.y.z?.a;
-    // ok, not our job to type check nullability
   }, [x.y.z.a]);
   const c = useMemo(() => {
     return x?.y.z.a?.b;
-    // error: too precise
   }, [x?.y.z.a?.b.z]);
   const d = useMemo(() => {
     return x?.y?.[(console.log(y), z?.b)];
-    // ok
   }, [x?.y, y, z?.b]);
   const e = useMemo(() => {
     const e = [];
     e.push(x);
     return e;
-    // ok
   }, [x]);
   const f = useMemo(() => {
     return [];
-    // error: unnecessary
   }, [x, y.z, z?.y?.a, UNUSED_GLOBAL]);
   const ref1 = useRef(null);
   const ref2 = useRef(null);
@@ -40,7 +34,6 @@ function Component({x, y, z}) {
     return () => {
       return ref.current;
     };
-    // error: ref is a stable type but reactive
   }, []);
   return <Stringify results={[a, b, c, d, e, f, cb]} />;
 }
@@ -51,7 +44,7 @@ function Component({x, y, z}) {
 ## Error
 
 ```
-Found 4 errors:
+Found 5 errors:
 
 Error: Found non-exhaustive dependencies
 
@@ -62,48 +55,61 @@ error.invalid-exhaustive-deps.ts:7:11
    6 |   const a = useMemo(() => {
 >  7 |     return x?.y.z?.a;
      |            ^^^^^^^^^ Missing dependency `x?.y.z?.a`
-   8 |     // error: too precise
-   9 |   }, [x?.y.z?.a.b]);
-  10 |   const b = useMemo(() => {
+   8 |   }, [x?.y.z?.a.b]);
+   9 |   const b = useMemo(() => {
+  10 |     return x.y.z?.a;
 
 Error: Found non-exhaustive dependencies
 
 Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI.
 
-error.invalid-exhaustive-deps.ts:15:11
-  13 |   }, [x.y.z.a]);
-  14 |   const c = useMemo(() => {
-> 15 |     return x?.y.z.a?.b;
+error.invalid-exhaustive-deps.ts:10:11
+   8 |   }, [x?.y.z?.a.b]);
+   9 |   const b = useMemo(() => {
+> 10 |     return x.y.z?.a;
+     |            ^^^^^^^^ Missing dependency `x.y.z?.a`
+  11 |   }, [x.y.z.a]);
+  12 |   const c = useMemo(() => {
+  13 |     return x?.y.z.a?.b;
+
+Error: Found non-exhaustive dependencies
+
+Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI.
+
+error.invalid-exhaustive-deps.ts:13:11
+  11 |   }, [x.y.z.a]);
+  12 |   const c = useMemo(() => {
+> 13 |     return x?.y.z.a?.b;
      |            ^^^^^^^^^^^ Missing dependency `x?.y.z.a?.b`
-  16 |     // error: too precise
-  17 |   }, [x?.y.z.a?.b.z]);
-  18 |   const d = useMemo(() => {
+  14 |   }, [x?.y.z.a?.b.z]);
+  15 |   const d = useMemo(() => {
+  16 |     return x?.y?.[(console.log(y), z?.b)];
 
 Error: Found unnecessary memoization dependencies
 
 Unnecessary dependencies can cause a value to update more often than necessary, which can cause effects to run more than expected.
 
-error.invalid-exhaustive-deps.ts:31:5
-  29 |     return [];
-  30 |     // error: unnecessary
-> 31 |   }, [x, y.z, z?.y?.a, UNUSED_GLOBAL]);
+error.invalid-exhaustive-deps.ts:25:5
+  23 |   const f = useMemo(() => {
+  24 |     return [];
+> 25 |   }, [x, y.z, z?.y?.a, UNUSED_GLOBAL]);
      |      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Unnecessary dependencies `x`, `y.z`, `z?.y?.a`, `UNUSED_GLOBAL`
-  32 |   const ref1 = useRef(null);
-  33 |   const ref2 = useRef(null);
-  34 |   const ref = z ? ref1 : ref2;
+  26 |   const ref1 = useRef(null);
+  27 |   const ref2 = useRef(null);
+  28 |   const ref = z ? ref1 : ref2;
 
 Error: Found non-exhaustive dependencies
 
 Missing dependencies can cause a value not to update when those inputs change, resulting in stale UI.
 
-error.invalid-exhaustive-deps.ts:37:13
-  35 |   const cb = useMemo(() => {
-  36 |     return () => {
-> 37 |       return ref.current;
+error.invalid-exhaustive-deps.ts:31:13
+  29 |   const cb = useMemo(() => {
+  30 |     return () => {
+> 31 |       return ref.current;
      |              ^^^ Missing dependency `ref`. Refs, setState functions, and other "stable" values generally do not need to be added as dependencies, but this variable may change over time to point to different values
-  38 |     };
-  39 |     // error: ref is a stable type but reactive
-  40 |   }, []);
+  32 |     };
+  33 |   }, []);
+  34 |   return <Stringify results={[a, b, c, d, e, f, cb]} />;
 ```
           
       

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-exhaustive-deps.js

@@ -5,29 +5,23 @@ import {Stringify} from 'shared-runtime';
 function Component({x, y, z}) {
   const a = useMemo(() => {
     return x?.y.z?.a;
-    // error: too precise
   }, [x?.y.z?.a.b]);
   const b = useMemo(() => {
     return x.y.z?.a;
-    // ok, not our job to type check nullability
   }, [x.y.z.a]);
   const c = useMemo(() => {
     return x?.y.z.a?.b;
-    // error: too precise
   }, [x?.y.z.a?.b.z]);
   const d = useMemo(() => {
     return x?.y?.[(console.log(y), z?.b)];
-    // ok
   }, [x?.y, y, z?.b]);
   const e = useMemo(() => {
     const e = [];
     e.push(x);
     return e;
-    // ok
   }, [x]);
   const f = useMemo(() => {
     return [];
-    // error: unnecessary
   }, [x, y.z, z?.y?.a, UNUSED_GLOBAL]);
   const ref1 = useRef(null);
   const ref2 = useRef(null);
@@ -36,7 +30,6 @@ function Component({x, y, z}) {
     return () => {
       return ref.current;
     };
-    // error: ref is a stable type but reactive
   }, []);
   return <Stringify results={[a, b, c, d, e, f, cb]} />;
 }

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps-allow-nonreactive-stable-types-as-extra-deps.expect.md

@@ -1,100 +0,0 @@
-
-## Input
-
-```javascript
-// @validateExhaustiveMemoizationDependencies
-import {
-  useCallback,
-  useTransition,
-  useState,
-  useOptimistic,
-  useActionState,
-  useRef,
-  useReducer,
-} from 'react';
-
-function useFoo() {
-  const [s, setState] = useState();
-  const ref = useRef(null);
-  const [t, startTransition] = useTransition();
-  const [u, addOptimistic] = useOptimistic();
-  const [v, dispatch] = useReducer(() => {}, null);
-  const [isPending, dispatchAction] = useActionState(() => {}, null);
-
-  return useCallback(() => {
-    dispatch();
-    startTransition(() => {});
-    addOptimistic();
-    setState(null);
-    dispatchAction();
-    ref.current = true;
-  }, [
-    // intentionally adding unnecessary deps on nonreactive stable values
-    // to check that they're allowed
-    dispatch,
-    startTransition,
-    addOptimistic,
-    setState,
-    dispatchAction,
-    ref,
-  ]);
-}
-
-export const FIXTURE_ENTRYPOINT = {
-  fn: useFoo,
-  params: [],
-};
-
-```
-
-## Code
-
-```javascript
-import { c as _c } from "react/compiler-runtime"; // @validateExhaustiveMemoizationDependencies
-import {
-  useCallback,
-  useTransition,
-  useState,
-  useOptimistic,
-  useActionState,
-  useRef,
-  useReducer,
-} from "react";
-
-function useFoo() {
-  const $ = _c(1);
-  const [, setState] = useState();
-  const ref = useRef(null);
-  const [, startTransition] = useTransition();
-  const [, addOptimistic] = useOptimistic();
-  const [, dispatch] = useReducer(_temp, null);
-  const [, dispatchAction] = useActionState(_temp2, null);
-  let t0;
-  if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
-    t0 = () => {
-      dispatch();
-      startTransition(_temp3);
-      addOptimistic();
-      setState(null);
-      dispatchAction();
-      ref.current = true;
-    };
-    $[0] = t0;
-  } else {
-    t0 = $[0];
-  }
-  return t0;
-}
-function _temp3() {}
-function _temp2() {}
-function _temp() {}
-
-export const FIXTURE_ENTRYPOINT = {
-  fn: useFoo,
-  params: [],
-};
-
-```
-      
-### Eval output
-(kind: ok) "[[ function params=0 ]]"

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/exhaustive-deps-allow-nonreactive-stable-types-as-extra-deps.js

@@ -1,42 +0,0 @@
-// @validateExhaustiveMemoizationDependencies
-import {
-  useCallback,
-  useTransition,
-  useState,
-  useOptimistic,
-  useActionState,
-  useRef,
-  useReducer,
-} from 'react';
-
-function useFoo() {
-  const [s, setState] = useState();
-  const ref = useRef(null);
-  const [t, startTransition] = useTransition();
-  const [u, addOptimistic] = useOptimistic();
-  const [v, dispatch] = useReducer(() => {}, null);
-  const [isPending, dispatchAction] = useActionState(() => {}, null);
-
-  return useCallback(() => {
-    dispatch();
-    startTransition(() => {});
-    addOptimistic();
-    setState(null);
-    dispatchAction();
-    ref.current = true;
-  }, [
-    // intentionally adding unnecessary deps on nonreactive stable values
-    // to check that they're allowed
-    dispatch,
-    startTransition,
-    addOptimistic,
-    setState,
-    dispatchAction,
-    ref,
-  ]);
-}
-
-export const FIXTURE_ENTRYPOINT = {
-  fn: useFoo,
-  params: [],
-};

packages/react-devtools-shared/src/__tests__/profilerContext-test.js

@@ -584,75 +584,4 @@ describe('ProfilerContext', () => {
     await utils.actAsync(() => context.selectFiber(childID, 'Child'));
     expect(inspectedElementID).toBe(parentID);
   });
-
-  it('should toggle profiling when the keyboard shortcut is pressed', async () => {
-    // Context providers
-    const Profiler =
-      require('react-devtools-shared/src/devtools/views/Profiler/Profiler').default;
-    const {
-      TimelineContextController,
-    } = require('react-devtools-timeline/src/TimelineContext');
-    const {
-      SettingsContextController,
-    } = require('react-devtools-shared/src/devtools/views/Settings/SettingsContext');
-    const {
-      ModalDialogContextController,
-    } = require('react-devtools-shared/src/devtools/views/ModalDialog');
-
-    // Dom component for profiling to be enabled
-    const Component = () => null;
-    utils.act(() => render(<Component />));
-
-    const profilerContainer = document.createElement('div');
-    document.body.appendChild(profilerContainer);
-
-    // Create a root for the profiler
-    const profilerRoot = ReactDOMClient.createRoot(profilerContainer);
-
-    // Render the profiler
-    utils.act(() => {
-      profilerRoot.render(
-        <Contexts>
-          <SettingsContextController browserTheme="light">
-            <ModalDialogContextController>
-              <TimelineContextController>
-                <Profiler />
-              </TimelineContextController>
-            </ModalDialogContextController>
-          </SettingsContextController>
-        </Contexts>,
-      );
-    });
-
-    // Verify that the profiler is not profiling.
-    expect(store.profilerStore.isProfilingBasedOnUserInput).toBe(false);
-
-    // Trigger the keyboard shortcut.
-    const ownerWindow = profilerContainer.ownerDocument.defaultView;
-    const isMac =
-      typeof navigator !== 'undefined' &&
-      navigator.platform.toUpperCase().indexOf('MAC') >= 0;
-
-    const keyEvent = new KeyboardEvent('keydown', {
-      key: 'e',
-      metaKey: isMac,
-      ctrlKey: !isMac,
-      bubbles: true,
-    });
-
-    // Dispatch keyboard event to toggle profiling on
-    // Try utils.actAsync with recursivelyFlush=false
-    await utils.actAsync(() => {
-      ownerWindow.dispatchEvent(keyEvent);
-    }, false);
-    expect(store.profilerStore.isProfilingBasedOnUserInput).toBe(true);
-
-    // Dispatch keyboard event to toggle profiling off
-    await utils.actAsync(() => {
-      ownerWindow.dispatchEvent(keyEvent);
-    }, false);
-    expect(store.profilerStore.isProfilingBasedOnUserInput).toBe(false);
-
-    document.body.removeChild(profilerContainer);
-  });
 });

packages/react-devtools-shared/src/devtools/views/Profiler/Profiler.js

@@ -8,7 +8,7 @@
  */
 
 import * as React from 'react';
-import {Fragment, useContext, useEffect, useRef, useEffectEvent} from 'react';
+import {Fragment, useContext} from 'react';
 import {ModalDialog} from '../ModalDialog';
 import {ProfilerContext} from './ProfilerContext';
 import TabBar from '../TabBar';
@@ -38,11 +38,6 @@ import {TimelineContext} from 'react-devtools-timeline/src/TimelineContext';
 import styles from './Profiler.css';
 
 function Profiler(_: {}) {
-  const profilerRef = useRef<HTMLDivElement | null>(null);
-  const isMac =
-    typeof navigator !== 'undefined' &&
-    navigator.platform.toUpperCase().indexOf('MAC') >= 0;
-
   const {
     didRecordCommits,
     isProcessingData,
@@ -52,8 +47,6 @@ function Profiler(_: {}) {
     selectedTabID,
     selectTab,
     supportsProfiling,
-    startProfiling,
-    stopProfiling,
   } = useContext(ProfilerContext);
 
   const {file: timelineTraceEventData, searchInputContainerRef} =
@@ -63,32 +56,6 @@ function Profiler(_: {}) {
 
   const isLegacyProfilerSelected = selectedTabID !== 'timeline';
 
-  // Cmd+E to start/stop profiler recording
-  const handleKeyDown = useEffectEvent((event: KeyboardEvent) => {
-    const correctModifier = isMac ? event.metaKey : event.ctrlKey;
-    if (correctModifier && event.key === 'e') {
-      if (isProfiling) {
-        stopProfiling();
-      } else {
-        startProfiling();
-      }
-      event.preventDefault();
-      event.stopPropagation();
-    }
-  });
-
-  useEffect(() => {
-    const div = profilerRef.current;
-    if (!div) {
-      return;
-    }
-    const ownerWindow = div.ownerDocument.defaultView;
-    ownerWindow.addEventListener('keydown', handleKeyDown);
-    return () => {
-      ownerWindow.removeEventListener('keydown', handleKeyDown);
-    };
-  }, []);
-
   let view = null;
   if (didRecordCommits || selectedTabID === 'timeline') {
     switch (selectedTabID) {
@@ -145,7 +112,7 @@ function Profiler(_: {}) {
 
   return (
     <SettingsModalContextController>
-      <div ref={profilerRef} className={styles.Profiler}>
+      <div className={styles.Profiler}>
         <div className={styles.LeftColumn}>
           <div className={styles.Toolbar}>
             <RecordToggle disabled={!supportsProfiling} />

packages/react-devtools-shared/src/devtools/views/Profiler/RecordToggle.js

@@ -30,19 +30,13 @@ export default function RecordToggle({disabled}: Props): React.Node {
     className = styles.ActiveRecordToggle;
   }
 
-  const isMac =
-    typeof navigator !== 'undefined' &&
-    navigator.platform.toUpperCase().indexOf('MAC') >= 0;
-  const shortcut = isMac ? '⌘E' : 'Ctrl+E';
-  const title = `${isProfiling ? 'Stop' : 'Start'} profiling - ${shortcut}`;
-
   return (
     <Button
       className={className}
       disabled={disabled}
       onClick={isProfiling ? stopProfiling : startProfiling}
       testName="ProfilerToggleButton"
-      title={title}>
+      title={isProfiling ? 'Stop profiling' : 'Start profiling'}>
       <ButtonIcon type="record" />
     </Button>
   );