Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Compiler Bug]: Compiler doesn't catch ref access in render #31406

Open
1 of 4 tasks
jthemphill opened this issue Nov 2, 2024 · 1 comment
Open
1 of 4 tasks

[Compiler Bug]: Compiler doesn't catch ref access in render #31406

jthemphill opened this issue Nov 2, 2024 · 1 comment
Labels
Component: Optimizing Compiler Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug Type: Bug

Comments

@jthemphill
Copy link

What kind of issue is this?

  • React Compiler core (the JS output is incorrect, or your app works incorrectly after optimization)
  • babel-plugin-react-compiler (build issue installing or using the Babel plugin)
  • eslint-plugin-react-compiler (build issue installing or using the eslint plugin)
  • react-compiler-healthcheck (build issue installing or using the healthcheck script)

Link to repro

https://playground.react.dev/#N4Igzg9grgTgxgUxALhAegFQYDoDsAEG+AKgBYCWY+l+Y5AtuQDYCGM+ALhPlGAgMocWHBABp8AIygdqMlkyYQA7lQDmCDh3K5VnUgnysRYGSeEGEANwS48hSQgBmEGAY77aQkflIsqEhBseAAcAE3NQyQBPfFcWOA4AOjsMNDwEAA9glxlHKFwE8ggCXgEogsFzAAptci15AEp8YDt8OGKTfABtcT4OADV5KAQAXXwAXh4+SpEa3DryRoBuVvbcTtdHCamEACUnOYXl1tcOWAIu1vx8KqbxgD5Yp0S4WFdcDlErm8shhDvHi0CNcQeQtlVNi83jYZABCcaTX5MYZNIEg9FPRxQmDvGSIv4rYEY2gaQbIhBVJEownEgC+31pX2BI0J9NsuEwOAIRAAgj4IBAANacbiCwLBTgweLCiBbdwWUJ1FgSJgGMzeWV6AzaYLSZLc-AAOQg3ncwnwSgMuECkS4PhYuFCqvwLDa0GCzs1CFC6jafgQYGQKXwAEZEm76IwZPQWDEAn6FN7qAR5bQWPQDO9Qgh2IoIBK-CFwiIAKKKgZ-YMAJnDUDC5jLdTJw3wMbjBjg8lVkRYjhE7Hakbq1ConcToWDZADBjYBlCLbtqcc8RkZpkhqg9ACMAAkrhdTJpMxyAAvAOyKjKAjqhAAWgk-onBqpBjrxfPDsig6jyfwLmz7B2sEObODA9BujiCAJPgoExpo2iqPqhBpNaWQ5DB+SFMUOyNkIKoUmif64AAwr4OhiK0xQAELIjATLXHEoQAPK4EwMSTMuTB8PR+AvjxxSkQ66h7gezYUcCAkOogTA8dmy5QEwHC4Qh2ycdxeC0qiqwdDIXTqBW5JUQgCG4d6vSkn8RkmeW3pjJMpT8OUcAzBS3xySwClKeWKkAPzNLxfz4LS+DIPguAKTJrQNIS2nrDI37DpMtwTIC3xrJ0L5WTopmRJM+liVlqg5bcNKguCmXGdlNmRPCkzhQo+AAGSNQFhmVUV1WJC++C1a1KLNN81wCWR6g+YklKWe1OVdX80UMt8fQFVN1VVPVTBzUSRE0bAY0la0tIxcC6XxVJCBMNsyUPANm2LZN1l1N6q0RRt6LDUJCAidIYm7S+L0gpJBRnbtf1Dbg20wMDpUHXgsWdDCOa4S5F0Atd6JgjcsKMSxbFaZt1y3W190iKEVTAH1BiaaV1xstTh3XMdRYNuWYkXdaShiSjhGg4J5GfQZwy7WzHNU26cXk4VOXbPld1VQ9JMg9Q5Uyx1ctNS1FVE96M3kj1CJhQg7OzajGIE8MEsrWTL6hULgWUwy+BnXwxvomgaAkBQVA0I45CuD2fY5hGUa9Nww5gKQ0BMJEqjcCYUrkKopCrtwb3kYN+Cu-gADqM5gGAm5uL4MjDkozDnda1jsPG3u+w4oEWB8CPli54jh5aFdIRiKejeNNvkgrNNBXTosZcrkt5RZhOy8Te3Aqc5zO+ToUa1PSZ+cvKvE9rLZL38PHXJQyk6Dvk8b0mvVrWrPVY6xUR74HdR352gORZt8MwIjXjieib5M02u-7RpPAIBaRAA

Repro steps

Hi! I'm hoping the React compiler catches this invalid ref access and bails out of optimizing code like this, but I understand that catching this kind of issue is probably a bigger undertaking than normal.

Our team's codebase has some dubious core library hooks which break React's rules, or at least make it easy to break React's rules without knowing what you're doing. Here, useSyncState advertises itself as a cooler useState that lets you see the freshest state, even before React does! (But after memoization, this just means you see stale state). This leads to it being used incorrectly in useEditable, when we call getValueBeingEdited() inside of a render.

When I installed the compiler on our codebase, many E2E tests started failing. One of the tests showed us that when we update one EditableCell, the state change doesn't propagate to another cell like it was supposed to. The test passed after I added 'use no memo' to either useSyncState.ts or useEditable.ts. For now, I'm going to add 'use no memo' to both of these files, but my understanding is that every time I add 'use no memo', you'd like to see why I had to do so.

How often does this bug happen?

Every time

What version of React are you using?

Playground

What version of React Compiler are you using?

Playground

@jthemphill
Copy link
Author

From the compiler team's end, it looks like the compiler team has decided to handle this problem with the enableTreatRefLikeIdentifiersAsRefs option.

From my codebase's end, we'll want to replace return () => ref.current with simply return ref. We'll let consumers access ref.current, and we'll make sure that consumers name the variables coming out of these custom hooks with the ...Ref convention so that the compiler can track refs across import boundaries.

Unless the compiler does full-on taint analysis instead of relying on naming conventions, we're unlikely to get to a point where the compiler never breaks code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Optimizing Compiler Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug Type: Bug
Projects
None yet
Development

No branches or pull requests

1 participant