Move createElement/JSX Warnings into the Renderer (#29088)

This is necessary to simplify the component stack handling to make way
for owner stacks. It also solves some hacks that we used to have but
don't quite make sense. It also solves the problem where things like key
warnings get silenced in RSC because they get deduped. It also surfaces
areas where we were missing key warnings to begin with.

Almost every type of warning is issued from the renderer. React Elements
are really not anything special themselves. They're just lazily invoked
functions and its really the renderer that determines there semantics.

We have three types of warnings that previously fired in
JSX/createElement:

- Fragment props validation.
- Type validation.
- Key warning.

It's nice to be able to do some validation in the JSX/createElement
because it has a more specific stack frame at the callsite. However,
that's the case for every type of component and validation. That's the
whole point of enableOwnerStacks. It's also not sufficient to do it in
JSX/createElement so we also have validation in the renderers too. So
this validation is really just an eager validation but also happens
again later.

The problem with these is that we don't really know what types are valid
until we get to the renderer. Additionally, by placing it in the
isomorphic code it becomes harder to do deduping of warnings in a way
that makes sense for that renderer. It also means we can't reuse logic
for managing stacks etc.

Fragment props validation really should just be part of the renderer
like any other component type. This also matters once we add Fragment
refs and other fragment features. So I moved this into Fiber. However,
since some Fragments don't have Fibers, I do the validation in
ChildFiber instead of beginWork where it would normally happen.

For `type` validation we already do validation when rendering. By
leaving it to the renderer we don't have to hard code an extra list.
This list also varies by context. E.g. class components aren't allowed
in RSC but client references are but we don't have an isomorphic way to
identify client references because they're defined by the host config so
the current logic is flawed anyway. I kept the early validation for now
without the `enableOwnerStacks` since it does provide a nicer stack
frame but with that flag on it'll be handled with nice stacks anyway. I
normalized some of the errors to ensure tests pass.

For `key` validation it's the same principle. The mechanism for the
heuristic is still the same - if it passes statically through a parent
JSX/createElement call then it's considered validated. We already did
print the error later from the renderer so this also disables the early
log in the `enableOwnerStacks` flag.

I also added logging to Fizz so that key warnings can print in SSR logs.

Flight is a bit more complex. For elements that end up on the client we
just pass the `validated` flag along to the client and let the client
renderer print the error once rendered. For server components we log the
error from Flight with the server component as the owner on the stack
which will allow us to print the right stack for context. The factoring
of this is a little tricky because we only want to warn if it's in an
array parent but we want to log the error later to get the right debug
info.

Fiber/Fizz has a similar factoring problem that causes us to create a
fake Fiber for the owner which means the logs won't be associated with
the right place in DevTools.

Author: sebmarkbage

fixtures/flight/server/region.js

@@ -81,17 +81,20 @@ async function renderApp(res, returnValue, formState) {
     ).main.css;
   }
   const App = m.default.default || m.default;
-  const root = [
+  const root = React.createElement(
+    React.Fragment,
+    null,
     // Prepend the App's tree with stylesheets required for this entrypoint.
     mainCSSChunks.map(filename =>
       React.createElement('link', {
         rel: 'stylesheet',
         href: filename,
         precedence: 'default',
+        key: filename,
       })
     ),
-    React.createElement(App),
-  ];
+    React.createElement(App)
+  );
   // For client-invoked server actions we refresh the tree and return a return value.
   const payload = {root, returnValue, formState};
   const {pipe} = renderToPipeableStream(payload, moduleMap);

packages/react-client/src/ReactFlightClient.js

@@ -579,6 +579,7 @@ function createElement(
   props: mixed,
   owner: null | ReactComponentInfo, // DEV-only
   stack: null | string, // DEV-only
+  validated: number, // DEV-only
 ): React$Element<any> {
   let element: any;
   if (__DEV__ && enableRefAsProp) {
@@ -624,13 +625,13 @@ function createElement(
     // Unfortunately, _store is enumerable in jest matchers so for equality to
     // work, I need to keep it or make _store non-enumerable in the other file.
     element._store = ({}: {
-      validated?: boolean,
+      validated?: number,
     });
     Object.defineProperty(element._store, 'validated', {
       configurable: false,
       enumerable: false,
       writable: true,
-      value: true, // This element has already been validated on the server.
+      value: enableOwnerStacks ? validated : 1, // Whether the element has already been validated on the server.
     });
     // debugInfo contains Server Component debug information.
     Object.defineProperty(element, '_debugInfo', {
@@ -644,7 +645,7 @@ function createElement(
         configurable: false,
         enumerable: false,
         writable: true,
-        value: {stack: stack},
+        value: stack,
       });
       Object.defineProperty(element, '_debugTask', {
         configurable: false,
@@ -1053,6 +1054,7 @@ function parseModelTuple(
       tuple[3],
       __DEV__ ? (tuple: any)[4] : null,
       __DEV__ && enableOwnerStacks ? (tuple: any)[5] : null,
+      __DEV__ && enableOwnerStacks ? (tuple: any)[6] : 0,
     );
   }
   return value;

packages/react-client/src/__tests__/ReactFlight-test.js

@@ -1143,6 +1143,9 @@ describe('ReactFlight', () => {
         '\n' +
         'Check the render method of `Component`. See https://react.dev/link/warning-keys for more information.\n' +
         '    in span (at **)\n' +
+        // TODO: Because this validates after the div has been mounted, it is part of
+        // the parent stack but since owner stacks will switch to owners this goes away again.
+        (gate(flags => flags.enableOwnerStacks) ? '    in div (at **)\n' : '') +
         '    in Component (at **)\n' +
         '    in Indirection (at **)\n' +
         '    in App (at **)',
@@ -1386,19 +1389,40 @@ describe('ReactFlight', () => {
     ReactNoopFlightClient.read(transport);
   });
 
-  it('should warn in DEV a child is missing keys', () => {
+  it('should warn in DEV a child is missing keys on server component', () => {
+    function NoKey({children}) {
+      return <div key="this has a key but parent doesn't" />;
+    }
+    expect(() => {
+      const transport = ReactNoopFlightServer.render(
+        <div>{Array(6).fill(<NoKey />)}</div>,
+      );
+      ReactNoopFlightClient.read(transport);
+    }).toErrorDev('Each child in a list should have a unique "key" prop.', {
+      withoutStack: gate(flags => flags.enableOwnerStacks),
+    });
+  });
+
+  it('should warn in DEV a child is missing keys in client component', async () => {
     function ParentClient({children}) {
       return children;
     }
     const Parent = clientReference(ParentClient);
-    expect(() => {
+    await expect(async () => {
       const transport = ReactNoopFlightServer.render(
         <Parent>{Array(6).fill(<div>no key</div>)}</Parent>,
       );
       ReactNoopFlightClient.read(transport);
+      await act(async () => {
+        ReactNoop.render(await ReactNoopFlightClient.read(transport));
+      });
     }).toErrorDev(
-      'Each child in a list should have a unique "key" prop. ' +
-        'See https://react.dev/link/warning-keys for more information.',
+      gate(flags => flags.enableOwnerStacks)
+        ? 'Each child in a list should have a unique "key" prop.' +
+            '\n\nCheck the top-level render call using <ParentClient>. ' +
+            'See https://react.dev/link/warning-keys for more information.'
+        : 'Each child in a list should have a unique "key" prop. ' +
+            'See https://react.dev/link/warning-keys for more information.',
     );
   });
 
@@ -2306,7 +2330,7 @@ describe('ReactFlight', () => {
     }
 
     function ThirdPartyFragmentComponent() {
-      return [<span>Who</span>, ' ', <span>dis?</span>];
+      return [<span key="1">Who</span>, ' ', <span key="2">dis?</span>];
     }
 
     function ServerComponent({transport}) {
@@ -2318,7 +2342,7 @@ describe('ReactFlight', () => {
     const promiseComponent = Promise.resolve(<ThirdPartyComponent />);
 
     const thirdPartyTransport = ReactNoopFlightServer.render(
-      [promiseComponent, lazy, <ThirdPartyFragmentComponent />],
+      [promiseComponent, lazy, <ThirdPartyFragmentComponent key="3" />],
       {
         environmentName: 'third-party',
       },
@@ -2410,8 +2434,8 @@ describe('ReactFlight', () => {
     const iteratorPromise = new Promise(r => (resolve = r));
 
     async function* ThirdPartyAsyncIterableComponent({item, initial}) {
-      yield <span>Who</span>;
-      yield <span>dis?</span>;
+      yield <span key="1">Who</span>;
+      yield <span key="2">dis?</span>;
       resolve();
     }
 

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

@@ -2562,13 +2562,7 @@ describe('InspectedElement', () => {
       const data = await getErrorsAndWarningsForElementAtIndex(0);
       expect(data).toMatchInlineSnapshot(`
         {
-          "errors": [
-            [
-              "Warning: Each child in a list should have a unique "key" prop. See https://react.dev/link/warning-keys for more information.
-            at Example",
-              1,
-            ],
-          ],
+          "errors": [],
           "warnings": [],
         }
       `);

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

@@ -1932,9 +1932,8 @@ describe('Store', () => {
       );
 
       expect(store).toMatchInlineSnapshot(`
-        ✕ 1, ⚠ 0
         [root]
-          ▾ <Example> ✕
+          ▾ <Example>
               <Child>
       `);
     });

packages/react-devtools-shared/src/backend/DevToolsComponentStackFrame.js

@@ -12,22 +12,8 @@
 // while still maintaining support for multiple renderer versions
 // (which use different values for ReactTypeOfWork).
 
-import type {LazyComponent} from 'react/src/ReactLazy';
 import type {CurrentDispatcherRef} from './types';
 
-import {
-  FORWARD_REF_NUMBER,
-  FORWARD_REF_SYMBOL_STRING,
-  LAZY_NUMBER,
-  LAZY_SYMBOL_STRING,
-  MEMO_NUMBER,
-  MEMO_SYMBOL_STRING,
-  SUSPENSE_NUMBER,
-  SUSPENSE_SYMBOL_STRING,
-  SUSPENSE_LIST_NUMBER,
-  SUSPENSE_LIST_SYMBOL_STRING,
-} from './ReactSymbols';
-
 // The shared console patching code is DEV-only.
 // We can't use it since DevTools only ships production builds.
 import {disableLogs, reenableLogs} from './DevToolsConsolePatching';
@@ -297,69 +283,3 @@ export function describeFunctionComponentFrame(
 ): string {
   return describeNativeComponentFrame(fn, false, currentDispatcherRef);
 }
-
-function shouldConstruct(Component: Function) {
-  const prototype = Component.prototype;
-  return !!(prototype && prototype.isReactComponent);
-}
-
-export function describeUnknownElementTypeFrameInDEV(
-  type: any,
-  currentDispatcherRef: CurrentDispatcherRef,
-): string {
-  if (!__DEV__) {
-    return '';
-  }
-  if (type == null) {
-    return '';
-  }
-  if (typeof type === 'function') {
-    return describeNativeComponentFrame(
-      type,
-      shouldConstruct(type),
-      currentDispatcherRef,
-    );
-  }
-  if (typeof type === 'string') {
-    return describeBuiltInComponentFrame(type);
-  }
-  switch (type) {
-    case SUSPENSE_NUMBER:
-    case SUSPENSE_SYMBOL_STRING:
-      return describeBuiltInComponentFrame('Suspense');
-    case SUSPENSE_LIST_NUMBER:
-    case SUSPENSE_LIST_SYMBOL_STRING:
-      return describeBuiltInComponentFrame('SuspenseList');
-  }
-  if (typeof type === 'object') {
-    switch (type.$$typeof) {
-      case FORWARD_REF_NUMBER:
-      case FORWARD_REF_SYMBOL_STRING:
-        return describeFunctionComponentFrame(
-          type.render,
-          currentDispatcherRef,
-        );
-      case MEMO_NUMBER:
-      case MEMO_SYMBOL_STRING:
-        // Memo may contain any component type so we recursively resolve it.
-        return describeUnknownElementTypeFrameInDEV(
-          type.type,
-          currentDispatcherRef,
-        );
-      case LAZY_NUMBER:
-      case LAZY_SYMBOL_STRING: {
-        const lazyComponent: LazyComponent<any, any> = (type: any);
-        const payload = lazyComponent._payload;
-        const init = lazyComponent._init;
-        try {
-          // Lazy may contain any component type so we recursively resolve it.
-          return describeUnknownElementTypeFrameInDEV(
-            init(payload),
-            currentDispatcherRef,
-          );
-        } catch (x) {}
-      }
-    }
-  }
-  return '';
-}

packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js

@@ -1833,10 +1833,18 @@ describe('ReactDOMFizzServer', () => {
         expect(mockError).toHaveBeenCalledWith(
           'Warning: Each child in a list should have a unique "key" prop.%s%s' +
             ' See https://react.dev/link/warning-keys for more information.%s',
-          '\n\nCheck the top-level render call using <div>.',
+          gate(flags => flags.enableOwnerStacks)
+            ? // We currently don't track owners in Fizz which is responsible for this frame.
+              ''
+            : '\n\nCheck the top-level render call using <div>.',
           '',
           '\n' +
             '    in span (at **)\n' +
+            // TODO: Because this validates after the div has been mounted, it is part of
+            // the parent stack but since owner stacks will switch to owners this goes away again.
+            (gate(flags => flags.enableOwnerStacks)
+              ? '    in div (at **)\n'
+              : '') +
             '    in B (at **)\n' +
             '    in Suspense (at **)\n' +
             '    in div (at **)\n' +
@@ -1890,7 +1898,12 @@ describe('ReactDOMFizzServer', () => {
           </b>
         );
         if (this.props.prefix) {
-          return [readText(this.props.prefix), child];
+          return (
+            <>
+              {readText(this.props.prefix)}
+              {child}
+            </>
+          );
         }
         return child;
       }
@@ -1900,7 +1913,13 @@ describe('ReactDOMFizzServer', () => {
       const {pipe} = renderToPipeableStream(
         <TestProvider ctx="A">
           <div>
-            <Suspense fallback={[<Text text="Loading: " />, <TestConsumer />]}>
+            <Suspense
+              fallback={
+                <>
+                  <Text text="Loading: " />
+                  <TestConsumer />
+                </>
+              }>
               <TestProvider ctx="B">
                 <TestConsumer prefix="Hello: " />
               </TestProvider>

packages/react-dom/src/__tests__/ReactServerRendering-test.js

@@ -819,7 +819,7 @@ describe('ReactDOMServer', () => {
     }
 
     function Child() {
-      return [<A key="1" />, <B key="2" />, <span ariaTypo2="no" />];
+      return [<A key="1" />, <B key="2" />, <span ariaTypo2="no" key="3" />];
     }
 
     function App() {